Modal, Sheet: Fix infinite focus loop (#649) #1442
Conversation
|
✔️ Deploy Preview for gestalt ready! 🔨 Explore the source changes: f4be74b 🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/621fd81ec6c49200073e428b 😎 Browse the preview: https://deploy-preview-1442--gestalt.netlify.app |
|
Thanks for fixing this bug! We're reviewing this PR internally and we'll get back to you soon. 🙂 |
|
Hey @leoschmitz – Thanks again for the PR. It looks like the stack overflow bug has been fixed! However, it also seems that focus is not being trapped anymore, which you can repro by opening a modal and using the keyboard to tab through the interactive elements. Focus should be confined to the open Modal, but as you can see in this video, we're able to tab through the rest of the elements as well. |
|
Yes, @dangerismycat, you're right. I guess not only blocking the event propagation, but re-trapping it, is necessary after all. One solution I can think of right now is that if we are able to identify that the EventTarget is a TrapFocusBehavior component, we should not call .focus(), but let the EventTarget do its course. So multiple trap components competing would eventually stop at the last registered listener, which would be the topmost Modal. I'm not sure about this last part though, so I'll try it out. Anyways, I'll try do deal with both Trap components competing for the event differently and get back to you. |
|
Sounds good. Thanks again for your work on this! |
The TrapFocusBehavior component listens to all document focus events. If a target of a FocusEvent does not match any children of the trapped component, the event is blocked and a new one is dispatched for the first child of the trapped component. This works well for a single trap. The infinite loop occurs when there is more than one TrapFocusBehaviour trying to trap the focus for its first child. Calling .focus() for it triggers a new FocusEvent, which is then captured by all Trap components, calling .focus() again and filling all call stack up. The solution is to check whether the event has been triggered by a trap component or not. If it has, then let the event run its course. Since the first child is actually the event target, and not the trap component, Element.closest is used to identify if the target has a trap ancestor. In short, * 'trap-focus' name added to identify TrapFocusBehavior components * Element.closest() used to identify trapped components * class component migrated to hook Impact: both Modal and Sheet use TrapFocusBehavior.
|
Hey @dangerismycat, It appears that identifying the event target as a child of the Trap component solves it. Problem is, since the focus goes to the first child of the trap component, I had to search the ancestors up to see if the event came from a Trap component. I did this with the help of a new 'trap-focus' attribute in the TrapFocusBehavior div. I don't know if adding a name there is acceptable. What do you think? |
|
That seems to do the trick! The name seems fine to me, though I'm interested in opinions from @christianvuerings or @jennyscript. |
|
@leoschmitz Thanks again for this work, and our apologies for the lack of action on it. We're taking this issue off the back burner and addressing it again soon — I may end up re-opening and merging this PR, or perhaps adapting it if needed. 🙂 |
|
@leoschmitz I finally had time to look through this, and it looks great! Seems to work perfectly to me. I'm re-opening this PR and will merge it soon. Thank you again for this work, and many apologies for the extended delays! 🙏 |
dangerismycat
changed the title
The TrapFocusBehavior component listens to all document focus events. If
a target of a FocusEvent does not match any children of the trapped
component, the event is blocked and a new one is dispatched for the
first child of the trapped component. This works well for a single trap.
The infinite loop occurs when there is more than one TrapFocusBehaviour
trying to trap the focus for its first child. Calling .focus() for it
triggers a new FocusEvent, which is then captured by all Trap components,
calling .focus() again and filling all call stack up.
The solution is to check whether the event has been triggered by a trap
component or not. If it has, then let the event run its course. Since
the first child is actually the event target, and not the trap
component, Element.closest is used to identify if the target has a trap
ancestor.
In short,
Impact: both Modal and Sheet use TrapFocusBehavior.
Test Plan