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:

  1. 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...
    1. The media item title
    2. The remove button (up, above the title)
    3. The weight field, if shown (down, below the title).
  2. 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.
  3. 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:

  1. The show/hide weights button
  2. The set of media items already selected
    1. The remove button
    2. The media item title
    3. The weight field, if shown
  3. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Focus 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.

andrewmacpherson’s picture

Priority: Normal » Major
andrewmacpherson’s picture

Title: Confusing visual tabbing order for "show media item weights" button in MediaLibraryWidget » Make the tabbing order match the visual reading order in MediaLibraryWidget
Issue summary: View changes
Status: Active » Needs work
Issue tags: +Needs issue summary update
FileSize
2.34 KB
1.19 KB

This 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.

seanB’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.42 KB
2.54 KB

Fixed 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!

Status: Needs review » Needs work

The last submitted patch, 5: 3024184-5.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review

Testbot issue has been resolved.

Kristen Pol’s picture

Thanks for the patch. I noticed one minor thing with a translation string (see below). I'll do some testing.

  1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -166,14 +166,13 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
                 '#attributes' => [
                   'class' => ['media-library-item__remove'],
    +              'aria-label' => $this->t('Remove') . ' ' . $media_item->label(),
                 ],
    

    Nitpick: Consider using placeholder, e.g.

    'aria-label' => $this->t('Remove @label'), ['@label' => $media_item->label()])

Kristen Pol’s picture

I tested the patch #5 in Chrome and found that the order was as follows:

  1. The show/hide weights button
  2. For the set of media items already selected:
    1. The remove ("X") button
    2. The media item title
    3. The weight field, if shown
  3. The "Browse media" button
  4. The "Add media" button

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.

phenaproxima’s picture

Thanks, @Kristen Pol!! That is super useful. I wonder, though -- to prevent regressions, is there any way we can add automated tests of this?

phenaproxima’s picture

Status: Needs review » Needs work

Setting back to "needs work" for #8.

Kristen Pol’s picture

Oh! Thanks to @phenaproxima for catching that I forgot to mark back to "Needs work"... burning midnight oil on a weekend has its drawbacks!

seanB’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
5.97 KB
2.41 KB

@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.

The last submitted patch, 13: 3024184-13-fail.patch, failed testing. View results

andrewmacpherson’s picture

Thanks 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:

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.

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.

andrewmacpherson’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Updated 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.

seanB’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
1.44 KB

Attached patch removes the assertions for the DOM order.

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

Perfect. #17 is good, addresses #16.
The test for the aria-label is still there, which is good.
I just repeated a manual test too.

  • webchick committed 9e459d9 on 8.6.x
    Issue #3024184 by seanB, andrewmacpherson, Kristen Pol: Make the tabbing...
webchick’s picture

Wow, 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!

  • webchick committed dd324bd on 8.7.x
    Issue #3024184 by seanB, andrewmacpherson, Kristen Pol: Make the tabbing...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.