Problem/Motivation
The MediaLibraryWidget has a "show/hide media item weights" button at the start of the fieldset. This is a special case of a multi-value field widget, which normally use tabledrag.
The numeric weights method was retained as an accessibility fallback for users who don't use a pointer device - primarily screen reader users, and speech control users. And currently in D8, sighted keyboard users need the numeric weights too, due to #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys.
With tabledrag the show/hide weights button comes before the draggable rows, both visually and in the keyboard tabbing order. A keyboard user who wants to reorder items can swap to the weights method as soon as they arrive at the table.
In the MediaLibrary widget, we have a different draggable items implementation, but it also offers a numeric weights method. However the tabbing order in this fieldset is visually confusing:
- The set of media items. Visually, focus appears to have bypassed the show weights button. Then it follows a zig-zag path through the media items...
- The media item title
- The remove button (up, above the title)
- The weight field, if shown (down, below the title).
- The show/hide weights button. Visually, this is a jump UP to the top of the fieldset, before the media items. This is an unexpected direction.
- The add media button. Here the focus jumps back down below the media items.
This is a WCAG level-A issue, see Focus Order: Understanding SC 2.4.3. The visual tabbing order should match the visual reading order.
As well as being visually confusing for sighted keyboard users, it also creates extra effort for all keyboard users. In order to use the numeric weights, the user first has to tab past all of the current media items, to reach the show-weights button, then tab backwards to the items they want to move, then forward again to the form submit button. Compared to tabledrag, this can easily involve twice the effort (in keypresses) and probably extra cognitive load too.
While working on this issue, seanb discovered the "remove" buttons all had identical accessible names. They have different outcomes though; so they need to say which item they remove. See comment #5.
Proposed resolution
Make the tabbing order match the visual reading order:
- Move the show/hide weights button, so it comes before the
.media-library-selection
container element in the DOM order. We can reduce the confusion and keypress effort, while making it more consistent with the existing tabledrag. - In each media item card, swap the DOM order of the rendered entity and the remove button, so the button is first in the tabbing order, and the visual focus order matches the reading order inside each card.
- Disambiguate the "remove" buttons, so they are clear for screen reader users about which media item they remove.
The desired tabbing order, upon reaching the MediaLibraryWidget fieldset, is:
- The show/hide weights button
- The set of media items already selected
- The remove button
- The media item title
- The weight field, if shown
- The add media button
Remaining tasks
- DONE. Update the form order in MediaLibraryWidget::formElement().
- Done. Added aria label to the remove button.
- Done. CSS fixes so the remove buttons don't jump on hover/focus.
User interface changes
- Fixes the tabbing position of the show/hide weights button, so it matches the visual reading order, making it more predictable for sighted keyboard users.
- Greatly reduces the effort required by a keyboard user, when they actually make use of the numeric weights to re-order media items.
- Disambiguates the accessible name of the "remove" buttons. This is invisible, screen reader users benefit from it.
- No visual design changes.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff-13-17.txt | 1.44 KB | seanB |
#17 | 3024184-17.patch | 5.3 KB | seanB |
#13 | interdiff-5-13.txt | 2.41 KB | seanB |
#13 | 3024184-13.patch | 5.97 KB | seanB |
#13 | 3024184-13-fail.patch | 1.38 KB | seanB |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedFocus order is at WCAG level-A, which I usually file under major.
This simple patch just moves the toggle-weights button in the Form order. This looks like a quick win! The tabbing order for this button is now correct, and the visual design is unchanged.
Note: the MediaLibraryWidget has some other confusing tabbing/visual order mismatches inside each media item "card". Tabbing through the title, remove button, and numeric weight, follows an up/down zig-zag path. This should also be fixed.
Comment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #4
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis patch swaps the order of the rendered media entity and the remove button in the form structure.
It makes the tabbing order follow reading order, inside a media item card.
But it introduces a visual regression: the remove button now jumps a little on hover or focus.
Changing title, we can fix all of the focus order problems in the same issue I think.
TODO: CSS to fix the jumping remove button.
Comment #5
seanBFixed the CSS. Apparently the
.button:first-child
selector was more specific than what we were using.Also added an aria label to the remove button. Since the button is now before the filename, it is a bit unclear what that button actually removes.
Please review!
Comment #7
seanBTestbot issue has been resolved.
Comment #8
Kristen PolThanks for the patch. I noticed one minor thing with a translation string (see below). I'll do some testing.
Nitpick: Consider using placeholder, e.g.
'aria-label' => $this->t('Remove @label'), ['@label' => $media_item->label()])
Comment #9
Kristen PolI tested the patch #5 in Chrome and found that the order was as follows:
The only difference between this and what is expected in the issue summary is the "Browse media" button. But, the tab order is the same as the visual order and is as expected.
Marking back to "Needs work" to consider the previous comment #8. Otherwise, this looks good.
Comment #10
phenaproximaThanks, @Kristen Pol!! That is super useful. I wonder, though -- to prevent regressions, is there any way we can add automated tests of this?
Comment #11
phenaproximaSetting back to "needs work" for #8.
Comment #12
Kristen PolOh! Thanks to @phenaproxima for catching that I forgot to mark back to "Needs work"... burning midnight oil on a weekend has its drawbacks!
Comment #13
seanB@Kristen Pol, thank you for the reviews! Regarding the 'Browse media' button in #9, that should have been removed in #3020707: Streamline buttons in the media library widget. Could you maybe double check to see if you have all the latest changes from 8.7.x?
Added a fix for #8. Also added a test for the new aria-label.
Besides that added some tests to validate the order of the widget elements, incl a fail patch to prove it works.
Comment #15
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks for working on this. I tried ut the patch and it works well on Firefox 64.
The browse media button was indeed removed, the tabbing order does match the desired order in the issue summary.
#5:
Good catch - we would need to disambiguate those buttons in any case, regardless of what DOM order they are in. That's because screen reader users don't have to "tab" through ALL interactive controls; screen readers also provide other commands to "move to previous/next button", and some also provide a palette/dialog window to "list all buttons". Both of these tools effectively bypass the media entity label, so it's good to disambiguate the buttons this way.
#10 + 13: regarding automated tests...
I don't think this is at all easy to do, and AFAIK we aren't testing tabbing order anywhere else in core. I don't think we should make automated tests a blocker for fixing this bug. Automated testing of the visual tabbing order is still in the blue-sky-thinking realm at present (I think about it a lot).
I can see what the tests in #13 are doing, with the "+" selector, which asserts that the elements appear in the expected DOM order.
However it doesn't actually test the tabbing order. The tabbing order can be differ from the DOM order for a variety of reasons (e.g. use of tabindex, aria-flowto, programmatic focus changes by JS) yet still satisfy WCAG. All this test can do is assert the DOM order, not the keyboard traversal of it. I don't think it's worth asserting the DOM order on it's own, that's not what this bug is about.
With Nightwatch, we might be able to write tests which perform tab keypresses, then make assertions about which element now has focus. It might also be possible with JavascriptTestBase now that it uses WebDriver - but previously keypresses were buggy with the old Goutte driver.
But that still isn't testing the visual path of the tabbing order, which is what this bug issue was actually about. This can be altered completely with bad use of CSS, and we'd never catch that unless we were able to assert the position of the element which currently has focus, and compare it with the position of the elements which previously had focus to assert that it wasn't jumping around in a haphazard zig-zag way. The multi-column arrangement of the media item cards makes this even more awkward, because a natural reading order does involve going back up when going to the next card.
Comment #16
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedUpdated the IS, the before-and-after tabbing order differed over the "browse media" button, which was removed by #3020707: Streamline buttons in the media library widget. Also explained the button label issue.
I think the test assertions about DOM order should be removed - they aren't much use we get a way to test the visual path.
Comment #17
seanBAttached patch removes the assertions for the DOM order.
Comment #18
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedPerfect. #17 is good, addresses #16.
The test for the aria-label is still there, which is good.
I just repeated a manual test too.
Comment #20
webchickWow, great work here, folks!
Committed and pushed to 8.7.x and cherry-picked to 8.6.x, since it's a bug fix in an experimental module. Thanks!
Comment #22
webchick