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

Make DOM utilities work in nested browsing contexts #188

Merged
merged 3 commits into from
Oct 7, 2016

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Sep 28, 2016

Fixes #156. In the discussion in that issue, @spicyj wrote:

If this is causing a problem in some Facebook project using fbjs, feel free to send a pull request.

I’ve seen a couple issues that I’m hoping can qualify, including:

facebook/react#427
facebookarchive/draft-js#527

I can also say that I am working with a team on an editor app that would benefit greatly from having React, Draft.js, and fbjs all work seamlessly across iframes. When building a sophisticated editor of some kind, the use case of rendering from a single parent browsing context into a separate nested browsing context, treating it as a sandboxed rendering target that can publish UI events, is a really compelling one.

About the PR: We used eslint’s no-undef rule with a config that didn’t include the browser env to get a list of all files that use browser globals directly. We then went through each instance and adapted them to support use across nested browsing contexts where necessary.

Here’s a list of the files that use browser globals that we didn’t modify, along with an explanation of why. The only file we didn’t change where it could actually matter is getViewportDimensions, so I’ll start with that one:

fbjs/src/core/dom/getViewportDimensions.js

Uses global document and window extensively. Could be easily refactored to take an optional document argument, like getDocumentScrollElement, but I also couldn’t find it being used anywhere in fbjs or in React, so didn’t know if it was worth it or desired. Please let me know if I should implement that change.

fbjs/src/__forks__/isEventSupported.js
fbjs/src/core/ExecutionEnvironment.js
fbjs/src/dom/translateDOMPositionXY.js

Feature (or UA) detection against the global window / document objects

fbjs/src/core/createNodesFromMarkup.js
fbjs/src/core/getMarkupWrap.js
fbjs/src/core/getVendorPrefixedName.js

Uses document.createElement(), no nested browsing context issues

fbjs/src/core/dom/getDocumentScrollElement.js

Uses global document as fallback default (just like the changes we made in this PR for getActiveElement.js)

fbjs/src/core/dom/getStyleProperty.js

Uses window.getComputedStyle, which seems to work fine across nested browsing contexts

There are also some polyfill / feature detections for fetch, requestAnimationFrame, Performance, ES2015 Map/Set that use globals, but I figured it wouldn’t be helpful to list those out.

@ghost ghost added the CLA Signed label Sep 28, 2016
@acusti
Copy link
Contributor Author

acusti commented Sep 28, 2016

I tried to add tests where there were any existing test files I could add to, which turned out to only be packages/fbjs/src/core/dom/getActiveElement.js and packages/fbjs/src/core/dom/isNode.js. The getActiveElement test case was easy, but the isNode one proved trickier. Here’s the test I wanted to add:

  it('should accept nodes from different document contexts', function() {
    var iframe = document.createElement('iframe');
    document.body.appendChild(iframe);
    try {
      expect(isNode(iframe.contentDocument.body)).toBe(true);
    } finally {
      document.body.removeChild(iframe);
    }
  });

However, I realized when verifying it that the assertions always passed, even without our changes (i.e. with isNode relying on the global Node constructor). This led to the realization that jsdom has only a single DOM implementation (set of DOM constructors) that it shares across any browsing contexts, which I documented in detail, so I don’t think there is currently a way to capture this behavior in a test case using jsdom, but instead would require an actual browser (and not Firefox, because it has surprising instanceof behavior across nested browsing contexts).

I also wasn’t sure about using flow. Tried to keep files in the style they were already written in, but I could of course add real flow annotations to, for example, packages/fbjs/src/core/dom/getActiveElement.js.

@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

I created a demo of a simple use case to illustrate the utility of this PR. I then duplicated the demo and have one using the current react and react-dom builds and one using built versions of those files made from the React branch I made that uses these changes with the changes in this branch npm linked into it.

Without the changes in this branch + the React PR: http://codepen.io/acusti/pen/kkBpWA?editors=0011
With the changes in this branch + the React PR: http://codepen.io/acusti/pen/rrpKYZ?editors=0011

@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

Here’s another version of the demo, but with a different implementation that illustrates the benefits of these changes slightly more effectively:

Without the changes in this branch + the React PR: http://codepen.io/acusti/pen/EgQxJG?editors=0011
With the changes in this branch + the React PR: http://codepen.io/acusti/pen/qaxBby?editors=0011

Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

I think this looks mostly good, thanks.

if (_isNodeScrollable(node, 'overflow') ||
_isNodeScrollable(node, 'overflowY') ||
_isNodeScrollable(node, 'overflowX')) {
return node;
}
node = node.parentNode;
}
return window;
return ownerDocument.defaultView;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do ownerDocument.defaultView || ownerDocument.parentWindow for IE8 compat?

