Skip to content
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

Check to ensure focus has intentionally left the wrapped component in `withFocusOutside` HOC #17051

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@getdave
Copy link
Contributor

commented Aug 15, 2019

withFocusOutside HOC aims to detect when a focus occurs outside of the wrapped compomnent.

Previously there was no check to see whether the blur event occurred within the wrapped component.

If it occurs within then we do not want to trigger the handleFocusOutside callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced by removal of react-click-outside in #16878

How has this been tested?

Still working on this.

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.
  • I've included developer documentation if appropriate.
… focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Looks like that there's a lot of failing tests.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Yeh I should look at those tests now.

getdave added 2 commits Aug 19, 2019
Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.
@getdave getdave changed the title Check event origin in withFocusOutside HOC to ensure focus has left wrapper component Check to ensure focus has intentionally left the wrapped component Aug 19, 2019
@getdave getdave changed the title Check to ensure focus has intentionally left the wrapped component Check to ensure focus has intentionally left the wrapped component in `withFocusOutside` HOC Aug 19, 2019
Copy link
Contributor

left a comment

This works as intended but it's not fixing all the issues I hoped it would fix :)
It's not fixing #17042 but it's slightly related.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

This works as intended but it's not fixing all the issues I hoped it would fix :)
It's not fixing #17042 but it's slightly related.

I'll take a look at that other bug as I might have some insight now I understand this area of the code better.

Thanks for approving review 👍

@getdave getdave merged commit 3a438b1 into master Aug 20, 2019
4 checks passed
4 checks passed
Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@getdave getdave deleted the fix/with-focus-outside-properly-check-event-location branch Aug 20, 2019
@senadir senadir added this to the Gutenberg 6.4 milestone Aug 25, 2019
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
… `withFocusOutside` HOC (WordPress#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in WordPress#16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
gziolo added a commit that referenced this pull request Aug 29, 2019
… `withFocusOutside` HOC (#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
gziolo added a commit that referenced this pull request Aug 29, 2019
… `withFocusOutside` HOC (#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
… `withFocusOutside` HOC (WordPress#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in WordPress#16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.