Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#43169 closed task (blessed) (fixed)

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

Reported by: afercia's profile afercia Owned by: afercia's profile 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 7 years ago.
43169.2.diff (19.4 KB) - added by afercia 6 years ago.

Download all attachments as: .zip

Change History (28)

#1 @afercia
7 years ago

Partially related: #41612

@afercia
7 years ago

#2 @afercia
7 years 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.


7 years ago

#4 @afercia
7 years ago

  • Milestone changed from Awaiting Review to 5.0

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


7 years ago

#6 @joemcgill
7 years ago

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

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


6 years ago

#8 @afercia
6 years 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.


6 years ago

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


6 years ago

#11 @afercia
6 years ago

@joemcgill ping :)

#12 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

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


6 years ago

#14 @afercia
6 years 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
6 years ago

#15 @afercia
6 years 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
6 years ago

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

#17 @afercia
6 years 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
6 years ago

  • Keywords commit removed

#19 @afercia
6 years 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
6 years ago

In 45531:

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

See #43169.

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


5 years ago

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


5 years ago

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


5 years ago

#24 @afercia
5 years ago

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

This ticket is still open only to catch potential focus losses edge cases or JS errors after the committed changes. In the last 3 months and more nothing relevant was noticed and reported. Today's WordPress 5.3. Beta 3 day so I'm going to close this task-ticket to clean up the Trac reports. Can always be reopened if need be.

#25 @afercia
5 years ago

In 46745:

Widgets: Avoid to move focus to the Image Widget "Insert from URL" field.

Fixes a bug after [45499], where the backbone view element isn't the input field any longer. Also, managing focus programmatically is often an assumption on a specific user flow and should generally be avoided, see #43169.

Props dufresnesteven.
See #43169.
Fixes #48588.

#26 @SergeyBiryukov
5 years ago

In 46781:

Widgets: Avoid to move focus to the Image Widget "Insert from URL" field.

Fixes a bug after [45499], where the backbone view element isn't the input field any longer. Also, managing focus programmatically is often an assumption on a specific user flow and should generally be avoided, see #43169.

Props dufresnesteven, afercia.
Merges [46745] to the 5.3 branch.
See #43169.
Fixes #48588.

Note: See TracTickets for help on using tickets.