Skip to content

Minimally support iframes (nested browsing contexts) in selection event handling #12037

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.

Sign up for GitHub

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

Merged
gaearon merged 29 commits into from
Aug 2, 2018
Merged

Minimally support iframes (nested browsing contexts) in selection event handling #12037

Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6940e33
Prefer node’s window and document over globals
acusti Jan 17, 2018
3f3c1d7
Support active elements in nested browsing contexts
acusti Jan 17, 2018
b08a270
Merge branch 'master' into minimal-iframes
acusti Jan 22, 2018
51be426
Avoid invoking defaultView getter unnecessarily
acusti Jan 23, 2018
3714067
Prefer node’s window and document over globals
acusti Jan 17, 2018
66b8766
Support active elements in nested browsing contexts
acusti Jan 17, 2018
75bc65e
Avoid invoking defaultView getter unnecessarily
acusti Jan 23, 2018
5a61b66
Implement selection event fixtures
Jan 29, 2018
f752792
Prefer node’s window and document over globals
acusti Jan 17, 2018
a8fcf05
Avoid invoking defaultView getter unnecessarily
acusti Jan 23, 2018
0003681
Fix react-scripts to work with alphas after 16.0.0
Feb 12, 2018
fef8519
Run prettier on new selection events fixtures
Feb 12, 2018
799ee39
Add fixture for onSelect in iframes, remove DraftJS fixture
Feb 13, 2018
6e3d4b7
Merge branch 'minimal-iframes' of github.com:brandcast/react into min…
acusti Mar 30, 2018
d1ad016
Purge remnants of draft.js from fixtures
acusti Apr 2, 2018
3c32963
Use prop-types import instead of window global
acusti Apr 2, 2018
3f7b5c5
Make fixtures’ Iframe component Firefox-compatible
acusti Apr 2, 2018
05d8969
Merge branch 'master' into minimal-iframes
acusti Apr 5, 2018
a776570
Merge branch 'master' into minimal-iframes
acusti May 17, 2018
26e9d82
Merge branch 'master' into minimal-iframes
wilsonhyng Jul 13, 2018
4db5c44
Fix switch case for SelectionEventsFixture
wilsonhyng Jul 16, 2018
ccf0329
Remove draft.js / immutable.js dependencies
wilsonhyng Jul 17, 2018
2edf1f8
Cache owner doc as var to avoid reading it twice
wilsonhyng Jul 17, 2018
db02b65
Add documentation for getActiveElementDeep to explain try/catch
wilsonhyng Jul 18, 2018
75b9992
Ensure getActiveElement always returns DOM element
wilsonhyng Jul 24, 2018
0f1de45
Tighten up isNode and isTextNode
wilsonhyng Jul 24, 2018
95b2aef
Remove ie8 compatibility
wilsonhyng Jul 24, 2018
d997ba8
Specify cross-origin example in getActiveElementDeep
wilsonhyng Aug 2, 2018
e63391c
Revert back to returning null if document is not defined
wilsonhyng Aug 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Support active elements in nested browsing contexts
  • Loading branch information
@acusti
acusti authored and Brandon Dail committed Feb 12, 2018
commit 66b87667ccf820b8201486c0e902511d9dd823dd
24 changes: 21 additions & 3 deletions packages/react-dom/src/client/ReactInputSelection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Contributor

@aweary aweary Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check if node exists? In what situation will node not be a DOM node?

Copy link
Contributor Author

@acusti acusti Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@gaearon gaearon Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@acusti acusti Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@nhunzaker nhunzaker Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@acusti acusti Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@nhunzaker nhunzaker Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

node.ownerDocument &&
containsNode(node.ownerDocument.documentElement, node)
);
}

function getActiveElementDeep() {
let win = window;
let element = getActiveElement();
while (element instanceof win.HTMLIFrameElement) {
try {
win = element.contentDocument.defaultView;
} catch (e) {
Copy link
Collaborator

@gaearon gaearon Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are we catching here? Is it defaultView access throwing? Or do we expect contentDocument to potentially be missing?

Copy link
Contributor Author

@acusti acusti Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just try to access the contentDocument of an HTMLIframeElement that comes from a non-matching (separate) domain (like if there’s an embedded YouTube player in the DOM), the browser will throw an error.

Copy link
Contributor

@nhunzaker nhunzaker Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this line describing such a scenario?

return element;
}
element = getActiveElement(win.document);
}
return element;
}

/**
Expand All @@ -33,7 +51,7 @@ export function hasSelectionCapabilities(elem) {
}

export function getSelectionInformation() {
const focusedElem = getActiveElement();
const focusedElem = getActiveElementDeep();
return {
focusedElem: focusedElem,
selectionRange: hasSelectionCapabilities(focusedElem)
Expand All @@ -48,7 +66,7 @@ export function getSelectionInformation() {
* nodes and place them back in, resulting in focus being lost.
*/
export function restoreSelection(priorSelectionInformation) {
const curFocusedElem = getActiveElement();
const curFocusedElem = getActiveElementDeep();
const priorFocusedElem = priorSelectionInformation.focusedElem;
const priorSelectionRange = priorSelectionInformation.selectionRange;
if (curFocusedElem !== priorFocusedElem && isInDocument(priorFocusedElem)) {
Expand Down