@k0ndee/focus indicators - #5107
Draft
k0ndee wants to merge 5 commits into
Draft
Conversation
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.
Fixes #5083
Supersedes #5084 (@lukemorawski's original implementation - includes his three commits as-is). This rebuilds the web half of the ring per feedback that it should be plain CSS
:focus-visible, not JS-tracked focus state, and fixes a couple of things a fresh pass turned up.Based on #5080 and contains its two commits, since a fork cannot open a stacked PR. Worth reviewing after that one merges, at which point this diff drops to just the focus work.
Motivation
No component in the library had a visible keyboard focus indicator, and the three that improvised one (
FAB,Checkbox,Switch) each did it differently. MD3 requires a visible focus indicator on every interactive component; this adds one shared implementation, used everywhereTouchableRippleis, plusCardandChip's close button.What
The ring is the platform's own
outline, not a bordered child view. It costs no layout, takes its corner radius from the view it sits on, and is not clipped by that view's ownoverflow: 'hidden', soborderlessis not a problem.outline*landed in RN 0.77 and Paper's floor is already 0.81 via the reanimated peer dep.src/utils/useFocusRing.tsholds the one shared hook.FAB's local hook is deleted;FAB,Checkbox,Switch,Card, andChipall use the shared one.TouchableRipplegets afocusRingprop,'outward' | 'inward' | 'none'.Changed from #5084: the web ring is now pure CSS, not JS-tracked state. A
data-focus-ring[-within]attribute plus one injected stylesheet key off the browser's own:focus-visible/:has(:focus-visible); only the (theme dependent) ring color crosses into CSS, as a custom property. Nothing tracks focus in JS on web anymore - the browser drives it. Native keeps JS-tracked state (onFocus/onBlur+ a liveoutline*style), since it has no CSS.Placement
Outward by default, which is what
outerOffsetinsrc/theme/tokens/sys/state.tsis for. Inward onChip's body and close button,FAB's menu items, andSwitch's track.Not a style preference. An outward ring sits outside the box and gets trimmed by any clipping ancestor sized to its content - a row of chips in a horizontal
ScrollViewloses its top and bottom and renders as two arcs. Inward fits. Making inward the default instead doesn't work either: MD3 tonal palettes are luminance-matched by tone, so asecondaryring on top of another role at the same tone is close to 1:1 contrast and disappears. Outward puts it on the page background instead, where contrast is predictable.Notes for review
:focus-visiblesemantics directly - no JS heuristic needed there anymore. Native still usesisKeyboardFocusEvent, which is hardcodedtruethere: RN doesn't distinguish touch- from keyboard-triggeredonFocuson native, relying on the fact that a plain touch tap doesn't normally callonFocuson a non-text control in the first place.TouchableRipple's ring is now keyed off the caller's realdisabledprop, not the internaldisabled || !hasPassedTouchHandlerconflation used elsewhere in that file (a separate, tracked bug in the primitive) - so the ring isn't coupled to it.Chip's close button is a plainPressable, not aTouchableRipple, so it callsuseFocusRingdirectly instead of going through thefocusRingprop like the chip body does - now called out in a comment so the two patterns don't read as an oversight.FAB'sShell/Menupassundefinedas the disabled argument touseFocusRing, since neither has adisabledprop today - also now commented, as a flag for whoever adds one later.tokens.md.sys.state.focusIndicatorinstead of hardcoding them, matching the web stylesheet test's own stated rule.-becomeFirstResponder/-resignFirstResponderfor hardware-keyboard/Full Keyboard Access navigation, but only emits theonFocus/onBlurJS event when the nativeenableImperativeFocusfeature flag is on, and it defaults off (old architecture has no equivalent path at all). Pre-existing - the oldFAB/Checkbox/Switchrings were equally inert on iOS before this PR.opacity: 0; the ring markup now only exists while actually focused.Test plan
yarn typecheck,yarn lint,yarn testclean.enableImperativeFocusnote above (rendering only, not a regression check).