Support iframes (nested browsing contexts) in selection event handling #9184
Conversation
acusti
force-pushed
the
iframes
branch
2 times, most recently
from
7a61aa4
to
380788d
Compare
acusti
force-pushed
the
iframes
branch
2 times, most recently
from
074e315
to
fc9dd51
Compare
|
Just fixed an issue that resulted from me incompletely recreating the changes of #7936 when I originally created this new PR. Then I rebased with latest master. The CircleCI build has 5 React DOM test failures: First failure has a lot of output related to The other failures have no info in the test output. I’m at a loss as to what’s causing the failures in CI. I ran the tests locally and each one of those tests listed above passed. @spicyj Any suggestions or intuitions about the cause? |
React generally handles being rendered into another window context correctly (we have been doing this for a while in native Mac popovers). The main place where there are global window/document accesses are in places where we deal with the DOM selection (window.getSelection() and document.activeElement). There has been some discussion about this in the public React GitHub repo: facebook/fbjs#188 facebook#7866 facebook#7936 facebook#9184 While this was a good starting point, those proposed changes did not go far enough, since they assumed that React was executing in the top-most window, and the focus was in a child frame (in the same origin). Thus for them it was possible to check document.activeElement in the top window, find which iframe had focus and then recurse into it. In our case, the controller and view frames are siblings, and the top window is in another origin, so we can't use that code path. The main reason why we can't get the current window/document is that ReactInputSelection runs as a transaction wrapper, which doesn't have access to components or DOM nodes (and may run across multiple nodes). To work around this I added a ReactLastActiveThing which keeps track of the last DOM node that we mounted a component into (for the initial render) or the last component that we updated (for re-renders). It's kind of gross, but I couldn't think of any better alternatives. All of the modifications are no-ops when not running inside a frame, so this should have no impact for non-elements uses. I did not update any of the IE8 selection API code paths, we don't support it.
@acusti Haha, I know that feeling... Thank you so much for doing all of this work and sticking with us. I'm working through this PR now. In the mean time: I had a couple of questions:
On my end, I need to follow up on the conversation in the prior PRs, some of the associated issues linked on projects, and better understand how React input selection restoration works. I promise to respond in no later than 2 days, even if it's just to say where I'm at. |
|
Just got back from vacation and saw this. Really glad to hear you're gonna dig into this, @nhunzaker! I'll try to update more thoroughly once I'm settled. However, I actually found that the cross-iframe React input selection restoration completely broke focus and blur functionality for all inputs with the release of Chrome v60, so I updated our React form build to completely remove the plugin, and it'd been working great for us since then. I have the suspicion that all of that code may actually have been in place for older browsers and/or React DOM rendering strategies that are no longer relevant or in use. However, making that determination would require knowing the exact set of circumstances that at some point were affected by that plugin, and there is not enough info in the comments for me to be sure of how to verify whether or not the plugin is still needed. However, in light of our success with our app in production without any of that code, I've been planning to create yet another branch that completely removed that particular plugin and includes other changes necessary to make React octane-agnostic, and I will run yarn prettier on that branch, thanks for the suggestion! Would love to hear what you're digging around uncovers about how to create the proper scenario for verifying the React input selection restoration functionality! Also, we can definitely close #7866 and #7936. |
React generally handles being rendered into another window context correctly (we have been doing this for a while in native Mac popovers). The main place where there are global window/document accesses are in places where we deal with the DOM selection (window.getSelection() and document.activeElement). There has been some discussion about this in the public React GitHub repo: facebook/fbjs#188 facebook#7866 facebook#7936 facebook#9184 While this was a good starting point, those proposed changes did not go far enough, since they assumed that React was executing in the top-most window, and the focus was in a child frame (in the same origin). Thus for them it was possible to check document.activeElement in the top window, find which iframe had focus and then recurse into it. In our case, the controller and view frames are siblings, and the top window is in another origin, so we can't use that code path. The main reason why we can't get the current window/document is that ReactInputSelection runs as a transaction wrapper, which doesn't have access to components or DOM nodes (and may run across multiple nodes). To work around this I added a ReactLastActiveThing which keeps track of the last DOM node that we mounted a component into (for the initial render) or the last component that we updated (for re-renders). It's kind of gross, but I couldn't think of any better alternatives. All of the modifications are no-ops when not running inside a frame, so this should have no impact for non-elements uses. I did not update any of the IE8 selection API code paths, we don't support it. (cherry picked from commit 94b759b in the 0.14-stable. Appears to work mostly as is, needed to be updated to take 5c5d2ec into account)
|
@nhunzaker I’m keeping this open for now in case it turns out that the React input selection restoration plugin is still necessary with React fiber and latest browsers and such. However, the latest version of this effort that I think should be reviewed and tested is #11410. |
|
Cool! Thanks for... sending out yet another PR. I really appreciate it. |
acusti
force-pushed
the
iframes
branch
2 times, most recently
from
1dd8abb
to
c5e3925
Compare
If an input inside a same-domain iframe is the actual activeElement, this change will make the ReactInputSelection module get from and restore selection to that input, rather than the iframe element itself
Had to remove some tests because they rely on DOM functionality that jsdom doesn’t yet support, like getSelection and iframe activeElement
| var doc = | ||
| nativeEventTarget.ownerDocument || | ||
| nativeEventTarget.document || | ||
| nativeEventTarget; |
There was a problem hiding this comment.
Could you explain what nativeEventTarget can be? When would an element be missing an owner document, but have a document? What are the usual values ofnativeEventTarget?
There was a problem hiding this comment.
If the event was dispatched at the window level, like if you had a window.onresize listener, for example, the event target would be the window object and would therefore have a document but no ownerDocument. And if it was dispatched on the window object of a nested browsing context, it would be the window object of that iframe, not the global one.
However, looking at where this gets invoked, I see that the call site has it’s own nested browsing context-compatible way to calculate doc that was added a year ago: 6e04bd7
I guess I’ll just repeat the same logic here, if that makes sense to you; i.e. I’ll replace these lines with:
var doc =
nativeEventTarget.window === nativeEventTarget
? nativeEventTarget.document
: nativeEventTarget.nodeType === DOCUMENT_NODE
? nativeEventTarget
: nativeEventTarget.ownerDocument;There was a problem hiding this comment.
Considering they’re in the same file, I’m making a getEventTargetDocument function. I dropped native from the name because I don’t think that distinction is relevant at the level of this function.
| return event.clipboardData; | ||
| } | ||
| var doc = (event.target && event.target.ownerDocument) || document; | ||
| return doc.defaultView.clipboardData; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
What does clipboard data have to do with selection?
There was a problem hiding this comment.
Nothing to do with selection; this is just another part of the codebase that relies on the global window object rather than the window object relative to the DOM node being used. That said, I just looked it up, and window.clipboardData is an IE-only API and one where it seems like the code being executed should only try to look up it’s own window.clipboardData (because of browser permissions / security settings), so I’m going to revert this set of changes.
There was a problem hiding this comment.
Ah still, do you think it's worth landing? What do you think about sending this out in a separate PR?
There was a problem hiding this comment.
I don’t know really know anything about clipboard APIs in IE, but based on a quick google search, my hunch is that this change wouldn’t actually be for the better. There’s some mentions about browser settings and issues with clipboardData not being accessible across different browsing contexts, so keeping it hardcoded to window.clipboardData might actually be correct.
But I’d be happy to open a separate PR with it if someone would be able to figure that all out.
There was a problem hiding this comment.
This is great work, and solves a very interesting problem. I'd like to get some opinions from other members of the team, but I'd also like to start thinking about the best way to test browser support for this. Much of that is simply because I don't have much experience with selection APIs.
So let's make a manual DOM fixture! Is there a good code example that we could pull in to dom text fixtures? I think this warrants a whole section.
Again, great work!
@jquense @aweary I think the next steps here are to do browser testing and I really want to dig into a few of the specific selection APIs. What team members do you think are most familiar with selection stuff?
I'll ping Dan when he's back from vacation :P
|
@nhunzaker Thanks so much for the review! I addressed all the comments and pushed up the changes. I also pushed up a fix for a bug in The bug came up in our internal team’s QA of this branch of work in our app and is fixed by that change. I’d also be happy to update |
|
@nhunzaker I decided to go ahead and update |
|
@nhunzaker I merged latest master and resolved the conflicts. Also added a guard based on a test failure and converted all class Iframe extends React.Component {
componentDidMount() {
const doc = this.getDocument();
doc.open();
doc.write(`<!doctype html><html><head><style>${iframeStyles}</style><body><div></div></body></html>`);
doc.close();
this.renderIframeContents();
}
componentDidUpdate() {
this.renderIframeContents();
}
getDocument() {
return ReactDOM.findDOMNode(this).contentWindow.document;
}
renderIframeContents() {
const doc = this.getDocument();
const mountTarget = doc.body.children[0];
ReactDOM.unstable_renderSubtreeIntoContainer(this, this.props.children, mountTarget);
}
render() {
return <iframe />;
}
}That component could be combined with an example like the one @aweary posted in another PR thread to create a fixture that verifies that all of the You also mentioned wanting to ping Dan when he was back from vacation. Thinking you all might be back on vacation again, but maybe 2018 will be a good opportunity to ping him and get this rolling again. |
|
What is the difference in bundle size? Maybe it can be okay if:
But I'm not sure this case is important enough to offset the complexity it introduces, and I'm not very optimistic about this landing. I really appreciate your effort though. |
|
I would love for this change to make it in. Using React portals greatly simplifies multi-window apps using the pattern documented here. There does seem to be a fair amount of demand for rendering into iframes as well - react-frame-component has 39 dependent packages and 20,627 downloads over the past month. In my opinion, the complexity required in fixing multi-context selection seems a worthwhile trade off in order to fully support this class of interesting React usage patterns. |
|
Fair enough. I'd like to see the bundle size difference I guess. |
|
@gaearon In order to show the bundle size difference, I originally committed Bundle size difference tableBeing able to render into an iframe is very powerful. It’s actually the only way to properly solve two problems:
If you want a demo of both of those use cases in action (using this fork of react plus the draft-js fork) and a pretty compelling case for why these changes are worth it, check out Brandcast, our app. We recently replaced our usage of In terms of complexity of this PR, I could simplify the changes necessary to make React work across nested browsing contexts (iframes or separate windows in multi-window apps). This set of changes is the most complete solution that handles any number of elements with visible selections across any number of nested browsing contexts with any level of recursion. I made another PR that takes the opposite approach and removes the React input selection restoration plugin entirely: #11410. However, as @sophiebits and @aweary pointed out, the plugin does still serve a purpose, though it’s a pretty edge-casey one. My first attempt at this set of changes, #7866 (from October 2016), implemented a third approach that was simpler but less complete. It only tries to preserve and restore the one active selection in the app and does nothing about any other visible (but inactive) selections. I would be willing to redo that approach against latest Lastly, @gaearon, you mentioned:
Are you referring to the exported Thanks for checking out the PR and giving your feedback! And as I mentioned previously to @nhunzaker, don’t hesitate to ask me for anything, whether info, documentation, questions, code, etc. |
|
I'm looking at the related PR on Draft.js that would enable iframe support there, and it would be helpful to get a verdict on this before I consider that one. I'm going to bring this to the other maintainers and hopefully we can get an answer one way or another. Thanks for your ongoing work on this @acusti and to all the folks guiding this work. |
|
Just an update - the React team at Facebook has discussed this a bit, still need to discuss more, but it is on our radar and hope to have an answer soon. |
|
@flarnie @gaearon I decided to go ahead and create a new PR that implements the minimal changes required to make React work across nested browsing contexts (with 0% or even -1% changes to the bundle size), as referred to in my last comment. The PR: #12037 I hope between these two approaches we can find something that everyone feels comfortable with being merged. |
|
:) Thanks for doing that @acusti - we are looking for a maintainer to take point on this; might take a while but it is not forgotten about. |
|
We went with #12037 |
I tried to rebase #7936 with latest master and everything was thoroughly screwed up. So rather than trying to merge master, I just made a new clean branch off master and cherry-picked the commits over. I’m not closing the original PR at this point, however, because there seem to be many interested people following the activity over there, and I don’t want to prevent them from doing so. I’ll make new built versions of everything off of these commits applied to 15-stable and update the npm / yarn instructions on using those when I get a chance.
I’ve also made a commit with a couple guards to accommodate varying cross-browser behaviors. The guards are for #7936 (comment) and summernote/summernote#1057