Try/use focus outside iframe #52040
Closed
Try/use focus outside iframe #52040
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
force-pushed
the
try/use-focus-outside-iframe
branch
from
c4d0561
to
fbeed05
Compare
ciampo
added
[Package] Compose
|
Size Change: +161 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
ciampo
commented
Jun 28, 2023
|
|
||
| await waitForElementToBeRemoved( confirmDialog ); | ||
|
|
||
| expect( confirmDialog ).not.toBeInTheDocument(); | ||
| expect( onCancel ).toHaveBeenCalled(); | ||
| await waitFor( () => expect( onCancel ).toHaveBeenCalled() ); |
There was a problem hiding this comment.
Due to the extra logic and changes in timeout, a few more callbacks related to onFocusOutside will fire with a bigger delay
|
Flaky tests detected in 6e7c669. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5404653930
|
ciampo
commented
Jun 28, 2023
| // blur events will fire as expected. | ||
| setPollingData( null ); | ||
| } | ||
| }, 50 ); |
There was a problem hiding this comment.
Using 50ms here because it's a sweet spot between not running the internal too frequently, but running it frequently enough that it still feels responsive to users.
ciampo
commented
Jun 28, 2023
| // the timeout delay is necessary to wait for browser's focus event to | ||
| // fire after the blur event, and therefore for this callback to be able | ||
| // to retrieve the correct document.activeElement. | ||
| }, 50 ); |
There was a problem hiding this comment.
This is a bit hacky, but I didn't have any easy solution for it (and didn't want to spend a lot of time trying to find one).
|
There may be some related e2e test failures, although it may just be the case to wait for callbacks / dom changes to fire asynchronously (similarly to some of the changes that this PR already includes for unit tests). Regardless, if y'all think that this approach is promising, feel free to take over (cc @stokesman / @t-hamano ). I may not have much time to work on this in the upcoming couple of months. |
chad1008
reviewed
Jun 29, 2023
I tested this on an iPhone 14 Pro simulator and Pixel 3 XL emulator, both functioned as I expected. Additionally, it appears the proposed changes to not modify the native-specific file. So, I believe this looks good from the mobile editor perspective. 🚀 |
|
Can I test this without building locally? Do not have my Mac on vacation and still trying to get my Windows dev environment running. Vacation might be over by then. 😆 |
@alexstine Not Storybook, but I think the editor can be tested with the Gutenberg PR Previewer. |
stokesman
reviewed
Jul 7, 2023
| ) { | ||
| // If focus is not inside the wrapper (but the document is in focus), | ||
| // we can fire the `onFocusOutside` callback and stop polling. | ||
| currentOnFocusOutside.current( event ); |
There was a problem hiding this comment.
It's maybe odd to pass the event stored from the last blur that caused polling to begin. I don't know that it matters for any current use case but it's not actually the event that causes this invocation of the callback.
|
It looks like we're going to continue with the proposed approach in #51602, therefore I'm going to close this PR. We can always keep this work in mind in case the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Supersedes #45349
Fixes #40912
Why?
The
useFocusOutsidehook doesn't currently behave as expected when focus moves to aniframeelement inside the container element setting the boundaries for the hook (see related issue for an example)How?
By adding a check, in the blur event, to see if the active element is a child of the wrapper element that we're trying to detect focus outside of.
But when the
iframeis in focus, the blur event would not fire as expected. That's why, un case the active element is aniframe, I added extra code to activate a temporary polling function, which checks every 50ms if focus has effectively left the wrapper element.I will highlight a few more details of my changes with inline comments.
Testing Instructions
On Storybook:
ConfirmDialogexample works as expectedPaletteEditbehaves as expected:onFocusOutsideprop is called as expected (see theModalcomponent testing instructions above)In the editor:
In the native editor (cc @geriux @dcalhoun @fluiddot )
Known bug
while testing on my machine, I noticed that on MacOS Safari focus is simply not trapped inside the modal. That seems to also happen on
trunk, so we should probably open a separate issue and treat this separately.Screenshots or screencast
use-focus-outside.mp4