Skip to content

fix(status-toggle): use space key to activate radio items#1982

Open
akashsonune wants to merge 1 commit intomainfrom
fix/a11y-status-toggle-space-key
Open

fix(status-toggle): use space key to activate radio items#1982
akashsonune wants to merge 1 commit intomainfrom
fix/a11y-status-toggle-space-key

Conversation

@akashsonune
Copy link
Copy Markdown
Member

@akashsonune akashsonune commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for the space key to trigger the selectItem function in the si-status-toggle component. The review correctly identifies that $event.preventDefault() should be called when handling the space key to prevent unintended browser behavior like page scrolling.

Comment thread projects/element-ng/status-toggle/si-status-toggle.component.html Outdated
@dr-itz
Copy link
Copy Markdown
Member

dr-itz commented Apr 29, 2026

I think these need to go from tab-activated to arrow left/right. i.e. the active one should have tabindex=0, the others tabindex=-1 and arrow/left (with RTL compensation) should move the focus and active the element.

But. In some use-cases changing the status triggers a potential heavy operation, so an explicit activation with space is probably more desirable but then we also need to wrap the the whole thing into a role=toolbar (i.e. on the host itself)

@akashsonune akashsonune force-pushed the fix/a11y-status-toggle-space-key branch from 24703af to f9a4955 Compare April 30, 2026 12:56
@akashsonune
Copy link
Copy Markdown
Member Author

I think these need to go from tab-activated to arrow left/right. i.e. the active one should have tabindex=0, the others tabindex=-1 and arrow/left (with RTL compensation) should move the focus and active the element.

But. In some use-cases changing the status triggers a potential heavy operation, so an explicit activation with space is probably more desirable but then we also need to wrap the the whole thing into a role=toolbar (i.e. on the host itself)

Done. I think it also mentions with the toolbar, the arrow navigation should be move around i.e. from last element to the first and first to last(back arrow)

@akashsonune akashsonune force-pushed the fix/a11y-status-toggle-space-key branch from f9a4955 to a2fb775 Compare April 30, 2026 13:03
@akashsonune akashsonune requested a review from dr-itz April 30, 2026 13:05
@akashsonune
Copy link
Copy Markdown
Member Author

Also I noticed that, selecting the item with mouse click does not focus it. so when we use keyboard later on , the focus is elsewhere while the selection is different. this was also the case earlier with tab navigation

@akashsonune akashsonune marked this pull request as ready for review April 30, 2026 13:08
@akashsonune akashsonune requested review from a team as code owners April 30, 2026 13:08
@akashsonune akashsonune force-pushed the fix/a11y-status-toggle-space-key branch from a2fb775 to 66b899a Compare April 30, 2026 14:38
@dr-itz
Copy link
Copy Markdown
Member

dr-itz commented May 1, 2026

The problem I see now is that we get a focus border when operated with the mouse. This shouldn't happen

@akashsonune
Copy link
Copy Markdown
Member Author

akashsonune commented May 1, 2026

The problem I see now is that we get a focus border when operated with the mouse. This shouldn't happen

@dr-itz That was intentional. As I mentioned above, the focus will be elsewhere , when user tries to use keyboard navigation.
I see similar pattern at other examples- https://carbondesignsystem.com/components/content-switcher/usage/
also https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio/

@dr-itz
Copy link
Copy Markdown
Member

dr-itz commented May 4, 2026

The problem I see now is that we get a focus border when operated with the mouse. This shouldn't happen

@dr-itz That was intentional. As I mentioned above, the focus will be elsewhere , when user tries to use keyboard navigation. I see similar pattern at other examples- https://carbondesignsystem.com/components/content-switcher/usage/ also https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio/

Yes, but the visual focus ring isn't expected when using the mouse. Other controls don't do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants