fix(checkbox,radio,toggle): add missing keyboard focus indicators - #31295
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good! My main feedback is we should be hiding the browser's native focus ring.
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
The code itself looks great! My requests are just for additional screenshots & some test changes.
…demo grid clipping
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
Great work! Thanks for adding more tests/screenshots. 🎉
# Conflicts: # core/src/components/checkbox/test/basic/checkbox.e2e.ts-snapshots/checkbox-focus-ios-ltr-Mobile-Chrome-linux.png # core/src/components/checkbox/test/basic/checkbox.e2e.ts-snapshots/checkbox-in-item-focus-ios-ltr-Mobile-Chrome-linux.png # core/src/components/toggle/test/basic/toggle.e2e.ts-snapshots/toggle-focus-ios-ltr-Mobile-Chrome-linux.png # core/src/components/toggle/test/basic/toggle.e2e.ts-snapshots/toggle-in-item-focus-ios-ltr-Mobile-Chrome-linux.png
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good! Mostly some questions on the expected behavior.
There was a problem hiding this comment.
I know md does this where the unchecked focus indicator is different, but this seems odd for ios. Should we make it always show as blue?
There was a problem hiding this comment.
It's definitely hard to tell. Maybe sticking to blue as Brandy said. I tried to find a native example with a keyboard on an iPad and it wasn't good. This is what it looks like but it's not really focusing on the toggle.
Blue rings on default toggles can work for now until we look into the next version styles.
There was a problem hiding this comment.
Is it expected that the checkbox focus would show when clicking on a clickable item? I thought it would still show the item's states.
There was a problem hiding this comment.
At this point, wouldn't we be using the activated state styles for this? If so, I would expect this to be in another PR.
There was a problem hiding this comment.
Normally, no (like this example), but in this case, yes because the item is clickable and the checkbox is too so you need the ring to know what has focus. When the only interactive thing in the item is the checkbox/radio, then no, it won't show the focus indicator separately. The dark background is the item's state FWIW so it's showing both here.
Showing focus when there's multiple interactive items is literally half of this PR.
There was a problem hiding this comment.
Is it expected that the radio focus would show when clicking on a clickable item? I thought it would still show the item's states.
There was a problem hiding this comment.
Same response as the checkbox question
thetaPC
left a comment
thetaPC
left a comment
There was a problem hiding this comment.
Waiting on your responses to Brandy to determine my decision.
4 participants
Issue number: internal
What is the current behavior?
Currently,
ion-checkboxandion-togglenever show a keyboard focus indicator.ion-checkboxdoesn't set theion-focusableclass and has no focus styles in theiosormdthemes, so it can't receive theion-focusedclass or render a ring.ion-toggle's only focus rule (:host(.ion-focused) input) targets the nativeinput, which isdisplay: none, so it never renders.Separately,
ion-checkbox,ion-radio, andion-toggleshow no focus indicator inside anion-itemthat holds more than one input.ion-checkboxandion-radiodisable their own indicator in an item on the assumption the item draws one, but a multi-input item has no cover to highlight, so nothing is focused.What is the new behavior?
ion-checkboxnow setsion-focusablewhen it isn't deferring to an item, with matchingiosandmdfocus styles, so a standalone checkbox shows the same indicatorion-radioalready did. Bothion-checkboxandion-radioalso become focusable inside a multi-input item, detected through the item'sitem-multiple-inputsclass, so each control shows its own indicator when the item can't.ion-togglealways shows its own indicator. Unlike checkbox and radio, it's excluded from the item's input cover, so an item never highlights on its behalf, in or out of a multi-input item. Its deadinputfocus rule is replaced with a ring on the visible track in both themes.Does this introduce a breaking change?
Other information
Preview checkbox:
Preview toggle:
Preview radio: