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/issue 5693 firefox focus trap #11601

Merged
merged 12 commits into from Nov 12, 2018

Conversation

Projects
None yet
2 participants
@timwright12
Contributor

timwright12 commented Nov 7, 2018

Description

Fixes #5693

How has this been tested?

Browser this fix in tested in Chrome, Safari, Firefox, Windows Firefox

Types of changes

Interrupted the key event on inputURL if the caret position isn't at the beginning or end of the input field text to mimic the expected behavior in non-Win FF browsers

Checklist:

  • [ x ] My code is tested.
  • [ x ] My code follows the WordPress code style.
  • [ x ] My code follows the accessibility standards.
  • [ x ] My code has proper inline documentation.
@timwright12

This comment has been minimized.

Contributor

timwright12 commented Nov 7, 2018

@tofumatt Please let me know if anything is "off" with the PR as it's pretty much my first one, happy to update or run anything locally that I forgot to to get tests passing or whatever!

@tofumatt

Thanks for the PR @timwright12! A few meta points around the PR:

  • no need to reference this PR in the original issue; GitHub does that for us so it's just extra notifications/noise that isn't needed 😄
  • You can use "Fixes #xyz" in the issue description so the issue you're working on fixing will be closed automatically once the patch lands
  • You should remove the addition of the gutenberg-mobile submodule; that was removed awhile back and shouldn't be committed 😄

Otherwise the process around everything looks good 👍

This looks good but I think could use comments that clarify things a bit more; right now this code would be a bit confusing to a developer unaware of this bug or the reason for this code to exist.

Normally I'd also advocate for an E2E (end-to-end) test here, but our E2E tests run in Chromium on Linux so 🤷‍♂️ Since the code path is still the same it might be nice to test it and make sure that keyboard presses work as intended, but we won't really be testing the browser/OS combo that caused the bug in the first place…

Thanks for the patch, let me know if anything needs clarification, but I think after some comment tweaks this'll be good to merge! 👍

@@ -137,6 +137,32 @@ class URLInput extends Component {
// If the suggestions are not shown or loading, we shouldn't handle the arrow keys
// We shouldn't preventDefault to allow block arrow keys navigation
if ( ! showSuggestions || ! posts.length || loading ) {
// This switch statement corrects a bug in Windows Firefox

This comment has been minimized.

@tofumatt

tofumatt Nov 8, 2018

Member

Is there anymore context for this bug? What causes it? Is there a link explaining what's happening here or what the bug is?

It would be helpful for future developers to get a bit more info here 😄

The info you wrote up in the original issue (
#5693 (comment)) was handy, so even including that or parts of it (and then linking to the comment for more context) would be great!

event.stopPropagation();
event.preventDefault();
// Set the inpit caret to the last position

This comment has been minimized.

@tofumatt

tofumatt Nov 8, 2018

Member

Typo: should be "input"

Show resolved Hide resolved packages/editor/src/components/url-input/index.js
// This switch statement corrects a bug in Windows Firefox
switch ( event.keyCode ) {
case UP: {
// If the caret is not in position 0

This comment has been minimized.

@tofumatt

tofumatt Nov 8, 2018

Member

This comment is pretty much describing the if condition below, which is pretty easy to read. But the point here is the caret should be moved to the beginning if it's not there when UP is pressed; it might be better to just explain that here or above the CASE UP: line. 🤷‍♂️

Tim Wright and others added some commits Nov 8, 2018

Tim Wright

@tofumatt tofumatt self-requested a review Nov 8, 2018

@tofumatt tofumatt added this to the 4.4 milestone Nov 12, 2018

@tofumatt

Awesome work, thanks for the clarification in the comment. Really sorry about the delay in getting to this review.

I've tweaked the comments just a little bit, all good to ship. Thanks again! 👍

@tofumatt tofumatt merged commit 5b8e64d into WordPress:master Nov 12, 2018

1 check passed

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

@timwright12 timwright12 deleted the timwright12:fix/issue-5693-firefox-focus-trap branch Nov 13, 2018

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

Fix/issue 5693 firefox focus trap (WordPress#11601)
* merge from upstream

* linting

* removed submodule

* updated commenting in the URL input component

* Delete merge from upstream

* chore: Tweak comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment