Fix visual and DOM order of the list view header tabs and close button. #58942
Conversation
afercia
added
[Focus] Accessibility (a11y)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
afercia
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: -3 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the PR and the context @afercia. This is testing nicely for me, and doesn't appear to have any effect on the list view other than altering the tab sequence. I see that @andrewhayward added feedback on the approach in the linked issue here: #58940 (comment). Personally, I don't have a preference for which tab sequence in this particular case, I can see there are good arguments either way. One thing that will need to be updated prior to landing is that the e2e tests currently expect the behaviour that exists on trunk. E.g.
gutenberg/test/e2e/specs/site-editor/list-view.spec.js
Lines 116 to 128 in 84715c0
And:
gutenberg/test/e2e/specs/editor/various/list-view.spec.js
Lines 358 to 370 in a2d9738
I'll just add another couple of folks familiar with the list view accessibility in case they have opinions, too.
|
I wasn't particularly involved in it, but I'm sure I recall that the implementation in trunk was for accessibility reasons It looks like @alexstine reworked the tabs (in Add TabPanel to document overview replacing fake tabs), and in that PR the close button was positioned absolutely to appear on the right, but would be focused first via its position in the DOM order. It was modified to use Some feedback from those folks would be good. |
|
@talldan thanks, I did check the PR where it was modified. |
afercia
force-pushed
the
fix/list-view-header-visual-dom-order
branch
from
4f1dffe to
13b0cea
Compare
That seems like an assumption. I personally think it'd be better to get feedback directly before undoing a change another contributor made. |
There was a problem hiding this comment.
I left some feedback in the tracking issue, as it's more conceptual than specific to this fix.
|
Flaky tests detected in 9dabda6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7928086807
|
There was a problem hiding this comment.
@afercia Thanks for the PR. I agree this is something that should be fixed but not like this.
Is there any way to fix the visual order so you have close, tabs, then tab panel content? I do not want the tab order to find the close button before the tabs. This makes the tabs undiscoverable to keyboard users as they will be able to assume that there is no content before the close button as that is generally the first item in a region/dialog.
That seems like an assumption. I personally think it'd be better to get feedback directly before undoing a change another contributor made.
Andrea is actually correct about this, I had no idea the visual order does not match and that is an accessibility WCAG failure. However, as I noted above, another solution needs to be found.
Thanks all.
|
@alexstine thanks for your feedback
Actually, this PR moves the Tabs before the Close button. The order is now:
This matches the visual order. However, in a Tabs interface there shouldn't be any focusable element between tabs and tabpanels. So yes, this PR fixes the order mismatch but it introduces a new problem.
That's not correct because there shouldn't be any focusable element between tabs and tab panels. That defeats the whole purpose of the Tabs interface which is allowing keyboard users to directly tab from the selected Tab to the active Tab panel. I will create a separate issue. |
|
Any idea about how to solve the lates error? |
afercia
force-pushed
the
fix/list-view-header-visual-dom-order
branch
from
23975f4 to
64e2013
Compare
|
Thanks @Mamaduka 👋🏽 |
|
@afercia Yes, I got it backwards. Should be no close button in between tabs and tab panel. I wrote that too late I guess. Let's follow-up since this effects other areas of the editor. |
afercia
force-pushed
the
fix/list-view-header-visual-dom-order
branch
from
7ee7191 to
9dabda6
Compare
|
I think the broader issue of not allowing extraneous tab stops between the Tabs and their panels should be addressed in #59013 as it needs feedback from the coomponents package maintainers and from design. In the meantime, I think fixing the order mismatch is worthwhile. I'd appreciate a final review, thanks. |
|
I'm not going to block this, but wanted to voice my personal concern that I don't think this is an improvement, and if it were up to me I wouldn't be racing to get it in to 6.5. As noted in the supporting issue, I understand the motivation behind this PR, but I think the programmatically determined order actually makes more sense as things stand. Not only does the close button belong to the entire sidebar, rather than anything relating to the tabs, but now we're putting controls between those tabs and their associated content. This is one of those murky "lesser of two evils" scenarios, and I'm not convinced we've picked the right one here. |
|
I agree with you @andrewhayward. Our other option is to leave a known accessibility WCAG failure. I really hate both and I tend to agree that the current trunk is a better experience for keyboard users. Any further thoughts? Maybe we forget this for now and make the proper solution for future release? |
|
Given that RC1 is today, I'm removing this from 6.5. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
I'd agree on this and I'm not opposed to close this PR as long as:
|
I'm quite happy to support the conversation, and to push for improvements (there shouldn't be focus breaks between tabs and panels, for example), but lets be mindful of how we discuss the project and the work that goes into it. It may be a small thing, but the project should be able to evolve safely; the designs aren't "wrong", they just need work. Not to start an argument (!), but a lot of the accessibility specs are equally subjective!
I'm not sure as this will ever be possible, based purely on how the admin interface is built. There will almost certainly always be times where the tabs and their associated panels have to be inserted into different parts of the DOM tree. |
Fixes #58940
What?
In the List View header, the tabs and the close button visual order and DOM order mismatch. A new
orderCSS property was introduced to swap the order in place of the previous absolute positioning so, in a way, the problem is not new. However, it would be great to not introduce new `order' properties in the codebase.Why?
For accessibility, visual / reading order and DOM order must match when they affect 'meaning and operation'.
How?
Render the elements in the desired order in the DOM rather than swapping their visual order via CSS.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast