feat(banner): compact phone layout for the notice buttons - #307
Open
fabiodalez-dev wants to merge 2 commits into
Open
fabiodalez-dev wants to merge 2 commits into
fabiodalez-dev wants to merge 2 commits into
Conversation
Below 440px the shipped templates give each notice button a full-width row of its own, so the notice is 372px tall on a 390x844 phone - 44% of the viewport, and past half of it on a 375x667 screen. Reported by a user whose banner took more than half the screen on mobile while rendering correctly on desktop. Add banner_control.mobile_layout with two values. 'comfortable' is the default and is byte-for-byte the current behaviour, so no installed site changes appearance on update; 'compact' lays the buttons out on a shared row. Measured against a live 1.32.0 site at five widths, before and after: 430px 351px 41.6% -> 223px 26.4% one row 390px 372px 44.1% -> 244px 28.9% one row 375px 372px 43.0% -> 244px 28.2% one row 360px 372px 41.3% -> 296px 32.9% two rows 320px 336px 33.2% -> 260px 25.7% two rows Three constraints shaped the CSS, and none of them is cosmetic: - `flex: 1 1 0` makes accept and reject exactly as wide as each other because one layout pass sizes them, not because two widths were written to match. EDPB Guidelines 03/2022 require equal prominence between accepting and refusing, and that has to survive translation into any language - which hardcoded widths do not. - `min-height: 44px` keeps the tap target at the size a finger needs. Reclaiming height by shrinking the target would trade one usability problem for a worse one. - Under 360px three buttons no longer fit side by side, so accept and reject stay paired on their own row and the customise button - the longest label, and the one that takes no part in the equal-prominence comparison - drops to a row of its own. The rules are emitted after boost_css_specificity(), already carrying their `#faz-consent` prefix, so they are specificity 1-1-0 against the template's own rules and win on document order without a single `!important`. A first prototype written at class specificity failed at 360px and below against a live site, which is what the prefix is there to prevent. The mobile layout is part of the boosted-CSS transient key. Keyed on the template alone, toggling the setting would keep serving yesterday's stylesheet for a day and the setting would look broken; the generated banner-<md5>.css filename already derives from the assembled CSS, so a switch mints a new file rather than overwriting a cached one. It is a site-wide value, not a per-visitor one, so it stays invariant under Cache Compatibility Mode. The stored value is whitelisted on save and re-checked on read. It decides which CSS is emitted, so a row written before the sanitiser existed, or by hand, must not reach the stylesheet. Tests: 13 sanitiser assertions covering the whitelist, array input through the full settings pipeline, case and whitespace normalisation and the shipped default; a new E2E spec asserting the three properties that a later CSS edit could silently break - that compact actually shortens the notice, that accept and reject keep identical size on a shared row, and that every button stays 44px tall - plus that no label is clipped. Both suites were confirmed to fail when the compact CSS is disabled, which on the E2E side required flushing the boosted-CSS transient: the first control run passed against a stale cached stylesheet and proved nothing.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: fabiodalez-dev/FAZ-Cookie-Manager/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. WalkthroughIl banner aggiunge l’opzione ChangesLayout mobile del banner
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The compact layout is opt-in and preserves comfortable as the default. No actionable merge-blocking issue remains in the available evidence; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The option selects between two fixed presentation layouts, retains the existing appearance by default and remains behind administrator permissions. No newly expanded attack path was identified. Interrupted updates and external page-cache recovery remain unverified, so the assessment does not declare zero risk. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The compact rules assume the notice has exactly three buttons. A combined "GDPR + US" banner (the documented applicableLaw='gdpr' + donotSell mode) renders a fourth .faz-btn — the Do-Not-Sell button — which the max-width:360px override does not re-base, so it collapses to a sliver at ≤360px. See the inline comment.
Reviewed changes
banner_control.mobile_layoutsetting — newcomfortable(default, byte-identical to current behaviour) /compactvalue; whitelisted insanitize_option(), added toget_excludes()so an array payload is coerced rather than recursed against a scalar default, and defaulted inget_defaults().- Compact CSS —
compact_mobile_css()returns a fixed stylesheet appended last inprepare_banner_styles(), already carrying the#faz-consentprefix so it wins on document order at equal-or-higher specificity. Cache key bumped tofaz_boosted_css_v3_and now includes the layout. - Settings UI — new "Mobile Layout" card with a
data-pathselect; populated through the existingFAZ.populateFormpath, so the hardcodedselectedis harmless. - Tests — a Playwright spec (row count, viewport-height ratio, accept/reject parity, ≥44px tap targets, no clipped labels, 320px fallback) plus sanitizer/
get_defaults()unit assertions. The spec asserts the comfortable baseline stacks and the compact layout actually shortens the notice, so it can fail if the CSS stops applying. - Translations — POT/PO churn from regeneration plus the new strings (only
it_IT.morecompiled). Mechanical.
I confirmed the default path emits an empty string so installed sites do not change appearance, that the REST endpoint (banner-rest) and the PHP render share the assembled CSS, and that the compact selectors really are specificity 1-2-0 and emitted after boost_css_specificity().
ℹ️ Verification scope is the three-button English GDPR notice
The layout switch is global, but the E2E spec only exercises the accept/reject/customize notice in English. The CCPA notice and combined banners render a Do-Not-Sell button (the case below), and translated/custom labels are never measured — so a fixed-width free-space layout that relies on white-space:nowrap has no regression guard for those. Consider at least a fourth-button assertion in the existing spec.
Technical details
# Coverage gap: non-GDPR banners and non-English labels
## Affected sites
- `tests/e2e/specs/mobile-compact-layout.spec.ts` — buttons are selected only by `.faz-notice-btn-wrapper .faz-btn` and identified by `/accept/`, `/reject|decline|refuse/`, so only the shipped English labels are covered.
- `frontend/class-frontend.php`:6908-6925 — layout is width-driven with `white-space:nowrap`; nothing bounds a label wider than its flex-basis.
## Required outcome
- The compact layout is exercised against a banner that carries a Do-Not-Sell control (CCPA or combined) at ≤360px, and against at least one label longer than the shipped English strings.
## Open questions for the human
- Is the compact layout intended for CCPA / combined banners too, or should the setting be gated to GDPR-only notices?ℹ️ Nitpicks
admin/modules/settings/includes/class-settings.php:516-518 — the comment says the value is "interpolated into a CSS class name on the banner container", butmobile_layoutis never interpolated anywhere; it selects which CSS is emitted and forms part of the transient key (frontend/class-frontend.php:6852-6864 describes this correctly). Worth correcting so the whitelist rationale matches the real attack surface.frontend/class-frontend.php:6908-6911 — compact forceswhite-space:nowrapon the notice buttons, where the template's mobile rule hadwhite-space:unset. Combined withmin-width:0andflex:1 1 0, a label longer than its one-third basis overflows and overlaps the next button instead of wrapping. Fine for the shipped English labels; worth a second look for narrower buttons.
Important
Pullfrog covered this run's model usage. DeepSeek Flash is fast and cheap — expect lighter work than a frontier model. This model allowance is temporary and separate from your Pullfrog plan. Connect a model-provider subscription or API key →
deepseek-v4.1-flash (model usage covered by Pullfrog) ｜ 𝕏
…layout Addresses both findings on the compact-layout PR. The Do-Not-Sell button is the fourth control in the notice wrapper and the only one with no `order` of its own — not in the template's mobile rules either, so at order:0 it already sorted ahead of Accept. Stacked full-width that was merely odd. On a single row it broke: it had to share the first line with the accept/reject pair, and under 360px their 40% bases left it almost nothing to grow into, so it collapsed to a few pixels while the inherited white-space:nowrap pushed its label across Accept. It now takes a full-width row of its own, last, behind the two options the equal-prominence rule actually compares (EDPB 03/2022 governs accept against reject, not this control). `white-space` is reset there as well: "Do Not Sell or Share My Personal Information" does not fit 270px at 13px, the wording is statutory and cannot be shortened, so the box gives instead. The selector matches on data-faz-tag because the two variants of the control share nothing else — the shortcode emits either a `.faz-btn` button or a bare `<a>` with no class at all. This is not a hand-built configuration. Geo_Runtime turns donotSell on for a US visitor even when applicableLaw stays 'gdpr', and class-template.php then keeps the button precisely because its status is true, so once jurisdiction routing lands this is the ordinary rendering for part of the audience — and the row where a US visitor's opt-out lives. Also bumps the assembled-CSS pipeline revision to v4. The compact rules are appended after $raw_css is hashed into the cache key, so without the bump an install that had already cached v3 within this same plugin version would keep serving the stylesheet in which the control collapses. That revision exists for exactly this case. The second finding was a comment, not behaviour: the mobile_layout sanitiser claimed the value is interpolated into a CSS class name on the banner container. It never reaches markup — it selects which stylesheet compact_mobile_css() returns and forms part of the cache key. The whitelist is still right; the stated reason was not, so it now says what the guard is actually for. Covered by a third case in the compact-layout spec, at 320px with the combined mode configured: the control must not share a row with either half of the pair, must span the row, must come last, and must neither overflow nor overlap. It restores the banner in a finally so a mid-test throw cannot leak a fourth button into later specs, and it clears the assembled-CSS transient first, so a red means the CSS is wrong rather than stale. Control run: removing the new rule turns exactly the shared-row assertion red.
There was a problem hiding this comment.
✅ No new issues found. Both earlier threads are addressed and resolved.
Reviewed changes
This run covers c9118632, the commit pushed after the initial review.
- Do-Not-Sell control gets its own row: a new compact rule matches
[data-faz-tag="donotsell-button"]and gives the controlorder:4; flex:1 1 100%; white-space:normal. I checked both shortcode variants inshortcodes.json(6.0.0 and 6.2.0), and each one carries that attribute, so the classless<a>variant is covered too. The rule is specificity 1-2-0 and is emitted after the.faz-btnrule, and the 360px block doesn't override it. - Cache key bumped to
faz_boosted_css_v4_: sites that already cached the v3 stylesheet within the same plugin version will rebuild it. - Sanitizer comment corrected: the
mobile_layoutwhitelist comment now says what the value is actually used for. It picks which stylesheet is emitted and is part of the cache key. It never reaches markup. - New 320px E2E case for the combined mode: the test turns on
donotSellunderapplicableLaw='gdpr', deletes the stale assembled-CSS transients, and checks four things: the control sits on a separate row below the accept/reject pair, spans the full row, doesn't overflow, and doesn't overlap the pair. The banner row is restored from a snapshot infinally.
ℹ️ Nitpicks
frontend/class-frontend.php:6896: thecompact_mobile_css()docblock still says the rules are "specificity 1-1-0". The selectors are#faz-consent .faz-notice-btn-wrapper .faz-btn*, and the new attribute selector is the same weight, so all of them are 1-2-0. Document order still decides the result, so this is only a wording fix.
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported by a user whose banner took more than half the screen on a phone while rendering correctly on desktop (
cabinhub.co.uk). Below 440px the shipped templates give each notice button a full-width row of its own, so the notice is 372px tall on a 390×844 phone — 44% of the viewport, and past half of it on a 375×667 screen.This adds
banner_control.mobile_layoutwith two values.comfortableis the default and is byte-for-byte the current behaviour, so no installed site changes appearance on update;compactlays the buttons out on a shared row.Measured against a live 1.32.0 site at five widths, before and after:
Three constraints shaped the CSS, and none of them is cosmetic.
flex: 1 1 0makes accept and reject exactly as wide as each other because one layout pass sizes them, not because two widths were written to match. EDPB Guidelines 03/2022 require equal prominence between accepting and refusing, and that has to survive translation into any language — which hardcoded widths do not.min-height: 44pxkeeps the tap target at the size a finger needs. Reclaiming height by shrinking the target would trade one usability problem for a worse one.Under 360px three buttons no longer fit side by side, so accept and reject stay paired on their own row and the customise button — the longest label, and the one that takes no part in the equal-prominence comparison — drops to a row of its own.
The rules are emitted after
boost_css_specificity(), already carrying their#faz-consentprefix, so they are specificity 1-1-0 against the template's own rules and win on document order without a single!important. A first prototype written at class specificity failed at 360px and below against a live site, which is what the prefix is there to prevent.Two things worth knowing about how this was tested. The setting is part of the
prepare_banner_styles()transient key, because without that a site toggling the layout would keep being served yesterday's stylesheet for a day. And the first control run of the E2E spec passed with the CSS disabled — it was reading a stalefaz_boosted_css_v3_transient. After clearing it both tests went red with the right message, which is the only reason I trust them: a test that cannot fail proves nothing.Covered by
tests/e2e/specs/mobile-compact-layout.spec.ts(viewport height ratio, single row at 390px, accept and reject identical in width, height and top offset, every button ≥44px, no clipped labels, plus the paired fallback at 320px) and 13 assertions in the settings sanitiser suite, since the value is interpolated into a CSS class name and so is whitelisted rather than passed through.Summary by CodeRabbit