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

Support iframes (nested browsing contexts) in selection event handling for React 16 #11410

Closed
wants to merge 8 commits into from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Oct 31, 2017

Fixes #427

As promised in #9184 (comment), here’s a version of #9184 that is based on React 16 and entirely removes the React input selection restoration plugin (other versions of this work include #7866 and #7936).

@nhunzaker was most recently active in the latest previous PR and had mentioned wanting to look into the React input selection restoration logic. I’m not sure how far he was able to get with that, but I’d love to know anything discovered! We are currently QAing the built version of the work in this PR to verify it for our own uses, but a quick initial pass was promising. As mentioned elsewhere, making the React input selection restoration logic work across nested browsing contexts broke regular text input blur as of Chrome 60, so I tried removing the whole plugin, and everything worked beautifully. I don’t know React’s official policy on browser support, but so far, in the latest 2 versions of every browser we’ve tested, there seem to be no regressions from removing that plugin. According to the comments in the file:

If any selection information was potentially lost, restore it. This is useful when performing operations that could remove dom nodes and place them back in, resulting in focus being lost.

The best way to test if the plugin is still needing would of course be to set up a component that can have selections and then cause a render that would result in it being removed and then placed back in within that single render transaction, but I don’t know how to make that happen, nor do I know if that is in fact still possible with React fiber.

For anyone who wants to test these changes out in their own project, you can do so by updating the react and react-dom dependencies in your package.json to:

    "react": "brandcast/react-built#7c8fc6b",
    "react-dom": "brandcast/react-dom-built#017534a",

If you’re also using draft.js and want the full latest and greatest cross-iframe experience, use:

    "draft-js": "brandcast/draft-js-built#003f24b",

@acusti
Copy link
Contributor Author

acusti commented Oct 31, 2017

Ok, I just looked at the failing test and found more helpful notes about the React input selection restoration code from @sophiebits:

When the second input is added, let's simulate losing focus, which is something that could happen when manipulating DOM nodes (but is hard to deterministically force without relying intensely on React DOM implementation details)

Open question for Sophie and anyone else who may know: is the circumstance referred to in those notes, in which an input loses focus (presumably from “performing operations that could remove dom nodes and place them back in”, as mentioned in the comments in ReactInputSelection.js), still possible with react fiber?

One more piece of info garnered from the breaking test. It was added in 0c885af, along with the commit message:

Restore DOM selection and suppress events (#8353)
This makes Draft mostly work.

Is Draft a good candidate for testing whether the ReactInputSelection plugin is still needed? At least in our testing, it seems to be working well (across iframes, no less) using this branch without it.

@sophiebits
Copy link
Collaborator

I think that's a decent test plan. Can you try having a draft editor and a non-draft sibling where you swap the position of the two on each keystroke? The selection should be kept. (That seems kind of contrived but I think it's a case where selection might otherwise be lost.)

@aweary
Copy link
Contributor

aweary commented Oct 31, 2017

Thanks for the PR @acusti!

Open question for Sophie and anyone else who may know: is the circumstance referred to in those notes, in which an input loses focus (presumably from “performing operations that could remove dom nodes and place them back in”, as mentioned in the comments in ReactInputSelection.js), still possible with react fiber?

Yes, this is still possible. I'm not sure on the specific operations that will cause elements to lose focus, but here's an example:

Enter some text into any of the moving inputs and then highlight some portion of it. You'll see that with the master build the selection is maintained no matter where the input moves. With the iframes-16 build the selection state is lost if the input is moved to the right (tested in Chrome 62).

So with that context in mind, I don't think totally removing ReactInputSelection will be the right approach.

@acusti
Copy link
Contributor Author

acusti commented Nov 1, 2017

Excellent, thanks so much for the explanations. I will revive the ReactInputSelection plugin and debug the input focus issues to figure out why that broke. Also, I’ll close this PR and return to #9185.

@acusti
Copy link
Contributor Author

acusti commented Nov 1, 2017

I fixed the input focus / blur issues mentioned above in #9184, so I am now closing this PR.

@acusti acusti closed this Nov 2, 2017
@ahutchings
Copy link

For anyone else looking for the updated PR, looks like it's #9184 rather than #9185.

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

Successfully merging this pull request may close these issues.

5 participants