Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

westonruter · 2025-02-01T06:29:06Z

Summary

This is a dependency to fix #1696 and blocks #1833 from being merged. This change allows for creating media queries that avoid any 1px gaps. The key code change is in OD_URL_Metric_Group::is_viewport_width_in_range() (and isViewportNeeded in detect.js). This also avoid the need to add the number 1 to the minimum viewport width which can be confusing given the known breakpoints. This instead moves the addition of 1 at the moment to od_generate_media_query() but this will be removed with resolution of #1696.

As part of this, the viewport width and height for a URL Metric are now required to be at least 1px, whereas before they could theoretically be 0px (which doesn't make sense).

While auditing the types after the above change, I also improved the type specificity with PHPStan's richer types.

Note that all of the snapshot tests for Optimization Detective, Image Prioritizer, and Embed Optimizer are all passing without any modification in this PR. This shows that the code change does not impact the output.

…th remains inclusive This will allow for creating media queries that avoid any 1px gaps. The key code change is in `OD_URL_Metric_Group::is_viewport_width_in_range()` (and `isViewportNeeded` in `detect.js`). This also avoid the need to add the number 1 to the minimum viewport width which can be confusing given the known breakpoints. This instead moves the addition of 1 at the moment to `od_generate_media_query()` but this will be removed with resolution of #1696. Require that a viewport width and height for a URL Metric be at least 1px.

github-actions · 2025-02-01T06:36:36Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter · 2025-02-01T06:37:44Z

plugins/optimization-detective/class-od-link-collection.php

-					$last_link['maximum_viewport_width'] + 1 === $link['minimum_viewport_width']
+					$last_link['maximum_viewport_width'] === $link['minimum_viewport_width']


Key change. The addition of 1 is no longer needed because the previous maximum is intended to equal the next minimum.

westonruter · 2025-02-01T06:41:52Z

plugins/optimization-detective/class-od-url-metric-group-collection.php

-		$groups    = array();
-		$min_width = 0;
-		foreach ( $this->breakpoints as $max_width ) {
-			$groups[]  = new OD_URL_Metric_Group( array(), $min_width, $max_width, $this->sample_size, $this->freshness_ttl, $this );
-			$min_width = $max_width + 1;
+		$groups              = array();
+		$min_width_exclusive = 0;
+		foreach ( $this->breakpoints as $max_width_inclusive ) {
+			$groups[]            = new OD_URL_Metric_Group( array(), $min_width_exclusive, $max_width_inclusive, $this->sample_size, $this->freshness_ttl, $this );
+			$min_width_exclusive = $max_width_inclusive;
 		}
-		$groups[] = new OD_URL_Metric_Group( array(), $min_width, PHP_INT_MAX, $this->sample_size, $this->freshness_ttl, $this );
+		$groups[] = new OD_URL_Metric_Group( array(), $min_width_exclusive, PHP_INT_MAX, $this->sample_size, $this->freshness_ttl, $this );


Key change. Note how there is no longer the need to add 1 to the maximum width to become the minimum width of the next group.

We could consider replacing PHP_INT_MAX here with null, to allow the maximum viewport width to be positive-int|null. The logic would then need to check if OD_URL_Metric_Group::is_viewport_width_in_range() the maximum_viewport_width is null, and if so it could always return true if the provided viewport width is greater than the minimum_viewport_width. This would match what I've implemented client-side in the isViewportNeeded() function to avoid exporting PHP_INT_MAX to the frontend.

Note that od_generate_media_query() supports this behavior, where you can either supply 0 or null to omit as the minimum viewport width to omit the min-width media query, whereas you can also supply null or PHP_INT_MAX as the maximum viewport width to omit the max-width.

I like the idea of getting rid of the need for PHP_INT_MAX here, to no longer have to export it, which in practice feels excessive anyway. I don't anticipate any viewports being wider than, let's say 10,000 px, and that's probably already excessive. I'm sure there are such cases, but in any case that's still miles away from PHP_INT_MAX :)

So no longer using a concrete number here and instead truly indicating "unbounded" makes more sense to me.

I've implemented this in b4ea6bd

westonruter · 2025-02-01T06:42:13Z

plugins/optimization-detective/class-od-url-metric-group.php

-			$viewport_width >= $this->minimum_viewport_width &&
+			$viewport_width > $this->minimum_viewport_width &&


Key change!

westonruter · 2025-02-01T06:42:43Z

plugins/optimization-detective/class-od-url-metric.php

-							'minimum'  => 0,
+							'minimum'  => 1,
 						),
 						'height' => array(
 							'type'     => 'integer',
 							'required' => true,
-							'minimum'  => 0,
+							'minimum'  => 1,


Important: Note how the minimum width/height for a URL Metric is now 1px (i.e. positive-int)

westonruter · 2025-02-01T06:43:08Z

plugins/optimization-detective/detect.js

+ * The comparison logic here corresponds with the PHP logic in `OD_URL_Metric_Group::is_viewport_width_in_range()`.
+ *
 * @param {number}                 viewportWidth          - Current viewport width.
 * @param {URLMetricGroupStatus[]} urlMetricGroupStatuses - Viewport group statuses.
 * @return {boolean} Whether URL Metrics are needed.
 */
 function isViewportNeeded( viewportWidth, urlMetricGroupStatuses ) {
-	let lastWasLacking = false;
-	for ( const { minimumViewportWidth, complete } of urlMetricGroupStatuses ) {
-		if ( viewportWidth >= minimumViewportWidth ) {
-			lastWasLacking = ! complete;
-		} else {
-			break;
+	if ( viewportWidth === 0 ) {
+		return false;
+	}
+
+	for ( const {
+		minimumViewportWidth,
+		maximumViewportWidth,
+		complete,
+	} of urlMetricGroupStatuses ) {
+		if (
+			viewportWidth > minimumViewportWidth &&
+			( null === maximumViewportWidth ||
+				viewportWidth <= maximumViewportWidth )
+		) {
+			return complete;
 		}
 	}
-	return lastWasLacking;
+	return false;


Key change.

plugins/optimization-detective/detection.php

codecov · 2025-02-01T06:44:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.82%. Comparing base (d94977f) to head (b4ea6bd).
Report is 6 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1839      +/-   ##
==========================================
- Coverage   65.89%   65.82%   -0.07%     
==========================================
  Files          88       88              
  Lines        6878     6865      -13     
==========================================
- Hits         4532     4519      -13     
  Misses       2346     2346

Flag	Coverage Δ
multisite	`65.82% <100.00%> (-0.07%)`	⬇️
single	`38.48% <23.07%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

westonruter · 2025-02-01T06:47:00Z

plugins/optimization-detective/helper.php

 		return null;
 	}
 	$media_attributes = array();
 	if ( null !== $minimum_viewport_width && $minimum_viewport_width > 0 ) {
-		$media_attributes[] = sprintf( '(min-width: %dpx)', $minimum_viewport_width );
+		$media_attributes[] = sprintf( '(min-width: %dpx)', $minimum_viewport_width + 1 );


The addition of 1 here can be redone as part of #1833. /cc @SohamPatel46

On the issue you mentioned potential issues with half pixels. By no means a blocker, but I would think that doing this here still leaves that problem unresolved for the largest viewport's minimum width.

Is there no way to use an exclusive minimum without providing a maximum with the new range syntax?

Looks like you can: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Using_media_queries#:~:text=%40media%20(width%20%3C%3D%201250px)%20%7B

Yes, but I figured to do that as part of #1833 since doing that change here would cause all of the snapshots to be invalidated.

See https://github.com/WordPress/performance/pull/1833/files#r1939991349

westonruter · 2025-02-02T23:53:33Z

plugins/optimization-detective/storage/class-od-url-metrics-post-type.php

-			array_map(
-				static function ( OD_URL_Metric $url_metric ): array {
-					return $url_metric->jsonSerialize();
-				},
-				$group_collection->get_flattened_url_metrics()
-			),
-			JSON_UNESCAPED_SLASHES // No need for escaped slashes since not printed to frontend.
+			$group_collection->get_flattened_url_metrics(),
+			JSON_UNESCAPED_SLASHES // No need for escaping slashes since this JSON is not embedded in HTML.


This is a bonus change referenced in #1156 (comment)

felixarntz

@westonruter This looks great, just a few minor comments. I think it would be great to implement getting rid of PHP_INT_MAX, see https://github.com/WordPress/performance/pull/1839/files#r1939774669.

felixarntz · 2025-02-03T17:34:06Z

plugins/optimization-detective/class-od-url-metric-group.php

+	 * @phpstan-param int<0, max> $minimum_viewport_width
+	 * @phpstan-param int<1, max> $maximum_viewport_width
+	 * @phpstan-param int<1, max> $sample_size
+	 * @phpstan-param int<0, max> $freshness_ttl


Neat, I didn't know you could do this :)

plugins/optimization-detective/detect.js

felixarntz · 2025-02-03T17:37:09Z

plugins/optimization-detective/detection.php

 */
 function od_get_cache_purge_post_id(): ?int {
 	$queried_object = get_queried_object();
-	if ( $queried_object instanceof WP_Post ) {
+	if ( $queried_object instanceof WP_Post && $queried_object->ID > 0 ) {


Is this just to satisfy PHPStan? Seems a bit odd of a check.

Yes, it is for the sake of PHPStan. It's so we can say that the function returns a positive-int. Technically $queried_object->ID can be zero or negative since at the the_post action it can be overridden. In fact, the Customizer does this for nav menus which aren't yet saved in the DB: it provides negative values for the IDs to indicate nav menu items that haven't yet been inserted.

plugins/optimization-detective/detection.php

felixarntz · 2025-02-03T17:39:32Z

plugins/optimization-detective/helper.php

 		return null;
 	}
 	$media_attributes = array();
 	if ( null !== $minimum_viewport_width && $minimum_viewport_width > 0 ) {
-		$media_attributes[] = sprintf( '(min-width: %dpx)', $minimum_viewport_width );
+		$media_attributes[] = sprintf( '(min-width: %dpx)', $minimum_viewport_width + 1 );


On the issue you mentioned potential issues with half pixels. By no means a blocker, but I would think that doing this here still leaves that problem unresolved for the largest viewport's minimum width.

Is there no way to use an exclusive minimum without providing a maximum with the new range syntax?

felixarntz

@westonruter Thanks, LGTM!

westonruter · 2025-02-04T01:52:40Z

plugins/optimization-detective/detect.js

+			( null === maximumViewportWidth ||
+				viewportWidth <= maximumViewportWidth )
+		) {
+			return complete;


Oops. This logic should be inverted as return ! complete;

Fixed in 4c01373 as part of #1833.

westonruter added 2 commits January 31, 2025 22:03

Improve specificity of types

b503cf3

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Feb 1, 2025

westonruter requested a review from felixarntz as a code owner February 1, 2025 06:29

westonruter mentioned this pull request Feb 1, 2025

Use CSS range syntax in media queries #1833

Merged

westonruter added this to the optimization-detective 1.0.0-beta2 milestone Feb 1, 2025

Avoid dumping PHP_INT_MAX to frontend

5915e81

westonruter commented Feb 1, 2025

View reviewed changes

plugins/optimization-detective/detection.php Outdated Show resolved Hide resolved

westonruter commented Feb 1, 2025

View reviewed changes

Remove needless array_map with JsonSerializable

0dafbb7

westonruter force-pushed the update/viewport-handling branch from ef74635 to 0dafbb7 Compare February 2, 2025 23:36

westonruter commented Feb 2, 2025

View reviewed changes

felixarntz reviewed Feb 3, 2025

View reviewed changes

Eliminate use of PHP_INT_MAX to represent the maximum viewport width

b4ea6bd

westonruter requested a review from felixarntz February 3, 2025 19:45

felixarntz approved these changes Feb 3, 2025

View reviewed changes

westonruter merged commit 5a1e472 into trunk Feb 3, 2025
22 of 24 checks passed

westonruter deleted the update/viewport-handling branch February 3, 2025 23:03

westonruter commented Feb 4, 2025

View reviewed changes

westonruter mentioned this pull request Feb 4, 2025

Compute responsive sizes attribute based on the width from the boundingClientRect in captured URL Metrics #1840

Merged

westonruter mentioned this pull request Feb 12, 2025

Prepare PL 3.9 Release #1852

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

westonruter commented Feb 1, 2025 •

edited

Loading

github-actions bot commented Feb 1, 2025 •

edited

Loading

westonruter Feb 1, 2025

westonruter Feb 1, 2025

westonruter Feb 1, 2025

felixarntz Feb 3, 2025

westonruter Feb 3, 2025

westonruter Feb 1, 2025

westonruter Feb 1, 2025

westonruter Feb 1, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

westonruter Feb 1, 2025 •

edited

Loading

felixarntz Feb 3, 2025

felixarntz Feb 3, 2025

westonruter Feb 3, 2025

westonruter Feb 3, 2025

westonruter Feb 2, 2025

felixarntz left a comment

felixarntz Feb 3, 2025

felixarntz Feb 3, 2025

westonruter Feb 3, 2025

felixarntz Feb 3, 2025

felixarntz left a comment

westonruter Feb 4, 2025

		$last_link['maximum_viewport_width'] + 1 === $link['minimum_viewport_width']
		$last_link['maximum_viewport_width'] === $link['minimum_viewport_width']

		$viewport_width >= $this->minimum_viewport_width &&
		$viewport_width > $this->minimum_viewport_width &&

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

Conversation

westonruter commented Feb 1, 2025 • edited Loading

Summary

github-actions bot commented Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

westonruter Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Feb 1, 2025 •

edited

Loading

github-actions bot commented Feb 1, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading

westonruter Feb 1, 2025 •

edited

Loading