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

Use ownerDocument instead of global document #2721

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@yamachig
Copy link
Contributor

@yamachig yamachig commented Aug 18, 2019

It seems that on Microsoft Edge, you cannot appendChild an Element from different Document.
This causes a problem, for example, when you are using StyleSheetManager with a new window created by window.open, like this code:

import React from 'react';
import { createPortal } from 'react-dom';
import styled, {StyleSheetManager} from 'styled-components';

export const NewWindow = props => {
  const [newWindowBody, setNewWindowBody] = React.useState(null);
  const unloaded = React.useRef(false);

  React.useEffect(() => {
    if (!unloaded.current && !newWindowBody) {
      const w = window.open("about:blank");

      w.addEventListener('load', () => {
        setNewWindowBody(w.document.body);
      });

      w.addEventListener('unload', () => {
        unloaded.current = true;
        setNewWindowBody(null);
      });

      setNewWindowBody(w.document.body);
    }
  });

  return newWindowBody && createPortal(
    <StyleSheetManager target={newWindowBody}>
      {props.children}
    </StyleSheetManager>,
    newWindowBody,
  );
}

const Button = styled.button`
  border: none;
  background: palevioletred;
  color: white;
`;

export const TestNewWindow = () => {
  return (
    <NewWindow>
      <Button>
        Hello from new window!
      </Button>
    </NewWindow>
  )
}

This code throws an error on Edge, but not on Chrome or Firefox. (I tested with Microsoft Edge 44.18362.267.0.)

I replaced the global document variables with dynamically created Document from ownerDocument so that the code works properly on Edge.

probablyup
probablyup previously approved these changes Aug 21, 2019
Copy link
Contributor

@probablyup probablyup left a comment

This looks good to me!

@probablyup
Copy link
Contributor

@probablyup probablyup commented Aug 21, 2019

@yamachig could you add a changelog entry and if possible also submit the same PR against canary?

Thanks!

@yamachig
Copy link
Contributor Author

@yamachig yamachig commented Aug 22, 2019

@probablyup thank you for reviewing! OK, I added a changelog entry and submitted the same PR #2726 against canary.

probablyup
probablyup previously approved these changes Aug 30, 2019
Copy link
Contributor

@probablyup probablyup left a comment

Thanks!

Jessidhia
Jessidhia previously approved these changes Sep 6, 2019
const el = document.createElement('style');
let targetDocument = document;
if(target) targetDocument = target.ownerDocument;
else if(tagEl) targetDocument = tagEl.ownerDocument;
Copy link
Member

@Jessidhia Jessidhia Sep 6, 2019

Choose a reason for hiding this comment

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

I'd write this as:

const targetDocument = target ? target.ownerDocument : tagEl ? tagEl.ownerDocument : document

but that's just my anti-mutability bias 🤷‍♀

Copy link
Contributor Author

@yamachig yamachig Sep 8, 2019

Choose a reason for hiding this comment

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

@Jessidhia thank you for reviewing! I agree with anti-mutability, though, the eslint rule (no-nested-ternary) didn't let me do so :)

if(target) targetDocument = target.ownerDocument;
else if(tagEl) targetDocument = tagEl.ownerDocument;

const el = targetDocument.createElement('style');
Copy link
Member

@Jessidhia Jessidhia Sep 6, 2019

Choose a reason for hiding this comment

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

Interesting that this causes no security issues; apparently any <style> tags' associated StyleSheet are considered same-origin. https://html.spec.whatwg.org/multipage/semantics.html#the-style-element:concept-css-style-sheet-origin-clean-flag

This can still break for CSP reasons, though, but it'd break regardless of whether we use CSSOM or child text nodes.

@Jessidhia
Copy link
Member

@Jessidhia Jessidhia commented Sep 6, 2019

Looks like the travis config / vm is way broken.

@probablyup
Copy link
Contributor

@probablyup probablyup commented Sep 9, 2019

@yamachig rebase when you get a chance

@yamachig yamachig dismissed stale reviews from Jessidhia and probablyup via daea21a Sep 9, 2019
@yamachig yamachig force-pushed the use-ownerDocument branch from fa460b8 to daea21a Sep 9, 2019
@yamachig
Copy link
Contributor Author

@yamachig yamachig commented Sep 9, 2019

@probablyup OK, I rebased. Is that correct?

@probablyup
Copy link
Contributor

@probablyup probablyup commented Sep 9, 2019

Yeah looks like CI is passing now, good.

@probablyup probablyup merged commit f3e2266 into styled-components:master Sep 9, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants