Minimally support iframes (nested browsing contexts) in selection event handling #12037
Conversation
|
@orta Doesn't seem like the hook worked here either :-( I'm not sure I understood your message about the PR status earlier. Do you mean it's not always reliable? Is there any way to force-trigger it? |
|
I'll take a look at this within the next week or so. We should also implement a fixture that renders some text inputs (or maybe a Draft.js editor?) in an iframe to make it easy for contributors to validate the new behavior. |
|
It should be reliable in cases like this (it's possibly unreliable on a same-repo PR, this is cross repo) (but you can re-run CI and it would fix it and by that point everything should be hooked up). Those titles must have been my change, I can fix that too. |
|
(The above bot comment is wrong, sorry 😞 We'll fix that soon) |
There was a problem hiding this comment.
@acusti could you provide a list of behaviors that currently don't work on master, which this PR would fix? I want to setup a DOM fixture set for this (here's some initial work on that) so it would be helpful to know exactly what we should be testing for.
| return; | ||
| } | ||
|
|
||
| const selection = window.getSelection(); | ||
| const selection = doc.defaultView.getSelection(); |
There was a problem hiding this comment.
defaultView is defined as a getter, and since we don't expect it to change within setOffsets maybe we should store the value in a local variable so we avoid triggering the getter twice.
There was a problem hiding this comment.
I made that change (for this line and two other instances) in 51be426
| @@ -12,7 +12,25 @@ import * as ReactDOMSelection from './ReactDOMSelection'; | |||
| import {ELEMENT_NODE} from '../shared/HTMLNodeType'; | |||
|
|
|||
| function isInDocument(node) { | |||
| return containsNode(document.documentElement, node); | |||
| return ( | |||
| node && | |||
There was a problem hiding this comment.
Why do we need to check if node exists? In what situation will node not be a DOM node?
There was a problem hiding this comment.
The value that gets passed in comes from fbjs/lib/getActiveElement, which returns a nullable HTMLElement (https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/core/dom/getActiveElement.js#L21). The only instance where the return value would be null is if the util was unable to find a document object, which I think would only happen in SSR. But because it is theoretically nullable, all the operations in this file first confirm that node (or elem, in hasSelectionCapabilities) is truthy.
There was a problem hiding this comment.
We don't use fbjs/lib/getActiveElement anymore, and have copied it in the repo. Let's tighten this up? Feel free to change getActiveElement as you see fit.
There was a problem hiding this comment.
@gaearon We can update getActiveElement.js to look like:
export default function getActiveElement(doc: ?Document): Element {
doc = doc || document;
try {
return doc.activeElement || doc.body;
} catch (e) {
return doc.body;
}
}But strictly speaking, flow will still complain that document.body can be null (ref: facebook/flow#4783 (comment)), so strictly speaking, the Element return value still has to be ?Element, unless we did something like
export default function getActiveElement(doc: ?Document): Element {
doc = doc || document;
const body = doc.body || doc.createElement('body');
try {
return doc.activeElement || body;
} catch (e) {
return body;
}
}Do you have a preferred approach?
There was a problem hiding this comment.
I want to lean towards the first, but I keep reasoning out of it in favor of the second. Body could be null, and it really it should never happen. But I've been surprised too much before :).
With the second example, do you need a try/catch?
Also: do you anticipate any problems with code downstream working with a document body that isn't attached?
There was a problem hiding this comment.
@nhunzaker No try/catch needed for the second example, and the ReactInputSelection plugin won’t have any issues; the code is already setup to handle detached DOM elements for a case where the active element becomes detached between when it is first read and cached and after React finishes committing an update. The second option has grown on me; I suggested it thinking it was silly, but now feel like it’s pretty reasonable. If we go with that one, should we add a comment explaining that document.body can be null?
There was a problem hiding this comment.
The second option has grown on me; I suggested it thinking it was silly, but now feel like it’s pretty reasonable. If we go with that one, should we add a comment explaining that document.body can be null?
Let's go with it 👍
|
@aweary I created a version of my Draft.js in an iframe demo that uses the current stable release of React (as opposed to builds based upon my fork): https://codepen.io/acusti/pen/YYgmBE If you start interacting with the draft editor in that demo (the primary text area), you will notice some things that don’t work:
My experience with these cross-iframe bugs has centered on using a Draft editor in the iframe, and the issues I have seen there (and described above) come from While the above example is a good practical illustration of the current issues, it isn’t a reduced test case, so I put together a much simpler example that increases the count of a state variable in an
For all of the above examples, I used class Iframe extends React.Component {
iframeRef = null;
handleRef = ref => {
if (ref !== this.iframeRef) {
this.iframeRef = ref;
if (ref) {
if (ref.contentDocument && this.props.head) {
ref.contentDocument.head.innerHTML = this.props.head;
}
// Re-render must take place in the next tick (Firefox)
setTimeout(() => {
this.forceUpdate();
});
}
}
};
render() {
const ref = this.iframeRef;
let portal;
if (ref && ref.contentDocument) {
portal = ReactDOM.createPortal(
this.props.children,
ref.contentDocument.body,
);
}
return (
<React.Fragment>
<iframe ref={this.handleRef} />
{portal}
</React.Fragment>
);
}
}It is used like so: (
<Iframe head="<style>input{font-size:24px}</style>">
<input placeholder="An input inside an iframe!" />
</Iframe>
)Looking at your skeleton outline for the fixtures, I don’t know how to use The other behavior that is currently broken in React is restoring selection to an active element inside an iframe, due to assumptions both in ReactInputSelection.js and in the |
|
Thanks for the great writeup @acusti. I'm going to work on setting up a fixture for this using the |
|
OK, I implemented a couple basic fixtures for selection events in aweary@dff3e52. I've deployed them here: http://react-fixtures-iframe-selection-events.surge.sh/selection-events It's currently using your version of Draft.js so it works in iframes. If possible it would be nice to also add a test case for the same behavior without Draft.js, but for now I think it's fine. I'll run through these fixtures soon, and then I can just push the commit to your branch and include them as part of this PR. If there are any other scenarios you think we should test @acusti let me know and I can add a test case for them! |
|
@aweary Those fixtures look great! For the 2nd fixture’s description, I would suggest something like:
Agreed that it would be nice to have an example that tests selection event triggering without Draft.js, but I think it would likely wind up being a lot less practical. As an aside, using the first “Reordered input elements in iframes” fixture illustrates well the difference between the set of changes implemented in this PR as opposed to the more complex changes in #9184. When you make a selection in one of those inputs and it remains the |
|
@aweary Would you like me to throw together a fixture that doesn’t rely on Draft.js? Or anything else I can help with to keep this moving along? |
|
@acusti that would be great, feel free to build off of the fixtures I have in aweary@dff3e52. I'll go ahead and push those to this branch if they look OK to you. There's nothing blocking this other than testing the fixtures in our supported browsers. There's no definite list of those, but #9301 (comment) is a good starting point. |
|
@aweary You should definitely push those to this branch, that would be great. I’ll go ahead and make the language change I suggested in #12037 (comment), then I’ll add a fixture that checks |
|
@acusti I've pushed the fixtures to your branch 👍 for now we can just test them with the version of Draft.js you provided, but we'll need to either get it working with an official release or remove Draft.js altogether before merging.
We don't require any screenshots or anything, we just go through the list and manually verify everything is working as expected. You can write up a check list and share it with us afterwards if that works with you :) I'll start testing myself today. |
The current logic just checks if the version is an alpha with a major version of 16 to account for weirdness with the 16 RC releases, but now we have alphas for newer minor releases that don't have weirdness
aweary
force-pushed
the
minimal-iframes
branch
from
cdcd6f6
to
fef8519
Compare
The DraftJs fixture wasn't really working in all supported browsers anyways, so just drop it and try to cover our bases without using it directly
|
Woooo! Congrats @acusti and @wilsonhyng! And thanks for sticking with us! It's super exciting to get this merged! |
|
@acusti @wilsonhyng this is pretty sweet |
As mentioned in #9184 (comment), this PR takes a minimal approach to making React work when rendering selectable elements across nested browsing contexts (iframes or browser windows opened via
window.open). That means changingReactDOMSelectionandSelectEventPluginto use thewindowanddocumentobjects relative to the DOM node being handled where possible, and updatingReactInputSelectionto find and restore selection to the single active element on the page, even when that element is inside a nested browsing context. I did a quick test using the code in this PR of rendering a Draft editor into an iframe and it seems sufficient to make it all work without issue.To try this out in your own project, update your
react-domdependency string togithub:brandcast/react-dom-built#e673e48. Also make sure you are using latest React (16.2.0) to avoid a version mismatch.I made additional fixes to ReactInputSelection’s
hasSelectionCapabilitiesfunction in 82443f7 that I have not replicated here to avoid bloating the PR. If I should add that change to this PR, let me know and I’d be happy to do so.@gaearon Attaching the full bundle size reporting table (note that the handling of the markdown table headers seems somewhat borked in
masterso the table is a bit unwieldy)Bundle size reporting table