External Media: Fix Modal focus loss on arrow keypress #16055

WunderBart · 2020-06-03T19:34:30Z

Fixes: #15855

Changes proposed in this Pull Request:

Stops keydown propagation for arrows to prevent unwanted modal closing.

Testing instructions:

Check out this PR and run yarn run build-extensions
In the editor, insert the Image block
Click Select Media button and select any one of the external media options (Google / Pexels)
When modal opens, press any arrow key any number of times
The modal shouldn't close

Proposed changelog entry for your changes:

no changelog entry needed

matticbot · 2020-06-03T19:34:36Z

Caution: This PR has changes that must be merged to WordPress.com
Hello WunderBart! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D44372-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

jetpackbot · 2020-06-03T19:37:00Z

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes
⚠️	The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16055

Generated by 🚫 dangerJS against d5cc32d

extensions/shared/external-media/external-media-modal.js

obenland · 2020-06-04T20:44:44Z

I took this for a test, the things that came to mind for me:

Would you mind changing the base branch to feature/external-media and rebase it? There are changes to useMedia in there.
After applying the patch I could get the modal to show up when replacing an image in a Media & text block.
Is there no way we can keep ExternalMediaModal as a HOC?
Could handleArrowKeysPropagation() be extended to prevent the upload modal to come up when adding images to a gallery block?

extensions/shared/external-media/external-media-modal.js

WunderBart · 2020-06-05T12:31:04Z

I've updated (and rebased) the code to include only the arrow keys handler. All the other changes were creating too many issues and weren't necessary ATM. This should be now easier to review and test.

jeryj · 2020-06-05T19:02:22Z

@WunderBart and I went over this a lot today and isolated that this is not an issue specific to this modal. This happens to any Gutenberg <Modal> component used within the editor: WordPress/gutenberg#22940

WunderBart · 2020-06-08T10:14:15Z

@WunderBart and I went over this a lot today and isolated that this is not an issue specific to this modal. This happens to any Gutenberg <Modal> component used within the editor: WordPress/gutenberg#22940

Nice find! We can probably still continue with this solution until a proper Gutenberg fix is landed, though. What do you think?

extensions/shared/external-media/sources/with-media.js

jeryj

I should have added my comments as a review. Sorry 🤦‍♂️. I think the functionality is good and the code is very close. Thanks for taking so much time to isolate and debug this issue!

WunderBart · 2020-06-08T17:20:25Z

@jeryj, this is ready for another go! 🤞 I was able to make it a lot simpler after your comments. Thanks!

FYI so you don't miss it - I removed the onMouseDown event stopper as it's already implemented in the current Modal implementation!

jeryj

This is SO much simpler! I loaded the changed files several time because I thought not all the changes were loading 😂

But, I did find one issue 😬 If you move focus to outside the modal content and press the arrow key, it will still close the modal:

Open the modal
Press shift+Tab to focus the X button
Press an arrow key

WunderBart · 2020-06-09T09:39:28Z

If you move focus to outside the modal content and press the arrow key, it will still close the modal:

Ugh, how did I miss that! I think we need to re-attach the handler to the modal listener element then. Back to the more complicated version! 😬

It is already being stopped by the IsolatedEventContainer that is wrapping the frame of the modal.

extensions/shared/external-media/sources/with-media.js

addressed by using ref, just slightly differently than expected!

extensions/shared/external-media/sources/with-media.js

jeryj

Thanks for y'alls diligence on this @WunderBart and @marekhrabe!

Co-authored-by: Marek Hrabe <marekhrabe@me.com>

@frontdevde

