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

[davidakennedy]

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/

[adamsilverstein]

restore focus when closing media grid modal

[adamsilverstein]

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.

[wonderboymusic]

Fixed in [29282]

[adamsilverstein]

keep selected model in sync when navigating in modal

[adamsilverstein]

find the correct li to focus on when closing modal

[adamsilverstein]

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?)

[wonderboymusic]

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.

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

[adamsilverstein]

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.

[adamsilverstein]

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]

Remove unused currentTarget

[adamsilverstein]

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

[ocean90]

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