Accessibility improvements for List View Part 2 #38358
Conversation
alexstine
added
[Type] Enhancement
alexstine
requested review from
youknowriad,
talldan,
MarcoZehe,
joedolson,
kevin940726 and
tellthemachines
|
Size Change: +1.46 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
@talldan Do you think this is too much of a stretch? I wanted to make sure the row could be expanded without first being all the way to the right but this will prevent users from focussing the Options button until the row is expanded. Do you think this is too much of a breaking change compared to how things currently work? It actually makes more sense to me this way but I can understand the Options menu may be independent of if child Blocks are showing or not. The other thing I had to change was preventing focus on Options button while state is collapsed. The reason being expanded would not be announced if focus were moved to the Options button so if the row is collapsed, it will require 2 Right Arrow key presses to focus the Options button. |
…st-view-accessibility
Yes, those states only work on the actual element, don't propagate to child elements. And because the link is the focused element within the list view item, it needs to get the expanded state. |
|
@MarcoZehe Thought so, it was an easy enough fix. I kept the aria-expanded on the row so I didn't break future code. It is possible other parts of code would not use child element as it is being used here. |
|
Good thinking. This also makes it clearer when navigating the grid with the virtual cursor, in NVDA, for example. |
There was a problem hiding this comment.
I've had a play with this, and to me, this makes a lot of sense. This includes the part where you first expand a collapsed element before moving to the options button. Even as a keyboard user, you may want to have a glance at what's hidden under that collapsed column etc. before you move it or such. I'm giving this my approval, but please only merge this once the others have agreed.
|
@MarcoZehe Thanks for the review. Yep, I'll be holding off here until I get a decision on if this will be allowed or not. I agree, better UX for sure. |
|
@talldan Thanks for the review.
I am not sure I understand. Essentially, I only want aria-expanded to change if the current index is equal to 0 that way the first column controls the expand/collapse. If the row is collapsed and I press Left Arrow again, nothing happens. Can you give me steps on how to replicate this? Do I need child of child Blocks?
Pushed a commit to fix this, should work fine now. |
| nextIndex !== 0 && | ||
| activeRow.getAttribute( 'aria-expanded' ) === 'false' | ||
| ) { | ||
| nextIndex -= 1; |
There was a problem hiding this comment.
I think the part that's confusing about this code is that it doesn't take into account whether the user presses right or left keys. I guess the intention is to prevent any changes to the index? Basically stopping focus from moving? I don't know that it's necessary because the return statement on line 135 prevents the same thing.
It seems to work better for me if I change it to something like this to prevent focus moving beyond the right edge of the grid:
if (
keyCode === RIGHT &&
nextIndex === currentColumnIndex
) {
return;
}
Thanks. That does fix pressing the left arrow key, but it still seems possible to press the right arrow key when at the right edge of the grid, and focus moves unexpectedly to the left. I don't think that's intentional, but let me know if it is. I left a couple of comments on why that might be. |
|
@talldan What makes this really difficult is that second column and knowing when to move back and forth. In an ideal world, it should work like this.
Index 0 being the Block and index 1 being the Options button. Index 0 = Left Arrow, index 1 = Right Arrow. Pushed a fresh commit, working better? |
| if ( | ||
| nextIndex !== 0 && | ||
| keyCode === RIGHT && | ||
| activeRow.getAttribute( 'aria-expanded' ) === 'false' | ||
| ) { | ||
| nextIndex -= 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think the problem I have reviewing here is that I'm not really sure what the desired behavior is.
Should the right key always be prevented from doing anything when a row is collapsed?
Something that needs to be considered is that the TreeGrid component is general purpose. List View has two columns, but another implementation of TreeGrid might have more than two columns, so any code added should take that into account.
I could have a TreeGrid with four columns. Say that the third column of a collapsed row is currently focused. Is the expectation that pressing right does nothing?
There was a problem hiding this comment.
@talldan The problem I am trying to solve is this.
- I only want the Block selection button to be able to expand and collapse the child Blocks. In the List View, that is the 1st column with index of 0.
- The current model is not very user friendly for screen reader users. To expand, you must find the collapsed Block, press Right Arrow to focus the Options button, and then Right Arrow again to expand. The problem is, all the way at the right, the user will not hear the expanded state get updated because Options button contains:
aria-expanded="false"
Because the Options menu for the Block is closed. The only way to know for sure that the row was expanded again is to either navigate with Down Arrow to the first child Block Options button or to press Left Arrow and you should hear the expanded state on the Block selection button.
To be honest, I think this needs a dedicated component as what we're trying to do here is really pretty complex. If not, I have another option that may work.
- Expand the List View to 3 columns.
- Block name column which will be used to expand/collapse, link that jumps to the Block, and finally the Block Options button.
- This way we don't have to worry about relying on table position to expand/collapse. We simply listen for an onClick for the first column.
It may be easier to show you why this doesn't work in practice. If you would like, happy to jump on a quick Zoom and I can show you the big UX improvement between trunk and my branch. Maybe that would be better, that way you can visualize exactly what the problem is?
Thanks for hanging in there with me.
There was a problem hiding this comment.
I hadn't considered the issue with the options button having its own aria-expanded.
Testing again now and I think the only remaining issue I see in List View is that it's possible to this:
- Add a paragraph and a columns block
- Focus the columns block in List View and collapse it
- Press the up key (focus the paragraph), then the right key (focus the options button of the paragraph), then the down key (focus the options button of the columns block)
- Press the right key
Expected: Nothing happens
Actual: The collapsed column is expanded.
The issue here might be screen reader announcements, since the options button's aria-expanded won't change.
Maybe the right arrow key to expand should only work in the first column.
There was a problem hiding this comment.
Yes, that's what I am trying to say. To expand should only happen in the 1st column. The 1st column should be responsible for collapsing and expanding.
There was a problem hiding this comment.
I'm not offering much more than has been said, but I've been comparing this PR's changes with trunk.
On trunk the expand does indeed occur on the second td element with a right arrow click.
This PR changes the expand to the first td element.
I can reproduce Dan's scenario above, but it appears to be consistent for all options buttons, that is, I cannot right arrow (only down arrow to the lower blocks' options, or left to select the row).
This appears fairly intuitive to me as I'm at the end item of a single row. The caveat is that I mainly navigate using a pointer. :)
There was a problem hiding this comment.
@ramonjd The only problem with an approach like this is the way the current column index is working. If I am in the Options column and I press Left Arrow, it will return a current index of 1 which makes no sense because I should not be in the column anymore. None the less, it doesn't work for whatever reason. Calculating indexes like this is turning out to be really unreliable when you want a specific column to do the expanding. It will only return 0 once you are at the edge of the left column or something like that. I don't have vision so can't honestly tell you exactly what it is doing.
How about instead of passing a prop, I attach a data-column-trigger="true"? I know @talldan didn't want to go that way but for me, it seems like it would be safer. That way I can always know that data tag when focus is placed on it, the collapse/expand can work.
Thoughts?
There was a problem hiding this comment.
That's a fair point, thanks for explaining. I was, in a clumsy way, trying to advocate for some sort of flag to tell TreeGrid to expand at a certain point.
How about instead of passing a prop, I attach a data-column-trigger="true"?
This sounds like it's worth a shot - I would tend to trust @talldan's judgement on this, but it would be good to see working. Maybe he'll come around ;)
There was a problem hiding this comment.
The only problem with an approach like this is the way the current column index is working. If I am in the Options column and I press Left Arrow, it will return a current index of 1 which makes no sense because I should not be in the column anymore. None the less, it doesn't work for whatever reason.
This is the same function that handles the arrow key press and moves focus, so currentColumnIndex is always the index before focus has moved.nextIndex is where focus will end up if the function doesn't return early.
I think @ramonjd's code should work. I wonder if it's needed though, with what's being proposed it seems like column zero would always be the column where expands/collapse works.
The trouble is that this code previously just handled moving focus, but now more stuff has been added to it, and it's a little confusing. If this code were to be rewritten, I think it might be best to assign some well named variables to make the code more readable:
const isCollapsed = activeRow.getAttribute( 'aria-expanded' ) === 'false';
const lastColumnIndex = focusablesInRow.length - 1;
const shouldExpand = isCollapsed && currentColumnIndex === 0 && keyCode === RIGHT;
const shouldCollapse = ! isCollapsed && currentColumnIndex === 0 && keyCode === LEFT;
const shouldMoveLeft = ! shouldCollapse && currentColumnIndex > 0 && keyCode === LEFT;
const shouldMoveRight = ! shouldExpand && currentColumnIndex < lastColumnIndex && keyCode === RIGHT;There was a problem hiding this comment.
@talldan I thought at one point you were worried about having another column that could possibly be the one doing the expanding/collapsing. E.g.
<table>
<tr>
<td></td>
<td>Expand/Collapse</td>
</tr>
<tr></tr>
</table>
That is why I was working on extra code to try to add support for any column specified by a developer but adapt it for the List View. If you are okay with column index of 0 (first column) doing all the expanding and collapsing, let me clean the code up.
There was a problem hiding this comment.
Probably a miscommunication. I'm ok with it just being the first column 👍
…st-view-accessibility
…st-view-accessibility
…ndex. Update comments.
| // Focus is either at the left or right edge of the grid. Allow Right Arrow to expand the row. | ||
| if ( | ||
| nextIndex === currentColumnIndex || | ||
| ( activeRow.hasAttribute( 'aria-expanded' ) && |
There was a problem hiding this comment.
Instead of checking the index, I opted to check the activeRow to see if it contained aria-expanded or not. This in theory should be more compatible with possible future uses as it doesn't require a hard coded index value which could change depending on how many td/column elements there are.
There was a problem hiding this comment.
Hey @alexstine , before merging would you mind adding an entry to the CHANGELOG ?
Thank you!
|
@talldan This looking better? |
|
@ciampo My Changelog update looks good? Thanks. |
Improvement for more than 2 <td> elements. Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
|
@talldan Changes implemented and still working fine on my end. Any final bug searches? |
|
Seems to work well! Thanks for all your patience, I know I held this one up a bit, appreciate you working with me on the review changes. |
Description
I have made further improvements to block-editor/list-view and components/tree-grid.
Testing Instructions
This should also be easy to test on gutenberg.run. Just enter this PR number to build.
Screenshots
Types of changes
Improvement and breaking change
Checklist:
*.native.jsfiles for terms that need renaming or removal).