WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 2 months ago

#43169 reopened task (blessed)

Media views: avoid to move focus programmatically when not strictly necessary

Reported by: afercia Owned by: afercia
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots has-patch
Focuses: ui, accessibility, javascript Cc:

Description

Ideally, focus should be managed programmatically only to "repair" the native tab order and avoid focus losses. For example, when opening a modal dialog focus needs to be moved to the modal and when closing it, focus must be moved back to the control that opened the modal.

In the media views, in many cases, moving focus is designed for a mouse-based interaction flow and, generally, it appears it was designed with a specific workflow in mind, that is: the workflow the developers felt was "right". In other words: based on assumptions.

While I understand the intent was good, this is very problematic for keyboard users. Let's make a couple examples:

https://cldup.com/SpSTMrgvdn.png

When adding an attachment to a post, for example selecting an image in the media grid, focus is moved to the URL field in the details sidebar. This is not an issue for mouse users: they can click another image at any time. Now, say a keyboard users is trying to add a gallery and needs to select multiple images: every time the user selects an image pressing the Spacebar, focus goes to the sidebar. Now the user is forced to tab many times, potentially hundreds of times depending on the number of images in the grid, to go to the grid again and select another image. Not to mention screen reader users will hardly understand why the context is changed and what the next step is.

In this case, focus should just stay in place on the selected image. It is far better to avoid an abrupt change of context rather than assuming that moving focus to the URL field is of any help for users.

One more example:

https://cldup.com/xmCQcKmGST.png

Over time, the media modal has been adapted and used also for other core features. In the screenshot above, that is what happens when editing an image attached to an Image widget in the widgets screen. Focus is moved to the URL field, if the image is linked (when it's not linked, there's no URL field displayed). Notice this happens only the first time the modal opens, not sure why.

Regardless, for keyboard and screen reader users big part of the content of the modal gets "skipped" and they land in the middle of some content without any context or meaningful information about what just happened.

I'd like to propose to remove any focus management from the media views, except for the cases where keeping focus within the modal is necessary. It is far better to keep the native tab order: users will find their way, once they learn how the interface is structured. Worth reminding assistive technologies users already have tools to jump to different part of the content, if the content is semantic and well structured.

I'd also like to quote what the React team states in their documentation dedicated to accessibility, they say it very well, far better than I could ever say:

https://reactjs.org/docs/accessibility.html#programmatically-managing-focus

Use it (Ed. focus management) to repair the keyboard focus flow when it is disturbed, not to try and anticipate how users want to use applications.

Attachments (2)

43169.diff (27.7 KB) - added by afercia 19 months ago.
43169.2.diff (19.4 KB) - added by afercia 2 months ago.

Download all attachments as: .zip

Change History (22)

#1 @afercia
19 months ago

Partially related: #41612

@afercia
19 months ago

#2 @afercia
19 months ago

  • Keywords has-patch dev-feedback added

Typically, the cases where managing focus programmatically is necessary are:

  • when opening and closing the media modal
  • then the media modal view changes and its whole contant re-renders
  • when adding or deleting attachments

43169.diff is a first pass

  • keeps focus management in the cases above and in a few other cases that make sense
  • removes any other usage of focus()
  • makes the edit-attachment navigation buttons really disabled when there are no next or previous attachments
  • tries to clarify with inline comments all the usages of focus()

There are a couple issues to address yet, most notably when MEDIA_TRASH is set to true, when deleting attachments from the Media Library, the view is re-queried and reset. This makes difficult to restore focus in a proper place and would probably need an event that fires at the end of the view re-rendering or something similar. Any help very welcome.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


19 months ago

#4 @afercia
19 months ago

  • Milestone changed from Awaiting Review to 5.0

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


18 months ago

#6 @joemcgill
18 months ago

  • Owner set to joemcgill
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


11 months ago

#8 @afercia
11 months ago

  • Milestone changed from 5.0 to 4.9.9

This ticket could be a good candidate for the 4.9.9 focus on quick a11y fixes.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


11 months ago

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


11 months ago

#11 @afercia
11 months ago

@joemcgill ping :)

#12 @pento
11 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 months ago

#14 @afercia
3 months ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from Future Release to 5.3
  • Owner changed from joemcgill to afercia

As in this release cycle there's some focus on improvements for the media views (thanks also to the WPCampus accessibility report) and this ticket has been waiting for a while, I'd like to propose it for 5.3 consideration.

@afercia
2 months ago

#15 @afercia
2 months ago

  • Keywords commit added; needs-refresh removed

43169.2.diff refreshes the patch. It also improves a few things compared to the previous one. Looks like in a few cases focus management in the media modal didn't work correctly since the beginning. For example, focus needs to be set again on the modal when switching between some "frames":

Gallery:

  • canceling a Gallery
  • jumping to Edit Gallery view
  • jumping back from Add to Gallery to Edit Gallery view

Audio Playlist:

  • canceling an Audio Playlist
  • jumping to Edit Audio Playlist view
  • jumping back from Add to Audio Playlist to Edit Audio Playlist view

Video Playlist:

  • canceling a Video Playlist
  • jumping to Edit Video Playlist view
  • jumping back from Add to Video Playlist to Edit Video Playlist view

Among those, only a few were implemented correctly while others completely missed focus management.

To recap, this patch:

  • keeps focus management only where necessary
  • removes focus management where a specific user workflow was assumed
  • makes the edit-attachment navigation buttons really disabled when there are no next or previous attachments
  • clarifies with inline comments all the usages of focus()

I'd like to commit this patch soon, as having it in core at the beginning of this release cycle is the best way to test it extensively with a larger testers base.

#16 @afercia
2 months ago

Also, the patch slightly changes wp.media.view.FocusManager as first step for future improvements.

#17 @afercia
2 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45524:

Accessibility: Improve focus management in the Media Views.

  • keeps focus management only where necessary to avoid focus losses
  • removes focus management where a specific user workflow was assumed
  • makes the "Attachment Details" navigation buttons really disabled when there are no next or previous attachments
  • adds inline comments to clarify all the usages of focus()

Fixes #43169.

#18 @afercia
2 months ago

  • Keywords commit removed

#19 @afercia
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from defect (bug) to task (blessed)

Spotted one more case of focus loss when switching between "frames":

  • make a multiple selection
  • navigate to and press "Edit Selection" in the bottom toolbar
  • in the "Add Media" frame, press "Return to library"
  • focus is not moved back to the modal

As I expect to find more edge cases that need to be addressed, I'd like to propose to make this ticket a "task" for the 5.3 cycle so that additional commits can reference this ticket.

#20 @afercia
2 months ago

In 45531:

Accessibility: Handle one more case of focus loss when switching view in the Media Views.

See #43169.

Note: See TracTickets for help on using tickets.