Bail out of dispatching a listener in useEventListenerOutside when the document doesn't have focus
#620
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 273a0cc:
|
|
Deploy preview for reakit ready! Built with commit 273a0cc |
|
It appears that the reason this issue appears on Firefox and not Chrome is that on Firefox the focus event fires with document as the target (making the For resizing, focus events aren't fired in Chrome but they are in Firefox; again, with the target set to the document. |
|
You can verify that it's fixed using the first Modal example in the deploy preview: https://deploy-preview-620--reakit.netlify.com/docs/dialog/#usage |
It seems that all clicks outside were disabled now. Tests are failing because of this.
So I guess the check should be |
Codecov Report
@@ Coverage Diff @@
## master #620 +/- ##
=======================================
Coverage 95.37% 95.37%
=======================================
Files 118 118
Lines 2269 2272 +3
Branches 674 675 +1
=======================================
+ Hits 2164 2167 +3
Misses 105 105
Continue to review full report at Codecov.
|
| } | ||
| }, | ||
| options.visible && options.hideOnClickOutside | ||
| ); | ||
|
|
||
| const document = getDocument(dialogRef.current); |
There was a problem hiding this comment.
dialogRef.current may not be populated at the time the useHideOnClickOutside function is called, so this should be inside the event handler in this case (it could be outside the event handler on the other custom hook because it was still inside useEffect.
There was a problem hiding this comment.
It sounds like getDocument shouldn't accept null as a parameter in that case, should the type be changed? Looking at it's call sites, it currently only accepts null from getActiveElement, and getActiveElement's call sites don't pass in null either.
There was a problem hiding this comment.
getDocument will return the current window's document as a fallback. My understanding is that that's safer as most people will not need the specific element.ownerDocument (as far as I know, only those using components inside of an iframe will need it).
But we can discuss it better on #456
There was a problem hiding this comment.
Ah ok, vscode missed a load of callsites where null unions were being passed in anyway so the change isn't as trivial as I first thought.
There was a problem hiding this comment.
It looks like it's working on Firefox now, and tests are passing! Thanks @tom-sherman! ❤️
| if (mouseDownRef.current === event.target && options.hide) { | ||
| options.hide(); | ||
| if (mouseDownRef.current === event.target) { | ||
| options.hide?.(); |
Fixes #619
Does this PR introduce a breaking change?
No