fix(checkbox,radio,toggle): add missing keyboard focus indicators - #31295
fix(checkbox,radio,toggle): add missing keyboard focus indicators#31295ShaneK wants to merge 14 commits into
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.
There was a problem hiding this comment.
Nit: when the window width is between 800-900px wide the 2nd checkbox is cut off:
I believe this might fix it:
.grid {
display: grid;
- grid-template-columns: repeat(3, minmax(250px, 1fr));
+ grid-template-columns: repeat(3, minmax(auto, 1fr));
grid-row-gap: 20px;
grid-column-gap: 20px;
}There was a problem hiding this comment.
Done here: a105ea7
Preview here: https://ionic-framework-git-fw-7585-fw-7586-ionic1.vercel.app/src/components/checkbox/test/item?ionic:mode=md
However, I noticed the focus now gets cut off on a small enough screen 🫠
| <ion-radio-group value="1"><ion-radio value="1" justify="start">Radio 1</ion-radio></ion-radio-group> | ||
| <ion-radio-group value="2"><ion-radio value="2" justify="start">Radio 2</ion-radio></ion-radio-group> |
There was a problem hiding this comment.
Nit, allow empty selection so we can see how it looks when not selected too:
| <ion-radio-group value="1"><ion-radio value="1" justify="start">Radio 1</ion-radio></ion-radio-group> | |
| <ion-radio-group value="2"><ion-radio value="2" justify="start">Radio 2</ion-radio></ion-radio-group> | |
| <ion-radio-group allow-empty-selection value="1"><ion-radio value="1" justify="start">Radio 1</ion-radio></ion-radio-group> | |
| <ion-radio-group allow-empty-selection value="2"><ion-radio value="2" justify="start">Radio 2</ion-radio></ion-radio-group> |
There was a problem hiding this comment.
Can we add a screenshot test for multiple checkboxes in an item?
There was a problem hiding this comment.
Can we add a screenshot test for multiple radios in an item?
There was a problem hiding this comment.
Can we add a screenshot test for checked + focused?
There was a problem hiding this comment.
Can we add a screenshot test for checked + focused?
There was a problem hiding this comment.
Can we add screenshot tests for unchecked + focused and checked + focused like the other components?
| @@ -139,23 +139,35 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c | |||
|
|
|||
| test.describe(title('checkbox: ionFocus'), () => { | |||
There was a problem hiding this comment.
I think this is in the wrong config block. It only takes screenshots for ios, we should include md since they differ in UI:
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
| @@ -96,23 +96,35 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c | |||
|
|
|||
| test.describe(title('toggle: ionFocus'), () => { | |||
There was a problem hiding this comment.
I think this is in the wrong config block. It only takes screenshots for ios, we should include md since they differ in UI:
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
…demo grid clipping
3 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: