feat(a11y)!: let .sf-btn size scale win over the coarse-pointer 44px floor - #589
Conversation
…floor The blanket `@media (pointer: coarse)` touch-target rule in core/accessibility.css applied `min-block-size: var(--sf-touch-target)` (44px) to every <button>, including .sf-btn. On any touch device this overrode the button's own XS–XL size ladder and pinned every rung to 44px — so the size scale collapsed to one height on phones/tablets even though it renders correctly with a mouse (the "all button sizes look identical on mobile" report). .sf-btn is now excluded from that floor (`button:not([class~="sf-btn"])`; the attribute form keeps .sf-btn filed under its `components` layer in the class catalogue instead of being re-attributed to `accessibility`). Buttons honour their --sf-btn-min-height / --sf-size-* ladder everywhere, touch included. The default control (--sf-size-m, 40px) still clears WCAG 2.2 AA (24px), and .sf-btn--xs sits at exactly the 24px AA minimum. Bare <button>s and native controls keep the 44px floor. Opt back into the 44px AAA touch target with `--sf-btn-min-height: var(--sf-touch-target)` (globally or per button). - core/accessibility.css: exclude .sf-btn from the coarse-pointer floor - optional/components.css: document the touch behaviour on the size ladder - docs/migration.md: breaking-change entry + opt-in path - docs/llm-guide.md: note touch behaviour on --sf-btn-min-height - tests/button-touch.spec.js: coarse-pointer regression guard BREAKING CHANGE: on touch devices, un-customised .sf-btn--xs/--s now render at their true (smaller) height instead of being floored to 44px. Restore the old behaviour with `--sf-btn-min-height: var(--sf-touch-target)`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GnwqYHgHNAWPbsUJWUeY1w
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe coarse-pointer accessibility rule now excludes ChangesTouch-target behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR fixes the "all button sizes look identical on mobile" regression by excluding
Confidence Score: 4/5The CSS change is a single, well-scoped selector tweak with no logic outside of the @media (pointer: coarse) block; the rest of the accessibility rules are untouched. The core change is minimal and correct — the attribute-selector exclusion is behaviorally identical to :not(.sf-btn) and all surrounding rules are unchanged. The migration guide has a small selector mismatch and the new tests don't guard ctx.close() in finally blocks, but neither affects runtime behaviour. docs/migration.md (selector shown differs from CSS), tests/button-touch.spec.js (missing try/finally around ctx.close()) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["@media (pointer: coarse)"] --> B{Is it a button?}
B -->|"button:not([class~='sf-btn'])\nbare buttons"| C["Apply 44px min-block-size\n(WCAG AAA floor)"]
B -->|"button.sf-btn\n(any size modifier)"| D["Honour --sf-btn-min-height\n(size ladder: xs=24px to xl=56px)"]
D --> E{Opt-in requested?}
E -->|"--sf-btn-min-height: var(--sf-touch-target)"| F["44px floor restored"]
E -->|"No override"| G["True design-scale height >= 24px WCAG AA"]
B -->|"input, select, summary"| C
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["@media (pointer: coarse)"] --> B{Is it a button?}
B -->|"button:not([class~='sf-btn'])\nbare buttons"| C["Apply 44px min-block-size\n(WCAG AAA floor)"]
B -->|"button.sf-btn\n(any size modifier)"| D["Honour --sf-btn-min-height\n(size ladder: xs=24px to xl=56px)"]
D --> E{Opt-in requested?}
E -->|"--sf-btn-min-height: var(--sf-touch-target)"| F["44px floor restored"]
E -->|"No override"| G["True design-scale height >= 24px WCAG AA"]
B -->|"input, select, summary"| C
Reviews (1): Last reviewed commit: "feat(a11y)!: let .sf-btn size scale win ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| size ladder and pinned every rung to 44px — so the whole size scale collapsed to | ||
| one height on phones and tablets, even though it renders correctly with a mouse. | ||
|
|
||
| `.sf-btn` is now **excluded** from that blanket floor (`button:not(.sf-btn)`). |
There was a problem hiding this comment.
The migration guide documents the exclusion as button:not(.sf-btn), but the live CSS in core/accessibility.css uses button:not([class~="sf-btn"]). While these are semantically identical, a developer following the migration guide and then inspecting DevTools would see the attribute-selector form and might wonder if they're looking at a different rule. The PR description and the CSS comment both explain the attribute-selector choice; the migration doc is the only place that shows the :not(.sf-btn) shorthand without that context.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| test('size ladder stays visible on touch (xs < s < m < l < xl), not flattened to 44px', async ({ browser }) => { | ||
| const { ctx, page } = await renderTouch(browser); | ||
| const h = await heights(page, ['xs', 's', 'm', 'l', 'xl']); | ||
| for (let i = 1; i < h.length; i++) { | ||
| expect(h[i], `#${i} taller than #${i - 1}`).toBeGreaterThan(h[i - 1]); | ||
| } | ||
| // xs sits below the 44px AAA floor — proof .sf-btn is exempt from the | ||
| // blanket coarse-pointer rule — while still clearing the WCAG 2.2 AA 24px. | ||
| expect(h[0]).toBeLessThan(44); | ||
| expect(h[0]).toBeGreaterThanOrEqual(24); | ||
| await ctx.close(); | ||
| }); | ||
|
|
||
| test('opt-in: --sf-btn-min-height: var(--sf-touch-target) restores the 44px floor on every size', async ({ browser }) => { | ||
| const { ctx, page } = await renderTouch(browser, { | ||
| extraCss: ':root{--sf-btn-min-height:var(--sf-touch-target)}', | ||
| }); | ||
| const h = await heights(page, ['xs', 's', 'm', 'l', 'xl']); | ||
| for (const v of h) expect(v).toBeGreaterThanOrEqual(44); | ||
| await ctx.close(); | ||
| }); | ||
|
|
||
| test('a bare <button> (no .sf-btn) keeps the 44px floor on touch', async ({ browser }) => { | ||
| const { ctx, page } = await renderTouch(browser, { html: `<button id="b">x</button>` }); | ||
| const h = await heights(page, ['b']); | ||
| expect(h[0]).toBeGreaterThanOrEqual(44); | ||
| await ctx.close(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Each test creates a browser context via renderTouch(browser) and calls ctx.close() at the very end, but there is no try/finally guard. If any expect() assertion throws (which is the normal Playwright failure path), ctx.close() is skipped and the context leaks until the browser process is torn down. This doesn't affect correctness on a happy path, but it can cause dangling contexts during a failing test run and makes it harder to read resource-usage output in CI. Wrapping the body in try/finally { await ctx.close(); } is the idiomatic fix.
- demos/*: regenerate full-api demos so the version stamp matches v0.7.12. Inherited drift — the release bump doesn't run demos/generate.mjs, so the check-artifacts CI gate flagged the stale 0.7.11 stamp. Diff is the stamp only. - docs/migration.md: show the real `button:not([class~="sf-btn"])` selector (was the `:not(.sf-btn)` shorthand) with a one-line note on the attribute form, so the migration guide matches what DevTools shows. (greptile P2) - tests/button-touch.spec.js: wrap each touch context in a withTouch() helper with try/finally so a failing assertion never leaks the browser context. (greptile P2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GnwqYHgHNAWPbsUJWUeY1w
2 participants
Problem
On touch devices the whole
.sf-btnXS–XL size scale collapsed to one height — the recurring "all button sizes look identical on mobile" report. The framework CSS, the deployed build and the size ladder were all correct; the scale rendered fine with a mouse but flattened on phones/tablets.Root cause: the blanket
@media (pointer: coarse)rule incore/accessibility.cssappliedmin-block-size: var(--sf-touch-target)(44px) to every<button>, including.sf-btn. On any coarse pointer this overrode the button's own min-height ladder and pinned every rung (XS/S/M/L) to 44px. It was tied to the device pointer type, which is why incognito, a different browser, a clean URL and resetting overrides made no difference.Change
.sf-btnis excluded from that blanket floor, so buttons honour their--sf-btn-min-height/--sf-size-*ladder everywhere, touch included. The 44px WCAG 2.5.5 AAA target becomes an explicit opt-in.--sf-size-m, 40px) still clears WCAG 2.2 AA (24px), and.sf-btn--xssits at exactly the 24px AA minimum.<button>s (no.sf-btn) and native controls (input,select,summary) keep the 44px floor.button:not([class~="sf-btn"])rather than:not(.sf-btn)so the class-catalogue tooling (which masks quoted strings) keeps.sf-btnfiled under itscomponentslayer instead of re-attributing it toaccessibility— behaviour and specificity are identical.Opt back into the 44px AAA touch target
or, in the configurator, Min height → touch-target.
Breaking change
On touch devices, un-customised
.sf-btn--xs/.sf-btn--snow render at their true (smaller) height instead of being floored to 44px. Migration + opt-in documented indocs/migration.md.Testing
tests/button-touch.spec.js(emulated coarse pointer, Chromium): size ladder stays visible on touch (xs < s < m < l < xl, xs < 44px, ≥ 24px AA); the--sf-btn-min-height: var(--sf-touch-target)opt-in restores 44px on every rung; a bare<button>keeps the floor.tests/button.spec.js(50 tests) unchanged and green.24·33·41·51·64, touch24·40·52·67·87(px) — scales in both, no 44px flattening.lint:css,check:llm-guide,audit:check,check:version,check:registry,check:macros,check-artifacts.🤖 Generated with Claude Code
https://claude.ai/code/session_01GnwqYHgHNAWPbsUJWUeY1w
Generated by Claude Code
Summary by CodeRabbit
Accessibility
.sf-btncontrols now preserve their size variants instead of being automatically enlarged to 44px.Documentation
Tests