* External Media: Render Media after authentication (#16044) See #15717 (review) * [not verified] Handle 401 responses (#16041) See #15717 (review) * External Media: Better reflect accepted media types (#16071) * External Media: Account for multiple images (#16077) See #16071 (comment) * External Media: Fix Modal focus loss on arrow keypress (#16055) Co-authored-by: Marek Hrabe <marekhrabe@me.com> * External Media: Add API unit tests (#15988) * [not verified] Add External media endpoint * [not verified] Add unit tests * [not verified] Add class coverage info * Simplify schema * Fix image copy test * Use local image * Remove duplicate file ending Props @frontdevde. * External Media: Improve Insert Flow (#16081) * [not verified] # This is a combination of 5 commits. External Media: Improve Insert Flow See #15867 Remove unused import Lock pointer events when copying This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending. Announce Inserting action along with alt text of selected images. Update modal headers to reflect status Props @jancavan. * External Media: Fix mobile modal size (#16098) * External Media: Improve Insert Flow See #15867 * Remove unused import * Lock pointer events when copying This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending. * Announce Inserting action along with alt text of selected images. * Fix mobile mobile width to take up the full available modal space * Change is-copying to modifier instead of element to follow BEM syntax Co-authored-by: Konstantin Obenland <obenland@gmx.de> Co-authored-by: Bart <bartlomiej.kalisz@gmail.com> * External Media: Loading and disabled states for the copying state (#16103) * disable pagination while loading * fade out all images when copying * disable event handlers and mark as disabled when copying * disable pexels search when copying * disabled google filters when copying * add text to copying indicator * disable google photos view selector when copying * [not verified] remove debugging condition * Pass functions not booleans to event emitters Co-authored-by: Konstantin Obenland <obenland@gmx.de> * Removed :not() copying css state since they are the same modal now * Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal. * Cosmetically rearranging code order * External Media: Screen Reader accessible states for Insert flow (#16113) * Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state * [not verified] Remove title attribute from grid images * [not verified] Move Inserting Image... copy to before the image so it is announced first. * Separate-out description and element id Helps with keeping describedby id in sync Co-authored-by: Konstantin Obenland <obenland@gmx.de> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Bart <bartlomiej.kalisz@gmail.com> Co-authored-by: Marek Hrabe <marekhrabe@me.com> * External Media: Restore focus on modal close (#16102) * External Media: Disable for existing galleries (#16176) * External Media: Disable for existing galleries * Update comment * External Media: Improve upload permission check (#16185) * Use `value` to disable for existing galleries `addToGallery` is not used by all gallery-type blocks, like tiled-gallery or slideshow. Props @jeherve. Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com> Co-authored-by: Marek Hrabe <marekhrabe@me.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

matticbot added the Touches WP.com Files label Jun 3, 2020

WunderBart self-assigned this Jun 3, 2020

WunderBart added the [Status] In Progress label Jun 3, 2020

WunderBart linked an issue Jun 3, 2020 that may be closed by this pull request

(3P) External Media: Arrow keys not captured by modal #15855

Closed

WunderBart commented Jun 3, 2020

View reviewed changes

extensions/shared/external-media/external-media-modal.js Outdated Show resolved Hide resolved

WunderBart marked this pull request as ready for review June 4, 2020 16:33

WunderBart added the [Extension] External Media Extend all block editor media tools to support external providers label Jun 4, 2020

WunderBart requested a review from jeryj June 4, 2020 16:33

WunderBart added [Status] Needs Team Review and removed [Status] In Progress labels Jun 4, 2020

WunderBart force-pushed the fix/external-media-modal-toggle branch from 49fd82b to d9ec352 Compare June 4, 2020 17:04

jeryj reviewed Jun 4, 2020

View reviewed changes

extensions/shared/external-media/external-media-modal.js Outdated Show resolved Hide resolved

jeryj reviewed Jun 4, 2020

View reviewed changes

extensions/shared/external-media/external-media-modal.js Outdated Show resolved Hide resolved

WunderBart force-pushed the fix/external-media-modal-toggle branch from d9ec352 to 2d0389a Compare June 5, 2020 12:22

WunderBart changed the base branch from master to feature/external-media June 5, 2020 12:23

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jun 5, 2020

WunderBart force-pushed the fix/external-media-modal-toggle branch 2 times, most recently from 0199678 to bf3eb0f Compare June 5, 2020 12:30

WunderBart force-pushed the fix/external-media-modal-toggle branch from bf3eb0f to e075167 Compare June 5, 2020 12:54

WunderBart requested review from jeryj and obenland June 5, 2020 12:59

jeryj reviewed Jun 8, 2020

View reviewed changes

extensions/shared/external-media/sources/with-media.js Outdated Show resolved Hide resolved

jeryj reviewed Jun 8, 2020

View reviewed changes

extensions/shared/external-media/sources/with-media.js Outdated Show resolved Hide resolved

jeryj reviewed Jun 8, 2020

View reviewed changes

extensions/shared/external-media/sources/with-media.js Outdated Show resolved Hide resolved

jeryj requested changes Jun 8, 2020

View reviewed changes

WunderBart requested a review from jeryj June 8, 2020 17:22

jeryj requested changes Jun 8, 2020

View reviewed changes

WunderBart added 4 commits June 9, 2020 12:14

Contain arrow keys within the External Media Modal

598f54f

Handle arrow propagation directly via content element

ab9d7af

Link to issue for tracking purposes

7ff7184

Remove onMouseDown propagation stopper

41de2b9

It is already being stopped by the IsolatedEventContainer that is wrapping the frame of the modal.

WunderBart force-pushed the fix/external-media-modal-toggle branch from ae77fc4 to 41de2b9 Compare June 9, 2020 10:14

Re-attach keys handler to Modal parent

c6c9171

WunderBart requested a review from jeryj June 9, 2020 10:42

marekhrabe previously requested changes Jun 9, 2020

View reviewed changes

extensions/shared/external-media/sources/with-media.js Outdated Show resolved Hide resolved

replace global querySelector with scoped one

d5cc32d

jeryj reviewed Jun 9, 2020

View reviewed changes

extensions/shared/external-media/sources/with-media.js Show resolved Hide resolved

jeryj approved these changes Jun 9, 2020

View reviewed changes

WunderBart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Team Review labels Jun 9, 2020

WunderBart merged commit 7ac15b5 into feature/external-media Jun 9, 2020

WunderBart deleted the fix/external-media-modal-toggle branch June 9, 2020 14:10

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 9, 2020

WunderBart mentioned this pull request Jun 9, 2020

(3P) External Media: Arrow keys not captured by modal #15855

Closed

obenland mentioned this pull request Jun 9, 2020

External Media: Updates for 8.7 #16069

Merged

obenland pushed a commit that referenced this pull request Jun 24, 2020

External Media: Fix Modal focus loss on arrow keypress (#16055)

81f731c

Co-authored-by: Marek Hrabe <marekhrabe@me.com>

jeherve removed the [Status] Needs Changelog label Jun 29, 2020

jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Media: Fix Modal focus loss on arrow keypress #16055

External Media: Fix Modal focus loss on arrow keypress #16055

WunderBart commented Jun 3, 2020 •

edited

Loading

matticbot commented Jun 3, 2020

jetpackbot commented Jun 3, 2020 •

edited

Loading

obenland commented Jun 4, 2020

WunderBart commented Jun 5, 2020

jeryj commented Jun 5, 2020

WunderBart commented Jun 8, 2020

jeryj left a comment

WunderBart commented Jun 8, 2020 •

edited

Loading

jeryj left a comment

WunderBart commented Jun 9, 2020

jeryj left a comment

External Media: Fix Modal focus loss on arrow keypress #16055

External Media: Fix Modal focus loss on arrow keypress #16055

Conversation

WunderBart commented Jun 3, 2020 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jun 3, 2020

jetpackbot commented Jun 3, 2020 • edited Loading

obenland commented Jun 4, 2020

WunderBart commented Jun 5, 2020

jeryj commented Jun 5, 2020

WunderBart commented Jun 8, 2020

jeryj left a comment

Choose a reason for hiding this comment

WunderBart commented Jun 8, 2020 • edited Loading

jeryj left a comment

Choose a reason for hiding this comment

WunderBart commented Jun 9, 2020

jeryj left a comment

Choose a reason for hiding this comment

WunderBart commented Jun 3, 2020 •

edited

Loading

jetpackbot commented Jun 3, 2020 •

edited

Loading

WunderBart commented Jun 8, 2020 •

edited

Loading