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

Query IFrames focusables #43

Merged
merged 3 commits into from Jan 27, 2023
Merged

Conversation

youennPennarun
Copy link
Contributor

@youennPennarun youennPennarun commented Dec 30, 2022

Currently focus-lock doesn't work as expected when the content of an iframe is dynamically updated.
For example if content of the iframe is added using ReactDOM.createPortal the focus is stuck on the first focusable element: https://codesandbox.io/s/focus-lock-issue-with-dynamic-iframes-hfujkh?file=/src/App.tsx (Click on "Open in iframe" and then try tabbing to a different input. The focus is stuck on "enter your name")

This PR handle iframes the same way it does with shadow nodes and has been heavily inspired by #33. It checks if the active element is an iframe and if it's the case look for tabbable element in the iframe document to prevent the focus to get stuck on the iframe element itself.

@theKashey theKashey self-requested a review December 30, 2022 05:15
@theKashey
Copy link
Owner

Thank you so much for this contribution and especially for the amount of tests added. Please give me a time to process it

@theKashey
Copy link
Owner

The might be another way to handle the same problem, where "problem" is a mismatch of ownerDocument between "top level document" where focus-lock is created and the ones used inside iframe.
This is more about performance balance between scanning from top to bottom rather than helping bubbling a few decisions from the bottom up.

The main - tests and broken example - is here, the rest are details

@youennPennarun
Copy link
Contributor Author

What other solution do you have in mind? Is it something we can do on the consumer side or a different implementation of the fix all together?

@theKashey
Copy link
Owner

So there are 3 cases:

  • ✅ all nodes are in one document
  • ✅ there is iframe inside those nodes, and we don't care about details
  • ⛔ one of the node given to a focus lock does not belong to the current document

You've tried to solve 3rd case using the terms of the first one, however it might be better be solved as a separate one, and might be checking activeElement from the ownerDocument, not document potentially pointing to something else, is what we need.
But there are details yet to uncover. With very big probability you did it right, and I am just trying to verify your solution.

@youennPennarun
Copy link
Contributor Author

Oh right that makes sense. Thanks!

@theKashey
Copy link
Owner

Just marking theKashey/react-focus-lock#218 as related to this change

@theKashey
Copy link
Owner

Merging into main branch continue development...

@theKashey
Copy link
Owner

I've spiked different implementations with ownedDocument read from the element and some functions like focusInside or focusIsHidden are not working as it might be expected by the browser.

However, it allowed me to optimize getActiveElement a little bit.


🙇‍♂️ this PR was flawless from day 1. Thank you for your generous contribution.

@theKashey theKashey merged commit bd44bb5 into theKashey:master Jan 27, 2023
@youennPennarun
Copy link
Contributor Author

Thanks Anton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants