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

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

Merged
merged 29 commits into from
Aug 2, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
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
3 changes: 2 additions & 1 deletion fixtures/dom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
"dependencies": {
"classnames": "^2.2.5",
"core-js": "^2.4.1",
"draft-js": "^0.10.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aweary Just double checking: we can remove this dependency now, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct!

"prop-types": "^15.6.0",
"query-string": "^4.2.3",
"react": "^15.4.1",
"react-dom": "^15.4.1",
"semver": "^5.3.0"
"semver": "^5.5.0"
},
"scripts": {
"start": "react-scripts start",
Expand Down
1 change: 1 addition & 0 deletions fixtures/dom/public/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<title>React App</title>
<script src="https://unpkg.com/prop-types@15.5.6/prop-types.js"></script>
<script src="https://unpkg.com/expect@1.20.2/umd/expect.min.js"></script>
<script src="https://unpkg.com/immutable@3.8.2/dist/immutable.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilsonhyng I believe we don’t need immutable.js in the fixtures anymore now that we’ve removed the draft.js dependency. @aweary Please correct me if I’m wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct!

</head>
<body>
<div id="root"></div>
Expand Down
2 changes: 1 addition & 1 deletion fixtures/dom/src/components/Fixture.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const PropTypes = window.PropTypes;
import PropTypes from 'prop-types';
const React = window.React;

const propTypes = {
Expand Down
1 change: 1 addition & 0 deletions fixtures/dom/src/components/Header.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class Header extends React.Component {
<option value="/media-events">Media Events</option>
<option value="/pointer-events">Pointer Events</option>
<option value="/mouse-events">Mouse Events</option>
<option value="/selection-events">Selection Events</option>
</select>
</label>
<label htmlFor="react_version">
Expand Down
57 changes: 57 additions & 0 deletions fixtures/dom/src/components/Iframe.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
const React = window.React;
const ReactDOM = window.ReactDOM;

class IframePortal 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 = null;
if (ref && ref.contentDocument) {
portal = ReactDOM.createPortal(
this.props.children,
ref.contentDocument.body
);
}

return (
<div>
<iframe
style={{border: 'none', height: this.props.height}}
ref={this.handleRef}
/>
{portal}
</div>
);
}
}

class IframeSubtree extends React.Component {
warned = false;
render() {
if (!this.warned) {
console.error(
`IFrame has not yet been implemented for React v${React.version}`
);
this.warned = true;
}
return <div>{this.props.children}</div>;
}
}

export default (ReactDOM.createPortal ? IframePortal : IframeSubtree);
3 changes: 3 additions & 0 deletions fixtures/dom/src/components/fixtures/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import CustomElementFixtures from './custom-elements';
import MediaEventsFixtures from './media-events';
import PointerEventsFixtures from './pointer-events';
import MouseEventsFixtures from './mouse-events';
import SelectionEventsFixtures from './selection-events';

const React = window.React;

Expand Down Expand Up @@ -52,6 +53,8 @@ function FixturesPage() {
return <PointerEventsFixtures />;
case '/mouse-events':
return <MouseEventsFixtures />;
case '/selection-events':
return <SelectionEventsFixtures />;
default:
return <p>Please select a test fixture.</p>;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import TestCase from '../../TestCase';
import Iframe from '../../Iframe';
const React = window.React;

class OnSelectIframe extends React.Component {
state = {count: 0, value: 'Select Me!'};

_onSelect = event => {
this.setState(({count}) => ({count: count + 1}));
};

_onChange = event => {
this.setState({value: event.target.value});
};

render() {
const {count, value} = this.state;
return (
<Iframe height={60}>
Selection Event Count: {count}
<input
type="text"
onSelect={this._onSelect}
value={value}
onChange={this._onChange}
/>
</Iframe>
);
}
}

export default class OnSelectEventTestCase extends React.Component {
render() {
return (
<TestCase
title="onSelect events within iframes"
description="onSelect events should fire for elements rendered inside iframes">
<TestCase.Steps>
<li>Highlight some of the text in the input below</li>
<li>Move the cursor around using the arrow keys</li>
</TestCase.Steps>
<TestCase.ExpectedResult>
The displayed count should increase as you highlight or move the
cursor
</TestCase.ExpectedResult>
<OnSelectIframe />
</TestCase>
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import TestCase from '../../TestCase';
import Iframe from '../../Iframe';
const React = window.React;

export default class ReorderedInputsTestCase extends React.Component {
state = {count: 0};

componentDidMount() {
this.interval = setInterval(() => {
this.setState({count: this.state.count + 1});
}, 2000);
}

componentWillUnmount() {
clearInterval(this.interval);
}

renderInputs() {
const inputs = [
<input key={1} defaultValue="Foo" />,
<input key={2} defaultValue="Bar" />,
];
if (this.state.count % 2 === 0) {
inputs.reverse();
}
return inputs;
}

render() {
return (
<TestCase title="Reordered input elements in iframes" description="">
<TestCase.Steps>
<li>The two inputs below swap positions every two seconds</li>
<li>Select the text in either of them</li>
<li>Wait for the swap to occur</li>
</TestCase.Steps>
<TestCase.ExpectedResult>
The selection you made should be maintained
</TestCase.ExpectedResult>
<Iframe height={50}>{this.renderInputs()}</Iframe>
</TestCase>
);
}
}
19 changes: 19 additions & 0 deletions fixtures/dom/src/components/fixtures/selection-events/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import FixtureSet from '../../FixtureSet';
import ReorderedInputsTestCase from './ReorderedInputsTestCase';
import OnSelectEventTestCase from './OnSelectEventTestCase';
const React = window.React;

export default function SelectionEvents() {
return (
<FixtureSet
title="Selection Restoration"
description="
When React commits changes it may perform operations which cause existing
selection state to be lost. This is manually managed by reading the
selection state before commits and then restoring it afterwards.
">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

<ReorderedInputsTestCase />
<OnSelectEventTestCase />
</FixtureSet>
);
}
6 changes: 5 additions & 1 deletion fixtures/dom/src/react-loader.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import semver from 'semver';

/**
* Take a version from the window query string and load a specific
* version of React.
Expand Down Expand Up @@ -42,9 +44,11 @@ export default function loadReact() {
let version = query.version || 'local';

if (version !== 'local') {
const {major, minor, prerelease} = semver(version);
const [preReleaseStage, preReleaseVersion] = prerelease;
// The file structure was updated in 16. This wasn't the case for alphas.
// Load the old module location for anything less than 16 RC
if (parseInt(version, 10) >= 16 && version.indexOf('alpha') < 0) {
if (major >= 16 && !(minor === 0 && preReleaseStage === 'alpha')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

REACT_PATH =
'https://unpkg.com/react@' + version + '/umd/react.development.js';
DOM_PATH =
Expand Down
18 changes: 17 additions & 1 deletion fixtures/dom/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2153,6 +2153,14 @@ dotenv@4.0.0:
version "4.0.0"
resolved "https://registry.yarnpkg.com/dotenv/-/dotenv-4.0.0.tgz#864ef1379aced55ce6f95debecdce179f7a0cd1d"

draft-js@^0.10.5:
version "0.10.5"
resolved "https://registry.yarnpkg.com/draft-js/-/draft-js-0.10.5.tgz#bfa9beb018fe0533dbb08d6675c371a6b08fa742"
dependencies:
fbjs "^0.8.15"
immutable "~3.7.4"
object-assign "^4.1.0"

duplexer2@^0.1.4:
version "0.1.4"
resolved "https://registry.yarnpkg.com/duplexer2/-/duplexer2-0.1.4.tgz#8b12dab878c0d69e3e7891051662a32fc6bddcc1"
Expand Down Expand Up @@ -2672,7 +2680,7 @@ fbjs@^0.8.1, fbjs@^0.8.4:
setimmediate "^1.0.5"
ua-parser-js "^0.7.9"

fbjs@^0.8.16:
fbjs@^0.8.15, fbjs@^0.8.16:
version "0.8.16"
resolved "https://registry.yarnpkg.com/fbjs/-/fbjs-0.8.16.tgz#5e67432f550dc41b572bf55847b8aca64e5337db"
dependencies:
Expand Down Expand Up @@ -3302,6 +3310,10 @@ ignore@^3.3.3:
version "3.3.3"
resolved "https://registry.yarnpkg.com/ignore/-/ignore-3.3.3.tgz#432352e57accd87ab3110e82d3fea0e47812156d"

immutable@~3.7.4:
version "3.7.6"
resolved "https://registry.yarnpkg.com/immutable/-/immutable-3.7.6.tgz#13b4d3cb12befa15482a26fe1b2ebae640071e4b"

imurmurhash@^0.1.4:
version "0.1.4"
resolved "https://registry.yarnpkg.com/imurmurhash/-/imurmurhash-0.1.4.tgz#9218b9b2b928a238b13dc4fb6b6d576f231453ea"
Expand Down Expand Up @@ -5878,6 +5890,10 @@ semver@^5.0.3:
version "5.4.1"
resolved "https://registry.yarnpkg.com/semver/-/semver-5.4.1.tgz#e059c09d8571f0540823733433505d3a2f00b18e"

semver@^5.5.0:
version "5.5.0"
resolved "https://registry.yarnpkg.com/semver/-/semver-5.5.0.tgz#dc4bbc7a6ca9d916dee5d43516f0092b58f7b8ab"

send@0.14.1:
version "0.14.1"
resolved "https://registry.yarnpkg.com/send/-/send-0.14.1.tgz#a954984325392f51532a7760760e459598c89f7a"
Expand Down
12 changes: 8 additions & 4 deletions packages/react-dom/src/client/ReactDOMSelection.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import {TEXT_NODE} from '../shared/HTMLNodeType';
* @return {?object}
*/
export function getOffsets(outerNode) {
const selection = window.getSelection && window.getSelection();
const win =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: let's not read ownerDocument twice. Put it in a variable.

(outerNode.ownerDocument && outerNode.ownerDocument.defaultView) || window;
const selection = win.getSelection && win.getSelection();

if (!selection || selection.rangeCount === 0) {
return null;
Expand Down Expand Up @@ -150,11 +152,13 @@ export function getModernOffsetsFromPoints(
* @param {object} offsets
*/
export function setOffsets(node, offsets) {
if (!window.getSelection) {
const doc = node.ownerDocument || document;
const win = doc ? doc.defaultView : window;
if (!win.getSelection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just doing some poking around:

image

In what cases does win not have getSelection, and would it be good to document this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of IE8, and possibly other very old browsers. We can remove the early return if that no longer matters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. That is fine by me. Sound good, @aweary?

return;
}

const selection = window.getSelection();
const selection = win.getSelection();
const length = node[getTextContentAccessor()].length;
let start = Math.min(offsets.start, length);
let end = offsets.end === undefined ? start : Math.min(offsets.end, length);
Expand All @@ -180,7 +184,7 @@ export function setOffsets(node, offsets) {
) {
return;
}
const range = document.createRange();
const range = doc.createRange();
range.setStart(startMarker.node, startMarker.offset);
selection.removeAllRanges();

Expand Down
24 changes: 21 additions & 3 deletions packages/react-dom/src/client/ReactInputSelection.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,25 @@ function containsNode(outerNode, innerNode) {
}

function isInDocument(node) {
return containsNode(document.documentElement, node);
return (
node &&
Copy link
Contributor

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

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

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

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

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

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

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

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

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

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 Down Expand Up @@ -80,7 +98,7 @@ export function hasSelectionCapabilities(elem) {
}

export function getSelectionInformation() {
const focusedElem = getActiveElement();
const focusedElem = getActiveElementDeep();
return {
focusedElem: focusedElem,
selectionRange: hasSelectionCapabilities(focusedElem)
Expand All @@ -95,7 +113,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
Loading