List view: Refactor ARIA attributes #48461
Conversation
alexstine
added
[Type] Enhancement
|
Size Change: +49 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
@afercia Can you give this one a test on Mac and provide any necessary feedback? I think this will lead to a much better experience. I also opened #48462 to help deal with the deselection bug I found. My big concern around this was the links adding extra verbosity. I think I made it less of an issue by forcing the selection announcement to happen as assertive, but not sure if it is good enough. It would be nice if I could somehow disable the link announcement while selection is happening but I think Thanks. |
|
Related PR: #39272 Maybe conditionally adding Thanks. |
There was a problem hiding this comment.
Thanks for the ping @alexstine! I'm happy to take your and @afercia's lead on what works best here in terms of where the aria attributes are placed in the list view. From reading this PR, my understanding is that we can remove the aria-hidden attribute by shifting most of the aria attributes to the link, so that the attributes that are announced are consolidated around the link, rather than the cell. That seems like good logic to me!
I've added a couple of questions just to make sure that I'm following along correctly. From a code perspective, the only issue I ran into is the removal of an if check that I think might still need to be there, since it was included a while back to fix a bug.
| @@ -112,20 +111,13 @@ function ListViewBlock( { | |||
| level | |||
| ); | |||
|
|
|||
| let blockAriaLabel = __( 'Link' ); | |||
| if ( blockInformation ) { | |||
There was a problem hiding this comment.
I notice that the condition to check if blockInformation is truthy has been removed. From a little digging, I think the check was added back in #38775 to deal with an issue with useSelect where it's possible that blockInformation will be null in some circumstances. So, I'm not too sure, but it might be necessary to keep the condition around. Just pinging @talldan who originally implemented the fix.
There was a problem hiding this comment.
If that is the case, please check the variable directly below this line. It was not included in this conditional and still used blockInformation to get the title for building the options aria-label.
There was a problem hiding this comment.
In the case of the below settingsAriaLabel it's doing a check for blockInformation within its ternary before attempting to access blockInformation.title, so it didn't need to be included in the if block. For blockAriaLabel both outcomes of the ternary are accessing properties of blockInformation, so if blockInformation were to be null then an error would be thrown.
There was a problem hiding this comment.
I'm not too sure if that check is still needed, though, but because #38775 introduced the check, it'd probaly be good to verify before removing the check.
There was a problem hiding this comment.
Ah, yeah, I totally missed that sneaky conditional on that line. NVM.
…st-view-aria-attributes
|
Flaky tests detected in cffe7d1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4623025067
|
…st-view-aria-attributes
There was a problem hiding this comment.
Just took this for another spin. I like the idea of this change — is there anything I can do to help progress this PR?
I noticed there's a failing e2e test, and it seems we can fix it up by changing a couple of lines.
In the test should multi-select in the ListView component with shift + click we need to update this line to use the link role, and for the name to be Paragraph. For example:
const navButtons = listView.getByRole( 'link', {
name: 'Paragraph',
} );
Then, further down in that test, where it checks that the third link is focused, because navButtons will be a list of links, we can remove the .getByRole call. So this line would become:
await expect( navButtons.nth( 3 ) ).toBeFocused();
Let me know if you'd like me to commit to this branch, and I can push a fix.
…st-view-aria-attributes
…st-view-aria-attributes
There was a problem hiding this comment.
@andrewserong Notes below. This is ready for another review round now that I finally fixed all the screen reader bugs.
One thing that stands out to me, the ListView has a lot of properties no longer used from the looks of it. Should create a follow-up issue to figure out if we're passing stuff around that is doing nothing. Makes the code so hard to understand what is doing what. Each iteration only makes it worse.
@afercia Would still appreciate your review on this since it is a fairly large refactor.
| @@ -35,6 +35,9 @@ function ListViewBlockSelectButton( | |||
| onDragStart, | |||
| onDragEnd, | |||
| draggable, | |||
| isExpanded, | |||
There was a problem hiding this comment.
Not sure why this component was no longer receiving this prop.
| @@ -277,9 +249,10 @@ function ListViewBlock( { | |||
| currentlyEditingBlockInCanvas ? 0 : tabIndex | |||
| } | |||
| onFocus={ onFocus } | |||
| isExpanded={ isExpanded } | |||
| isExpanded={ canExpand ? isExpanded : undefined } | |||
There was a problem hiding this comment.
Changed to match the others.
| @@ -144,7 +144,7 @@ export default function useBlockSelection() { | |||
| } | |||
|
|
|||
| if ( label ) { | |||
| speak( label ); | |||
| speak( label, 'assertive' ); | |||
There was a problem hiding this comment.
Unrelated to this PR but this required a fix.
| @@ -28,7 +29,7 @@ function UnforwardedTreeGridRow( | |||
| aria-level={ level } | |||
| aria-posinset={ positionInSet } | |||
| aria-setsize={ setSize } | |||
| aria-expanded={ isExpanded } | |||
There was a problem hiding this comment.
This is the best area to explain this. The reason why this needs to exist is as follows.
- If aria-expanded exists on the tr and the link, it is announced twice when it is toggled.
- If it only exists on the row, it is announced when toggled but not after that.
- If it exists on the link only, it works fine when toggled and after.
- Had to figure out how to eliminate it off the tr.
- The better way about this may be to add data-expanded directly here and pass the value from the
ListView. Something like this.
aria-expanded={ disableAriaExpanded ? undefined : isExpanded }
data-expanded={ disableAriaExpanded ? isExpanded : undefined }
One of these attributes is required for the keyboard functionality. Not pretty, but it works.
|
@andrewserong Added a fallback for the possible title bug. |
There was a problem hiding this comment.
Thanks for all the updates @alexstine and for refining this PR! From a code quality standpoint, I think this is in a good place to land 👍
Nice work tidying up the use of blockTitle, too, and overall I like the way this PR now uses the data-expanded attribute for the styling rules and as a fallback within the tree grid component. I think that creates a useful bit of separation between the data we're passing around for behaviour and styling, and where we deliberately flag values for accessibility.
In terms of accessibility, the removal of aria-hidden and moving the label, description, and expanded values to the button level all sound good to me.
You might like to get another review, too, before landing, but I've retested that the list view is still working the same as on trunk via keyboard and mouse. I'll be AFK until mid-next week, but if there's any more follow-up, I'm happy to help out then.
LGTM, and great work again! ✨
|
@getdave @scruffian I see you have some recent PRs working on list view. Mind giving this a review as well? |
|
I think this would be a good PR to get in. I've just added a wider ping in case we can get some extra eyes on it. I notice there's a branch conflict with |
|
I can refresh it soon. Just can be really aggravating at times. Seems like people try to avoid reviewing my PRs excluding @andrewserong of course. 👍 I am fairly certain this will be a decent change for screen reader UX but getting any further testing time seems to be near impossible. Even mentioned it in a meeting. |
|
Thanks for the update, Alex! I think once the changelog is updated, it might be worth us trying out merging the PR, if we don't hear back from anyone. Sometimes folks might be cautious about reviewing a particular area of the code base if they aren't too familiar with it. Once the PR's merged, if there's any pushback or follow-up issues, I'm happy to help debug or revert if need be, in case that helps us get this one over the line! I'm wrapping up for the week now, but can jump back in to help next week. Thanks again for all your work improving the accessibility here. |
|
Merged. We can make follow-ups if necessary. Next task, migrate this work to the other editors. 👍 |
|
This works well in NVDA for me; and it's a significant improvement in JAWS. |
@alexstine Apologies I couldn't get to this as I was AFK |
|
Apologies @alexstine for the delay in the review — I haven't been able to contribute to Gutenberg for a couple of weeks, and I'm slowly catching up.
As Andrew also mentioned, not everyone may be comfortable reviewing this part of the repository and/or have valuable feedback on this particular feature — although I can guarantee you that no one is trying to avoid reviewing your PRs. I also wanted to express again my gratitude for the work that you, @joedolson and other folks do daily for improving the accessibility of this project. |
@andrewserong , do you think we could look for an alternative approach here? We shouldn't really be using internal component's classnames. Could we find a way to improve the |
|
@ciampo Understood but it can be very hard to move these things forward without feedback. There will be much bigger, much messier changes to come so I hope when that time is here, I will see a little more community involvement. I got the accessibility part covered but I am very new to React and never totally sure that I am doing things the correct way. This is a learning experience for all of us. Hope you enjoyed your time away, we all need that for sure. 👍 |
Oh, good idea! Yes, I'm happy to open up a quick follow-up PR, we should be able to use the
No worries @ciampo, and thanks for following up — it's never too late for good feedback! 🙂 |
|
I have a tiny PR in #50155 to swap out the usage of |
Thank you! I'll take a look :)
That makes a lot of sense. It would be great if you managed to work on that at some point, but no immediate rush :) |
What?
This PR attempts to remove
aria-hidden="true"from the links in the list view.Why?
This was a hack to get around block selection bugs in list view. I think I found a way to make it no longer necessary. There is a little extra screen reader verbosity, but I do not think this is a bad thing since the selection announcement will now take priority with
aria-live="assertive".How?
Changes multiple ARIA attributes and ensure
speak()function gets assertive argument.Testing Instructions
Testing Instructions for Keyboard
Provided above.
Screenshots or screencast