@@ -19,7 +19,7 @@ const containsNode = require('containsNode');
* @return {object}
*/
function getElementRect(elem) {
const docElem = document.documentElement;
const docElem = (elem.ownerDocument || document).documentElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

When is it appropriate to fall back to document here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never, good point. WIll update

@acusti acusti force-pushed the iframe-compatibility branch 2 times, most recently from 44db018 to 2ea6a39 Compare October 6, 2016 21:35
@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

I updated with those changes, thanks @spicyj

@@ -15,8 +15,10 @@
* @return {boolean} Whether or not the object is a DOM node.
*/
function isNode(object) {
var doc = object ? (object.ownerDocument || object) : document;
var defaultView = doc.defaultView || window;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will throw if doc is null, won't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m thinking doc can’t be null. It definitely passes the isNode tests.

If object is null, doc is document
If object is not null but object.ownerDocument is null, doc becomes object, which wasn’t null
If object is not null and object.ownerDocument is not null, it won’t throw

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my mistake. I missed the || object. This seems good.

@acusti
Copy link
Contributor Author

acusti commented Oct 7, 2016

@spicyj Do you have an opinion about getViewportDimensions? Should I modify the get* functions to take an optional doc argument?

@sophiebits
Copy link
Contributor

Eh, let's leave it for now.

@sophiebits sophiebits merged commit 43b9b91 into facebook:master Oct 7, 2016
@conartist6
Copy link

Look at the commit and original rationale for #59. In getActiveElement.js. Either we should remove the check for undefined document or we should ensure that we do not reference document before we check to make sure its type is not undefined.

@sophiebits
Copy link
Contributor

@conartist6 Good catch. Do you mind sending a PR fixing this and a diff internally?

@conartist6
Copy link

Will do.

@conartist6
Copy link

I think the fix is actually slightly more complicated because this function's return type in flow (which I'm adding and is why I'm looking) should be DOMElement (not DOMElement?). Calling in doesn't make sense if you know there's no document.

The original puller's issue should be fixed in react then, which can be trivially modified to guard the calls with if (ExecutionEnvironment.canUseDOM) { since we already know that react considers the result to be nullable.

@sophiebits
Copy link
Contributor

I don't believe we call getActiveElement in React except when doing client-side rendering.

@acusti
Copy link
Contributor Author

acusti commented Oct 13, 2016

I don’t believe React ever calls getActiveElement outside of SelectEventPlugin and ReactInputSelection which are definitely only used in client-side rendering.

I only kept the if (typeof doc === 'undefined') { check because it was the pre-existing behavior and I didn’t know where else that utility is used. Seems like removing that and changing the return value to DOMElement is a good idea and should work fine.

@acusti
Copy link
Contributor Author

acusti commented Oct 18, 2016

@spicyj I have a PR (well, 2 of them, but they’re just alternative approaches to the same feature/fix) to React that depend on these changes: facebook/react#7936 and facebook/react#7866

Don’t know when this PR will be released, and I’m not sure how to coordinate this all. There’s also an updated version of jsdom that is now being required in master at jest, but there’s no new release available there either, and that would be nice to have in React for my PR because it would mean removing hacky jsdom workarounds from the tests I added. And then there’s the fact that those are all required for a PR I’m preparing to draft.js to make it independent of the global window / document objects (so it will work when rendered into an iframe), but I haven’t even opened that one yet.

Is there anything I can do to help with the coordination of all this? I’m particularly curious if there’s anything I can do to help with the React PR; one of my new tests is currently failing on Travis, but that’s because it depends on this PR, and I think it could still be worthwhile for someone to review it. Thanks for any info or recommendations you can provide! I truly and deeply appreciate the extraordinary effort and commitment you and the rest of the React ecosystem-related team at Facebook have been making for the open source stewardship of all these projects.

@sophiebits
Copy link
Contributor

We're just really behind and I haven't had time to look. I don't think there's anything else we need from you right now. Sorry for the wait.

@conartist6
Copy link

@acusti These changes are merged, what are you waiting on?

@acusti
Copy link
Contributor Author

acusti commented Oct 18, 2016

@conartist6 Can’t be installed until a new release of fbjs gets published to npm. In order to use it in our app, we needed to create a repo with just the built files of the fbjs package and an updated package.json and point to that repo in our package.json ("fbjs": "Brandcast/fbjs-built#97b8e54",).

@acusti acusti deleted the iframe-compatibility branch October 18, 2016 17:20
@acusti acusti restored the iframe-compatibility branch October 18, 2016 17:20
@conartist6
Copy link

Ah, ok.

acusti added a commit to brandcast/react that referenced this pull request Nov 10, 2016
acusti added a commit to brandcast/react that referenced this pull request Nov 21, 2016
acusti added a commit to brandcast/react that referenced this pull request Dec 2, 2016
acusti added a commit to brandcast/react that referenced this pull request Dec 2, 2016
acusti added a commit to brandcast/react that referenced this pull request Dec 2, 2016
acusti added a commit to brandcast/react that referenced this pull request Mar 15, 2017
acusti added a commit to brandcast/react that referenced this pull request Mar 15, 2017
acusti added a commit to brandcast/fbjs that referenced this pull request Mar 16, 2017
* [fbjs] Use relative document and window (facebook#156)

* [fbjs] Support nested browsing contexts (facebook#156)

* [fbjs] Test getActiveElement api change (facebook#156)
zpao pushed a commit that referenced this pull request Mar 21, 2017
* [fbjs] Use relative document and window (#156)

* [fbjs] Support nested browsing contexts (#156)

* [fbjs] Test getActiveElement api change (#156)
acusti added a commit to brandcast/react that referenced this pull request Apr 9, 2017
acusti added a commit to brandcast/react that referenced this pull request May 4, 2017
acusti added a commit to brandcast/react that referenced this pull request May 4, 2017
acusti added a commit to brandcast/react that referenced this pull request May 4, 2017
mihaip added a commit to quip/react that referenced this pull request Jun 29, 2017
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 added a commit to brandcast/react that referenced this pull request Jul 27, 2017
mihaip added a commit to quip/react that referenced this pull request Sep 29, 2017
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)
@ExE-Boss
Copy link

With jsdom/jsdom#2691 merged and jsdom/jsdom#1453 finally fixed, the isNode(…) test can finally be implemented:

it('should accept nodes from different document contexts', () => {
	const iframe = document.createElement('iframe');
	document.body.appendChild(iframe);
	try {
		expect(isNode(iframe.contentDocument.body)).toBe(true);
	} finally {
		document.body.removeChild(iframe);
	}
});

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.

Using document and window global variables won't cover all cases
6 participants