WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#28857 closed defect (bug) (fixed)

Media Grid: Manage focus when toggling between the grid and an edit attachment modal

Reported by: davidakennedy Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords:
Focuses: accessibility, javascript Cc:

Description (last modified by SergeyBiryukov)

What happened: When I visit /wp-admin/upload.php and navigate using a keyboard only and select any of the items in the Media Libray, focus is shifted to the modal (good!), but if I press esc or the close button to exit the modal, my focus is dumped at the beginning of #wpbody.

What I expected: I would expect the focus to return to the item I was last focused on, in this case, whichever media items I was on.

Steps to reproduce:

  1. Visit /wp-admin/
  2. Navigate to a media item via the keyboard by pressing tab.
  3. Activate the modal by pressing enter.
  4. Exit the modal by pressing esc or the close button.

Thoughts/Solutions: There are some great thoughts on this and a potential solution here: http://accessibility.oit.ncsu.edu/blog/2014/01/02/incredible-accessible-modal-window-version-2/

Attachments (4)

28857.diff (1.8 KB) - added by adamsilverstein 6 years ago.
restore focus when closing media grid modal
28857.2.diff (484 bytes) - added by adamsilverstein 6 years ago.
keep selected model in sync when navigating in modal
28857.3.diff (1013 bytes) - added by adamsilverstein 6 years ago.
find the correct li to focus on when closing modal
28857.4.diff (545 bytes) - added by adamsilverstein 6 years ago.
Remove unused currentTarget

Download all attachments as: .zip

Change History (19)

#1 @ocean90
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.0

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


6 years ago

#3 @ericlewis
6 years ago

  • Summary changed from Add Media: Manage focus better to Media Grid: Manage focus when toggling between the grid and an edit attachment modal

#4 @SergeyBiryukov
6 years ago

  • Description modified (diff)

@adamsilverstein
6 years ago

restore focus when closing media grid modal

#5 @adamsilverstein
6 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

28857.diff

  • When opening modal, store the event currentTarget on the model.
  • When closing modal, restore focus to closest li (currentTarget is sometimes li, sometimes enclosed image).
  • Need to address focus restoration after originally clicked item is deleted from modal.

Here is a screencast, all actions are keyboard driven.

Last edited 6 years ago by adamsilverstein (previous) (diff)

#6 @adamsilverstein
6 years ago

  • Focuses javascript added

#7 @wonderboymusic
6 years ago

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

Fixed in [29282]

@adamsilverstein
6 years ago

keep selected model in sync when navigating in modal

@adamsilverstein
6 years ago

find the correct li to focus on when closing modal

#8 @adamsilverstein
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I noticed an issue where if you open the image detail modal, navigate around with the arrows, then close the modal, the focus is not set.

28857.3.diff fixes that - it skips storing the target li at all; instead, when closing the modal it finds the correct li to focus on based on model data.

we still need to address focus when deleting an image from the modal (which right now seems to not close the modal?)

Last edited 6 years ago by adamsilverstein (previous) (diff)

#9 @wonderboymusic
6 years ago

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

In 29299:

Media Grid: when closing the modal, automatically focus the proper attachment by reading the model's ID, which is unique.

Props adamsilverstein (for the red), wonderboymusic (for the green).
Fixes #28857.

#10 follow-up: @ericlewis
6 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

We should think about the implementation here more thoroughly.

We should never use global jQuery selectors in Backbone code, this typically implies an object is taking on more responsibility than it can handle.

The task of reestablishing focus on the correct element should probably live within the media.view.MediaFrame.Manage controller. As this controller opens up the modal via the openEditAttachmentModal method, it should probably also listen to events happening on that frame to keep track of what attachment is currently being edited/viewed in the modal, and on close reestablish focus appropriately.

#11 in reply to: ↑ 10 @adamsilverstein
6 years ago

Good point. I put the focus call inside the close callback because we already had a close callback - I can see how shifting it to a listener in MediaFrame.Manage makes more sense structurally and would allow us to more narrowly focus the selector.

Replying to ericlewis:

We should think about the implementation here more thoroughly.

We should never use global jQuery selectors in Backbone code, this typically implies an object is taking on more responsibility than it can handle.

The task of reestablishing focus on the correct element should probably live within the media.view.MediaFrame.Manage controller. As this controller opens up the modal via the openEditAttachmentModal method, it should probably also listen to events happening on that frame to keep track of what attachment is currently being edited/viewed in the modal, and on close reestablish focus appropriately.

#12 @adamsilverstein
6 years ago

I worked on this a little bit and wound up with far more complicated code that seems harder to maintain; part of the issue is that the modal is repeatedly closed and reopened in resetContent inside MediaFrame.EditAttachments;

Any actions attached to the modal in MediaFrame.Manage are lost when the modal is destroyed/opened. Perhaps watching for the div itself would work, seems more complicated than the current solution.

@adamsilverstein
6 years ago

Remove unused currentTarget

#13 @adamsilverstein
6 years ago

28857.4.diff removes an unused variable that crept in from an earlier patch.

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.


6 years ago

#15 @ocean90
6 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

I agree with ericlewis, but this should go into another ticket. The main issue is fixed.

Note: See TracTickets for help on using tickets.