fix(F0NumberInput): make the stepper keyboard operable - #5196
Open
albertpmz wants to merge 1 commit into
Open
Conversation
The stepper was mouse-only. Arrows.tsx renders the increase/decrease controls as `role="button"` divs with an `onClick` and nothing else: no tabIndex, no key handler. internal.tsx had no onKeyDown either, so the input did not step on arrow keys. The failure mode was worse than "no keyboard support": the arrows are revealed by `group-focus-within`, so tabbing into the field makes them appear, and then they cannot be reached or activated. The affordance shows up precisely for the user who cannot use it. WCAG 2.1.1 Keyboard, level A. Fixed on the input, not on the arrows. ArrowUp/ArrowDown now route to the existing handleStep, which already clamps to min/max and seeds from `step` when the value is null. This is what a native number input does, and it is what a keyboard user tries first. Deliberately not adding tabIndex to the arrow divs. Un-focusable controls are outside the scope of axe's target-size rule, because widget-not-inline-matches requires _isFocusable. Making them focusable brings two 16x12 CSS px targets sitting ~12px apart into scope, failing both the size and the offset sub-check, which would break the axe enforcement this file just gained. Also moves the hardcoded English "Increase"/"Decrease" labels onto useI18n(), joining the existing numberInput namespace. axe cannot see this class of defect: focus-order-semantics is best-practice and outside the enforced WCAG tag set.
Contributor
🔍 Review policy: Code changeDefault rule: any other change needs one approval from f0-devs (rule 4). Required approvals
How this was decided
Policy source: |
Contributor
📦 Alpha Package Version PublishedUse Use |
Contributor
🔍 Visual review for your branch is published 🔍Here are the links to: |
Contributor
♿ Accessibility (axe) — components changed in this PR✅ No a11y issues in the stories this PR changed. Scope: only stories in the files/component folders this PR changed. It can't yet flag downstream ripple from shared-code/token changes, or diff against |
Contributor
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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
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.
Description
The
F0NumberInputstepper is mouse-only, which is a WCAG 2.1.1 Keyboard failure(level A) in a component tagged
stable. This adds ArrowUp/ArrowDown on the input.Type of change
Implementation details
The defect
components/Arrows.tsxrenders the increase/decrease controls asrole="button"divs carrying an
onClickand nothing else: notabIndex, no key handler.internal.tsxhad noonKeyDown, so the input did not step on arrow keys either.The shape of the failure is worse than plain "no keyboard support". The arrows are
revealed by
group-focus-within, so tabbing into the field makes them appear, andthen they cannot be reached or activated. Tab moves straight past to the next
field. The affordance shows up precisely for the user who cannot operate it, and
anyone who cannot use a mouse has no way to reach the stepper.
axe cannot catch this class of defect. The rule that flags a
role="button"with no focusability is
focus-order-semantics, which isbest-practiceandoutside the enforced WCAG tag set. #5192 turned on axe enforcement for this file
and went green on all 16 stories with the defect sitting there untouched.
The fix
onKeyDownon the input routes ArrowUp/ArrowDown into the existinghandleStep,which already clamps to
min/maxand seeds fromstepwhen the value is null.This mirrors a native number input and is what a keyboard user reaches for first.
Details worth reviewing:
preventDefault()on the two handled keys, otherwise the caret jumps to thestart/end of the field while stepping.
step,disabledandreadonly. Fields with no stepper areunaffected, so this changes behaviour only where a stepper was opted into.
NumberInputInternalPropsis aPickthat doesnot include
onKeyDown, so consumers cannot pass one today.src/ui/input.tsxdestructuresonKeyDownand forwards it tothe native
<input>, so it is not swallowed by the wrapper.Why the arrows keep no tabIndex
Adding
tabIndexto the arrow divs is the obvious-looking fix and it is the wrongone. Un-focusable controls are outside the scope of axe's
target-sizerule(WCAG 2.5.8), because
widget-not-inline-matchesrequires_isFocusable. Makingthem focusable brings two 16x12 CSS px targets, stacked roughly 12px apart, into
scope, where they fail the size sub-check and the 24px offset sub-check alike. That
would break the axe enforcement this file gains from #5192. Keying the input avoids
the problem entirely.
The reasoning is recorded as a comment above
Arrowsso the next person does notundo it.
Also in here
The hardcoded English
"Increase"/"Decrease"labels now read throughuseI18n(), joining the existingnumberInputnamespace ini18n-provider-defaults.tsalongsidebetween/greaterThan/lessThan. Thecomponent already used
useI18n()for its range hints, so these two were theoutliers.
Verification
Run from
packages/react:pnpm tsccleanoxlinton the touched trees, 0 warnings / 0 errorspnpm vitest --project=unit run src/components/F0NumberInput60 pass (was 56)pnpm vitest --project=unit run src/lib/providers/i18n4 pass, 1 skippedoxfmt --checkcleanclean, so the fix exposed no new targets
pnpm test-storybook --url … F0NumberInput15/15 pass, with axe enforced via thebase branch
Red-green
check-bugfix-red-greenagainst the stacked base passes:One honest note on the four new tests: only
ArrowUp increases and ArrowDown decreases the valueis red on the base. The otherthree assert that the value does not move (above
max, belowmin, and with nostepset), and those pass trivially on a branch where arrow keys do nothing. Theyare regression guards for the clamping and the
stepgate rather than reproductionsof this bug.
Follow-up, deliberately not here
The arrow divs stay
role="button"divs. Converting them to real<button>elements is the tidier long-term shape, but it needs the 24px hit-area work first
for the
target-sizereason above, which is a visual density change that wantsdesign sign-off and moves the Chromatic baseline.