-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Modal
: fix closing when contained iframe is focused
#51602
Conversation
Flaky tests detected in c7824d6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6422264407
|
Size Change: +59 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
9633558
to
3217f61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and tests well for me. I'll let @ciampo/@mirka take a look and provide an approval though, to get more eyeballs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing well for me, although I'd like to also hear @alexstine and @t-hamano 's opinion.
I wish we could find a solution that involves improving the useFocusOutside
hook, since it's used in a few places across Gutenberg that will likely face the same issue as the Modal
component when an iframe
is rendered.
Also noting that, while focus is inside the iframe, pressing the Escape key won't close the Modal (although I guess that is expected and would kind of fall on the consumer of Modal
to decide what to do with keyboard events and iframe
s)
I'll let @ ciampo/ @ lena
I guess you meant @mirka 😄
It's in my queue. Waiting for green tests first. |
Yep. Edited... sorry for the mis-ping 🤦 |
3217f61
to
a7b00b7
Compare
Thanks for testing this out Chad and Marco. 🙇
Agreed. My efforts in that direction haven’t come to fruition and I figured this should be simple enough that it's worth doing for the time being.
👍 😁. There’s some e2e tests that are consistently failing here and so far it’s a head-scratcher. I’d just started to manually run those tests when I noticed the focus return behavior of the modal wasn’t working. I found a fix and pushed bf431ed hoping that would be it but no luck. That’s apparently an existing bug in trunk so I just made #51722 for it. I'll keep looking into these tests and meanwhile kick this back into draft mode. |
bf431ed
to
e7582c9
Compare
Thanks for the PR, @stokesman! This PR looks good to me, including the test, but I still don't know what caused #51722 😅 Furthermore, and this may be a future improvement, if it were possible to close the modal by clicking on the overlay, it would mean that it would be possible to determine which key on the mouse was pressed. In other words, it is possible to prevent the modal from closing when it is not the primary key. There is no correct answer for this, but the behavior of other libraries seems to be as follows:
493722f7629cb47ece6588af7fa3e86f.mp4 |
7db84d0
to
a689fea
Compare
Thanks for taking a look @t-hamano. I appreciate the suggestion to consider the behavior for a non-primary key too. I'd be in favor of not closing the modal then. Back to the tests that are failing here. They are all related to #51737 which this branch happens to escalate. On this branch, the Command Center modal stays open. On trunk, it only flickers open for an instant before closing. In such a situation one might think So I think the only two options that would unblock this are:
|
Testing an alternative solution that only applies changes to |
a689fea
to
b38de98
Compare
b38de98
to
862a27e
Compare
Yes it does:
|
82b405a
to
ecd0a43
Compare
The way it's framed sounds like a11y should be treated as a Focus, not a Type of label. I've opened a PR to address this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the changes test well - unit tests are passing, the storybook iframe example is working well!
One thing that could be done, though, is to add some extra inline comments explaining the logic behind dismissers
, level0Dismissers
, nestedDismissers
and the recursion via the context. To a developer with no context on these changes, it may take a while to understand what it going on.
Finally, we'll also need to add a CHANGELOG entry (I guess under the "Big Fix" section, since this is a fix for #40912 ?
I'm going to preemptively approve this PR, we can merge once all feedback is addressed
const level0Dismissers: MutableRefObject< | ||
ModalProps[ 'onRequestClose' ] | undefined | ||
>[] = []; | ||
const context = createContext( level0Dismissers ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: let's capitalize the variable, and give it a more meaningful name (this better aligns with naming conventions and can help while debugging code in React Dev Tools)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the inline comments are addressed in 145dc0e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the iframe
from the Storybook example before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have an automated test for this but I think it'd likely have to be e2e. Seems like I tried a unit test for this at some point and couldn't pull it off.
Remove focus outside hook.
This reverts commit 6502514.
ecd0a43
to
3c18cfe
Compare
Came here after some investigation for #61313 where there's an ongoing discussion oh whether to implement some modal behaviors to the Popover component. I share the same concern @ciampo expressed above. A popover could contain an iframe. If we implement the current hooks from Overall, the current state of these features seems a little confusing to me and uses ad-hoc implementations that should be better abstracted to be reusable. I'll try to create a new issue soon. |
What?
Fix for a bug in
Modal
it closes when a contained iframe is focused.Why?
It seems like iframes in modals should be supported.
to fix: #40912
How?
Uses an event handler on the Modal’s overlay element to trigger the closing instead of the
useFocusOutside
hook.Testing Instructions