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

Patterns list and grid: incorrect headings hierarchy and wrong usage of ARIA attributes #52253

Closed
afercia opened this issue Jul 3, 2023 · 6 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 3, 2023

Description

Note: In the following screenshots, I'm using the HeadingsMap browser extension (available for Chrome and Firefox) to visually illustrate the headings in the page.

In the Site Editor > Design > Patterns, the available patterns are shown in a series of screen by type..

1
In these screens, the headings hierarchy is incorrect: it jumps from H1 level to H4:

incorrect h4

That happens also in the 'My patterns' screen:

incorrect h4 grid

Additionally, all the H5 headings highlighted in the screenshot below are places within a container that user aria-hidden="true". As such, they're not perceivable by assistive technologies. In any case, the H5 heading level is incorrect:

hidden h5

These headings should either be perceivable (by removing aria-hidden="true"from the container) and use a correct level or not be heading. Everything is inside ARIA listbox / options and the options are labeled so I don't see the need to use a H5 in the first place. Why use a H5 and then hide it by teh means of aria-hidden?

However, I'd reconsider the listbox / options implementation as it introduces unnecessary complex interaction for screen reader users. This i sjust a list of objects, visually presented in a grid. I'd recommend to use a standard list.

To my understanding, this headings part comes from previous code that was moved / rearranged to add the Patterns management screens. I guess part of the incorrect headings hierarchy comes from pre-existing code but it would be great that all developers always check the headings levels are correct, as they're of fundamental importance for screen rearer users to efficiently navigate the content. ❤️ Cc @aaronrobertshaw

2
The visually hidden description of each pattern:

  • For user patterns, it is not a real description of the pattern: it's more generic instructions / help text e.g. 'press Enter to do this, Delete, to do that etc.' It's the same text for all user patterns.
  • It uses aria-describedby incorrectly. As explained in [Library] Add lock icon for theme patterns #51990 (comment) aria-describedby accepts an ID reference. Instead, in this case it is passed the description text:
Screenshot 2023-07-03 at 16 08 38

Step-by-step reproduction instructions

  • Go to Site Editor > Design > Patterns
  • Use the HeadingsMap browser extension.
  • Navigate the various screens.
  • Observe the headings hierarchy is incorrect.
  • In the dev tools Inspector, observe each pattern does have a visually hidden description. However, the aria-describedbby attribute on the listed pattern doesn't point to the ID of the hidden description. Instead, it incorrectly holds the description text.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Labelling [Package] Edit Site /packages/edit-site labels Jul 3, 2023
@annezazu
Copy link
Contributor

annezazu commented Jul 3, 2023

Thanks so much for opening. Adding to the 6.3 board but can you clarify what beta version you're using and/or GB?

@aaronrobertshaw
Copy link
Contributor

Thank you for the detailed issue! 🙇

It might also be worth flagging some other discussion around the listbox/options implementation in #52009. It would be great to get your thoughts on the proposed solution there @afercia.

I've added this to the Patterns Tracking Issue as well and will try and at least get a fix in for the headings soon while leaving any potential change to a simple list implementation as a follow-up.

@ramonjd
Copy link
Member

ramonjd commented Jul 4, 2023

Would aria-details be appropriate in a list element? E.g.,

			<li className={ patternClassNames }>
				<CompositeItem
					className={ previewClassNames }
					{ ...composite }
					onClick={ item.type !== PATTERNS ? onClick : undefined }
					onKeyDown={ isUserPattern ? onKeyDown : undefined }
					aria-label={ item.title }
					aria-details={
						ariaDescriptions.length ? descriptionId : undefined
					}
				>
					{ isEmpty && __( 'Empty pattern' ) }
					{ ! isEmpty && <BlockPreview blocks={ item.blocks } /> }
				</CompositeItem>
				<div hidden id={ descriptionId }>
					{ ariaDescriptions.map( ( ariaDescription, index ) => (
						<p key={ index }>{ ariaDescription }</p>
					) ) }
				</div>

@aaronrobertshaw
Copy link
Contributor

@afercia can this be closed now that #52273 & #52263 have been merged?

@afercia
Copy link
Contributor Author

afercia commented Jul 7, 2023

@aaronrobertshaw thanks for the fix and the ping. ❤️ Yes I think this can be closed.
Is there already an issue to keep track of the discussion about reconsidering the patterns grid / roving tabindex implementation and use a simple list instead? Thinking that part of the points from this issue and from #52270 would need to be moved to a new issue. Any thoughts?

Nevermind: I just saw #52270 (comment)

@aaronrobertshaw
Copy link
Contributor

aaronrobertshaw commented Jul 7, 2023

Is there already an issue to keep track of the discussion about reconsidering the patterns grid / roving tabindex implementation and use a simple list instead?

I don't believe we have a dedicated issue at present for this, only the in-progress PR aiming to use a simple ul list and solve a few a11y issues with the patterns list.

You're welcome to add your thoughts and feedback there. Would that be sufficient or do you think we need another issue?

Edit: Sorry, I already hit send before I saw your comment update 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

No branches or pull requests

5 participants