feat: add MD3 keyboard focus indicators - #5084
Open
lukemorawski wants to merge 3 commits into
Open
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
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 I will rebase and this diff
drops to just the focus work.
What
MD3 keyboard focus indicators, from one shared implementation, on every component
built on
TouchableRippleplusCardandChip's close button.The ring is the platform's own
outline, not a bordered child view. It costs nolayout, takes its corner radius from the view it sits on, and is not clipped by that
view's own
overflow: 'hidden', soborderlessis not a problem.outline*landedin RN 0.77 and Paper's floor is already 0.81 via the reanimated peer dep.
New
src/utils/useFocusRing.tsholds the hook, the style helper and the onewebNoOutline.FAB's local hook is deleted andFAB,CheckboxandSwitcharemoved onto the shared one, so the three hand-rolled rings become one.
TouchableRipplegets afocusRingprop,'outward' | 'inward' | 'none'.Placement
Outward by default, which is what
outerOffsetinsrc/theme/tokens/sys/state.tsis for. Inward onChip,List.Item,RadioButton.ItemandSegmentedButtons.Not a style preference. An outward ring is 5dp outside the box, and it gets trimmed
by any clipping ancestor sized to its content. A row of chips in a horizontal
ScrollViewis the case that shows up in this repo's own example app, same on weband Android: the ring loses its top and bottom and renders as two arcs. Inward fits.
List.ItemandSegmentedButtonsare inward for the neighbour version of the sameproblem, full bleed rows and flush segments. material-web does the same split and
uses inward for its list item and tab.
Going the other way and making inward the default does not work either. MD3 tonal
palettes are luminance matched by tone, so a
secondaryring drawn on top of anotherrole at the same tone is about 1:1 and disappears. On a contained
Buttonfill thatis 1.00:1. Outward puts it on the page background instead, 6.13:1 light and 10.91:1
dark. There is a test for this :)
Notes for review
isKeyboardFocusEvent.Pressable's ownfocusedstate fires for clicks, Pressable: focus-visible support necolas/react-native-web#1849.
webNoOutlinestays onFABandSwitchonly, since both ring an inner element.Nowhere else sets
outline: none, so a JS failure still leaves the browser ring.opacity: 0;this one only exists while focused. Index based tree queries against
FABandSwitchwill shift.enableImperativeFocusis off bydefault. Pre existing, the old
FAB/Checkbox/Switchrings were inert there too.docs/scripts/generate-current-component-docs.tsdeletes itsoutput dir before parsing and then crashes on
satisfies. Pre existing, needs it's own PR.Testing
yarn typecheck,yarn lint,yarn testcleanTwo tricky things that cost me time:
TouchableRippleimport resolves to.native, so the web file goesuntested unless you import it by extension
disabledelement, so negative focus assertions passfor free
Manual pass per component on web and Android, hardware keyboard: ring on tab, no ring
on mouse click, not clipped. iOS rendering only, for the reason above.