Opened 5 weeks ago
Last modified 5 weeks ago
#65841 new enhancement
theme.json settings.viewport: warn when a breakpoint is dropped
| Reported by: | bejignesh | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | Editor | Version: | 7.1 |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: |
Description
Follow-up to #65833, requested by @wildworks in comment:3.
WP_Theme_JSON::sanitize_viewport_settings() drops tablet in two cases and says nothing either time:
tabletis not larger thanmobile. Existing behaviour, unchanged by #65833.mobileandtabletare measured against different bases, onepxand the otheremorrem, so their order cannot be determined. Added in #65833.
In both cases the theme gets a two state layout instead of the three states it configured, and nothing reports why.
A notice would match how the same class already reports invalid settings. settings.spacing.spacingScale does this in compute_spacing_sizes():
wp_trigger_error( __METHOD__, sprintf( /* translators: 1: theme.json, 2: settings.spacing.spacingScale */ __( 'Some of the %1$s %2$s values are invalid' ), 'theme.json', 'settings.spacing.spacingScale' ), E_USER_NOTICE );
The two cases are not equivalent and may deserve different wording, or possibly only the first deserves a notice at all. A tablet smaller than mobile is a plain configuration mistake that is visible in the theme.json itself. Mixed bases are not something an author can reason about from the file or from the generated CSS, because the font size the media query resolves against belongs to the browser rather than to the page.
Not suitable for 7.1, since a notice needs a translatable string and 7.1 is past hard string freeze. That is why the fix in #65833 is silent.
Change History (2)
This ticket was mentioned in PR #12974 on WordPress/wordpress-develop by @hasnainashfaq.
5 weeks ago
#1
- Keywords has-patch has-unit-tests added
#2
@
5 weeks ago
I've opened a PR for Case 1 (tablet not larger than mobile): https://github.com/WordPress/wordpress-develop/pull/12974
The fix adds an elseif branch in sanitize_viewport_settings() that fires wp_trigger_error() with E_USER_NOTICE when the tablet breakpoint is dropped, following the same pattern used in compute_spacing_sizes() for invalid spacingScale values.
Case 2 (mixed em/px bases) is left for a follow-up once #65833 lands.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Fixes the silent failure described in #65841.
When
WP_Theme_JSON::sanitize_viewport_settings()drops thetabletbreakpoint because its value is not larger thanmobile, it now triggers awp_trigger_error()E_USER_NOTICEinforming theme authors of the reason.What the problem was:
When a theme.json sets a
tabletviewport breakpoint whose value is equal to or smaller thanmobile, thetabletkey is silently removed. The theme gets a two-state layout instead of the three states it configured, with no indication of why.What the fix does:
Adds an
elseifbranch insanitize_viewport_settings()that fireswp_trigger_error()withE_USER_NOTICEwhentabletis present but not larger thanmobile. This matches the pattern already used incompute_spacing_sizes()for invalidspacingScalevalues.Testing:
wp_trigger_error_runand asserts the notice fires with the correct message content whentablet≤mobile.test_get_viewport_media_queries_omits_tablet_when_its_breakpoint_is_not_larger_than_mobile()continues to verify the sanitized output is correct.Notes:
The ticket also describes a second case (mixed em/px bases, added by #65833) that cannot be addressed here until #65833 lands. This PR covers only Case 1 from the ticket description.
See https://core.trac.wordpress.org/ticket/65841