Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CSS range syntax in media queries #1833

Merged

Conversation

SohamPatel46
Copy link
Contributor

@SohamPatel46 SohamPatel46 commented Jan 28, 2025

Summary

Fixes #1696

Relevant technical choices

Using CSS range syntax helps eliminate frontend issues where windows are sized sub-pixel dimensions.

This converts media queries from

@media (min-width: 481px) and (max-width: 600px) { 
    #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 400px; } 
}

to

@media (480px < width <= 600px) { 
    #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 400px; } 
}

The above conversion results in eliminating sub-pixel dimension issues possible between 600px to 601px screen.

@westonruter westonruter added [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature labels Jan 28, 2025
@westonruter
Copy link
Member

Here's a script you can use to update the snapshots once the actual.html files have been generated:

#!/bin/bash
for actual in $( find . -name 'actual.html' ); do
	expected=${actual/actual/expected};
	echo "$actual => $expected"
	mv "$actual" "$expected"
done

There should probably be something like this added to scripts in package.json.

@SohamPatel46
Copy link
Contributor Author

Here's a script you can use to update the snapshots once the actual.html files have been generated:

#!/bin/bash
for actual in $( find . -name 'actual.html' ); do
	expected=${actual/actual/expected};
	echo "$actual => $expected"
	mv "$actual" "$expected"
done

There should probably be something like this added to scripts in package.json.

Hello @westonruter I didn't get your above message. Is this for updating PHP unit tests ?

@westonruter
Copy link
Member

Yes, the unit test failures are due to the HTML snapshots being out for date now that the media query is updated. So you need to move all the actual.html files generated during the unit tests over to replace the expected.html (if indeed the changes are as expected!) That bash script is a way to do so in bulk.

@@ -34,27 +34,27 @@ public function data_to_test_od_generate_media_query(): array {
'mobile' => array(
'min_width' => 0,
'max_width' => 320,
'expected' => '(max-width: 320px)',
'expected' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this null? It should be unchanged.

),
'mobile_alt' => array(
'min_width' => null,
'max_width' => 320,
'expected' => '(max-width: 320px)',
'expected' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. This should be unchanged, right?

Comment on lines 52 to 57
'expected' => '(min-width: 601px)',
'expected' => null,
),
'desktop_alt' => array(
'min_width' => 601,
'max_width' => null,
'expected' => '(min-width: 601px)',
'expected' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.86%. Comparing base (5a1e472) to head (4c01373).
Report is 16 commits behind head on trunk.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1833      +/-   ##
==========================================
+ Coverage   65.82%   65.86%   +0.03%     
==========================================
  Files          88       88              
  Lines        6865     6873       +8     
==========================================
+ Hits         4519     4527       +8     
  Misses       2346     2346              
Flag Coverage Δ
multisite 65.86% <100.00%> (+0.03%) ⬆️
single 38.45% <44.44%> (-0.04%) ⬇️

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.

@SohamPatel46 SohamPatel46 marked this pull request as ready for review January 31, 2025 10:49
Copy link

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: SohamPatel46 <[email protected]>
Co-authored-by: westonruter <[email protected]>

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

@westonruter
Copy link
Member

westonruter commented Jan 31, 2025

I'm just realizing that this isn't going to solve the potential sub-pixel rendering issue. Look at this diff:

Screenshot_20250131-072705

Note that min-width: 481px and 481px < width are identical in behavior. Only the syntax is different. For there to be any change in behavior the new syntax would need to change to 480px < width, so decrementing the minimum bound by one.

See correction below: #1833 (comment)

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1833 (comment).

The od_generate_media_query() function may need to have the semantics of its parameters changed, so that it is understood somehow the passed $minimum_viewport_width is not inclusive meaning that the viewport can never exactly equal that minimum width but it can be greater than it. In contrast, the $maximum_viewport_width is inclusive, in that the viewport width can exactly equal the max.

This may end up going down a rabbit hole because the URL Metric Groups have minimum and maximum viewport widths too, so then their semantics would probably need to change as well.

This will take some more thought.

if ( $has_min_width && $has_max_width ) {
$media_attributes = sprintf( '( %dpx < width <= %dpx )', $minimum_viewport_width, $maximum_viewport_width );
} elseif ( $has_min_width ) {
$media_attributes = sprintf( '(min-width: %dpx)', $minimum_viewport_width );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the new syntax be used?

} elseif ( $has_min_width ) {
$media_attributes = sprintf( '(min-width: %dpx)', $minimum_viewport_width );
} elseif ( $has_max_width ) {
$media_attributes = sprintf( '(max-width: %dpx)', $maximum_viewport_width );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@westonruter
Copy link
Member

westonruter commented Feb 1, 2025

Note that min-width: 481px and 481px < width are identical in behavior. Only the syntax is different. For there to be any change in behavior the new syntax would need to change to 480px < width, so decrementing the minimum bound by one.

Correction: they aren't actually identical in behavior. In the min-width: 481px syntax, the viewport width can be exactly 481px whereas in the new syntax it can never be exactly 481px since it must be greater. So this means the result is worse. This emphasizes the need to somehow have this output 480px < width.

@westonruter
Copy link
Member

The od_generate_media_query() function may need to have the semantics of its parameters changed, so that it is understood somehow the passed $minimum_viewport_width is not inclusive meaning that the viewport can never exactly equal that minimum width but it can be greater than it. In contrast, the $maximum_viewport_width is inclusive, in that the viewport width can exactly equal the max.

This may end up going down a rabbit hole because the URL Metric Groups have minimum and maximum viewport widths too, so then their semantics would probably need to change as well.

This will take some more thought.

Take a look at #1839.

Comment on lines -188 to +202
$style_rules[] = sprintf(
'@media %s { #%s { min-height: %dpx; } }',
od_generate_media_query( $minimum['group']->get_minimum_viewport_width(), $minimum['group']->get_maximum_viewport_width() ),
$style_rule = sprintf(
'#%s { min-height: %dpx; }',
$element_id,
$minimum['height']
);

$media_feature = od_generate_media_query( $minimum['group']->get_minimum_viewport_width(), $minimum['group']->get_maximum_viewport_width() );
if ( null !== $media_feature ) {
$style_rule = sprintf(
'@media %s { %s }',
$media_feature,
$style_rule
);
}
$style_rules[] = $style_rule;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes a bug I noticed which can happen when a site is configured to have zero breakpoints (which is very unlikely). In this case, the return value of od_generate_media_query() is null since the minimum is 0 and the maximum is null.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working well in my testing!

@westonruter
Copy link
Member

@felixarntz Please take a look when you get a chance.

@westonruter westonruter changed the title Updates media query generator function to use CSS range syntax Use CSS range syntax in media queries Feb 4, 2025
@westonruter westonruter merged commit 2fa59f0 into WordPress:trunk Feb 4, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider switch to CSS range syntax for media queries in Optimization Detective
3 participants