Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisions: Fix focus ring on the item container #66642

Closed
jarekmorawski opened this issue Oct 31, 2024 · 3 comments · Fixed by #66780
Closed

Revisions: Fix focus ring on the item container #66642

jarekmorawski opened this issue Oct 31, 2024 · 3 comments · Fixed by #66780
Assignees
Labels
[Feature] History History, undo, redo, revisions, autosave. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@jarekmorawski
Copy link

Hi there! I noticed the focus ring in the Revisions panel is on each item's upper part instead of the whole container. I understand it's structured this way to make it easy for users to navigate to the Apply button using the keyboard, but the whole item is a button and as such should probably be highlighted.

Image

@jarekmorawski jarekmorawski added [Feature] History History, undo, redo, revisions, autosave. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended labels Oct 31, 2024
@ramonjd
Copy link
Member

ramonjd commented Nov 4, 2024

Thanks for reporting.

This is due to there being two focusable buttons for each revision item: one to load the styles in the preview, and one for the Apply button. The border is from the former.

Buttons can't be nested, so the were placed next to each other.

Citing @jasmussen

it’s the “button” that lets you select that particular revision ... I’d also think it the better experience to surround the whole container

So we can try to make the list items focusable when navigating between list items, and then the buttons themselves would be tabbable children (?).

Anyway, I'm sure there'll be a way... 👍🏻

@ramonjd
Copy link
Member

ramonjd commented Nov 4, 2024

Added to the general group of follow up tasks too: #60450

@ramonjd
Copy link
Member

ramonjd commented Nov 4, 2024

I'm not sure if I'll get time to work on this anytime soon, but I had a quick play around adding the following props to the list elements:

						tabIndex={ 0 }
						onKeyDown={ ( event ) => {
							if ( event.key === 'Enter' ) {
								onChange( revision );
							}
						} }

With active/focus styles on them it works as I'd expect, but there are some accessibility concerns, primarily making assigning a keyboard listener to a non-interactive element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] History History, undo, redo, revisions, autosave. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants