New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix escape key failing to close inline link popover #11806

Merged
merged 2 commits into from Nov 16, 2018

Conversation

@talldan
Contributor

talldan commented Nov 13, 2018

Description

Ensure the escape key can be used to close the Inline Link UI regardless of where focus is within the popover.

The bug
InlineLinkUI previously rolled it's own handler for closing the popover when the escape key was pressed:

onKeyDown( event ) {
if ( event.keyCode === ESCAPE ) {
event.stopPropagation();
this.resetState();
}

Unfortunately this is only bound to a subset of the popover's UI. If the focus was on the settings part of the popover, the escape key would not trigger closure of the popover.

Fix
Bind an onClose handler for the popover (which handles the escape key implicitly) instead of handling it in an ad-hoc fashion.

How has this been tested?

  1. Add a paragraph and add some text
  2. Highlight a word and hit command+k (or ctrl+k on linux/windows) to turn it into a link
  3. Tab to the link settings icon (three dots)
  4. Press escape
  5. Observe that the popover closes

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan self-assigned this Nov 13, 2018

@talldan talldan requested a review from afercia Nov 13, 2018

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 13, 2018

Made this WIP, as I plan to add an e2e test.

@talldan talldan force-pushed the fix/url-popover-escape-key branch from b009c2b to b3d8d23 Nov 14, 2018

@talldan talldan requested a review from noisysocks Nov 14, 2018

@talldan talldan changed the title from WIP: Fix escape key failing to close inline link popover to Fix escape key failing to close inline link popover Nov 15, 2018

@jorgefilipecosta

It works correctly on my tests 👍
There is only one small detail that can be improved in the future, but it is not a regression and not a blocker of this PR. If we are in the middle of a paragraph, we press meta +k and escape the cursor goes back to the start of a paragraph instead of staying in the middle.

@jorgefilipecosta jorgefilipecosta added this to the 4.5 milestone Nov 15, 2018

@noisysocks noisysocks self-assigned this Nov 15, 2018

@noisysocks noisysocks merged commit f3b4794 into master Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the fix/url-popover-escape-key branch Nov 16, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix escape key failing to close inline link popover (WordPress#11806)
* Bind an onClose handler to handle use of escape key in InlineLinkUI

* Add test case for escape key usage when the settings icon is focused
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment