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.

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

[core] Remove direct references to window/document objects #10328

Merged
merged 6 commits into from
Feb 17, 2018
Merged

[core] Remove direct references to window/document objects #10328

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c72147
Remove remaining direct references to window/document objects
ianschmitz Feb 16, 2018
368b65d
remove getOwnerDocument
oliviertassinari Feb 17, 2018
5bfbb8e
Fix withWidth trying to get ownerWindow from EventListener instance
ianschmitz Feb 17, 2018
68b3745
Merge branch 'v1-beta' into document-refs
oliviertassinari Feb 17, 2018
5cd6ef0
Change ReactDOM import for consistency
ianschmitz Feb 17, 2018
9cfc8d6
Revert withWidth changes
ianschmitz Feb 17, 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
12 changes: 4 additions & 8 deletions src/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ function getContainer(container, defaultContainer) {
return ReactDOM.findDOMNode(container) || defaultContainer;
}

function getOwnerDocument(element) {
return ownerDocument(ReactDOM.findDOMNode(element));
}

function getHasTransition(props) {
return props.children ? props.children.props.hasOwnProperty('in') : false;
}
Expand Down Expand Up @@ -110,12 +106,12 @@ class Modal extends React.Component {
};

handleOpen = () => {
const doc = getOwnerDocument(this);
const doc = ownerDocument(this.mountNode);
const container = getContainer(this.props.container, doc.body);

this.props.manager.add(this, container);
this.onDocumentKeydownListener = addEventListener(doc, 'keydown', this.handleDocumentKeyDown);
this.onFocusinListener = addEventListener(document, 'focus', this.enforceFocus, true);
this.onFocusinListener = addEventListener(doc, 'focus', this.enforceFocus, true);
};

handleClose = () => {
Expand Down Expand Up @@ -170,7 +166,7 @@ class Modal extends React.Component {
}

const dialogElement = this.getDialogElement();
const currentActiveElement = activeElement(getOwnerDocument(this));
const currentActiveElement = activeElement(ownerDocument(this.mountNode));

if (dialogElement && !contains(dialogElement, currentActiveElement)) {
this.lastFocus = currentActiveElement;
Expand Down Expand Up @@ -208,7 +204,7 @@ class Modal extends React.Component {
}

const dialogElement = this.getDialogElement();
const currentActiveElement = activeElement(getOwnerDocument(this));
const currentActiveElement = activeElement(ownerDocument(this.mountNode));

if (dialogElement && !contains(dialogElement, currentActiveElement)) {
dialogElement.focus();
Expand Down
5 changes: 3 additions & 2 deletions src/Modal/ModalManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import css from 'dom-helpers/style';
import ownerDocument from 'dom-helpers/ownerDocument';
import getScrollbarSize from 'dom-helpers/util/scrollbarSize';
import isOverflowing from './isOverflowing';
import { ariaHidden, hideSiblings, showSiblings } from './manageAriaHidden';
Expand Down Expand Up @@ -39,7 +40,7 @@ function setContainerStyle(data, container) {
style.paddingRight = `${getPaddingRight(container) + scrollbarSize}px`;

// .mui-fixed is a global helper.
const fixedNodes = document.querySelectorAll('.mui-fixed');
const fixedNodes = ownerDocument(container).querySelectorAll('.mui-fixed');
for (let i = 0; i < fixedNodes.length; i += 1) {
const paddingRight = getPaddingRight(fixedNodes[i]);
data.prevPaddings.push(paddingRight);
Expand All @@ -57,7 +58,7 @@ function removeContainerStyle(data, container) {
container.style[key] = data.style[key];
});

const fixedNodes = document.querySelectorAll('.mui-fixed');
const fixedNodes = ownerDocument(container).querySelectorAll('.mui-fixed');
for (let i = 0; i < fixedNodes.length; i += 1) {
fixedNodes[i].style.paddingRight = `${data.prevPaddings[i]}px`;
}
Expand Down
7 changes: 4 additions & 3 deletions src/Modal/isOverflowing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isWindow from 'dom-helpers/query/isWindow';
import ownerDocument from 'dom-helpers/ownerDocument';
import ownerWindow from 'dom-helpers/ownerWindow';

export function isBody(node) {
return node && node.tagName.toLowerCase() === 'body';
Expand All @@ -8,15 +9,15 @@ export function isBody(node) {
// Do we have a scroll bar?
export default function isOverflowing(container) {
const doc = ownerDocument(container);
const win = isWindow(doc);
const win = ownerWindow(doc);

/* istanbul ignore next */
if (!win && !isBody(container)) {
if (!isWindow(doc) && !isBody(container)) {
return container.scrollHeight > container.clientHeight;
}

// Takes in account potential non zero margin on the body.
const style = window.getComputedStyle(doc.body);
const style = win.getComputedStyle(doc.body);
const marginLeft = parseInt(style.getPropertyValue('margin-left'), 10);
const marginRight = parseInt(style.getPropertyValue('margin-right'), 10);

Expand Down
3 changes: 2 additions & 1 deletion src/Popover/Popover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ class Popover extends React.Component {
return anchorPosition;
}

const anchorElement = anchorEl || document.body;
// If an anchor element wasn't provided, just use the parent body element of this Popover
const anchorElement = anchorEl || ownerDocument(ReactDOM.findDOMNode(this.transitionEl)).body;
const anchorRect = anchorElement.getBoundingClientRect();
const anchorVertical = contentAnchorOffset === 0 ? anchorOrigin.vertical : 'center';

Expand Down
3 changes: 2 additions & 1 deletion src/transitions/Slide.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { findDOMNode } from 'react-dom';
import EventListener from 'react-event-listener';
import debounce from 'lodash/debounce';
import Transition from 'react-transition-group/Transition';
import ownerWindow from 'dom-helpers/ownerWindow';
import withTheme from '../styles/withTheme';
import { duration } from '../styles/transitions';
import { reflow, getTransitionProps } from './utils';
Expand All @@ -24,7 +25,7 @@ function getTranslateValue(props, node) {
if (node.fakeTransform) {
transform = node.fakeTransform;
} else {
const computedStyle = window.getComputedStyle(node);
const computedStyle = ownerWindow(node).getComputedStyle(node);
transform =
computedStyle.getPropertyValue('-webkit-transform') ||
computedStyle.getPropertyValue('transform');
Expand Down
6 changes: 4 additions & 2 deletions src/utils/ClickAwayListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import { findDOMNode } from 'react-dom';
import EventListener from 'react-event-listener';
import ownerDocument from 'dom-helpers/ownerDocument';

const isDescendant = (el, target) => {
if (target !== null && target.parentNode) {
Expand Down Expand Up @@ -33,10 +34,11 @@ class ClickAwayListener extends React.Component {
// IE11 support, which trigger the handleClickAway even after the unbind
if (this.mounted) {
const el = findDOMNode(this);
const doc = ownerDocument(el);

if (
document.documentElement &&
document.documentElement.contains(event.target) &&
doc.documentElement &&
doc.documentElement.contains(event.target) &&
!isDescendant(el, event.target)
) {
this.props.onClickAway(event);
Expand Down
18 changes: 15 additions & 3 deletions src/utils/withWidth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import EventListener from 'react-event-listener';
import ownerWindow from 'dom-helpers/ownerWindow';
import debounce from 'lodash/debounce';
import wrapDisplayName from 'recompose/wrapDisplayName';
import hoistNonReactStatics from 'hoist-non-react-statics';
Expand Down Expand Up @@ -35,15 +37,19 @@ const withWidth = (options = {}) => Component => {
};

componentDidMount() {
this.updateWidth(window.innerWidth);
const win = ownerWindow(ReactDOM.findDOMNode(this.mountNode));
this.updateWidth(win.innerWidth);
}

componentWillUnmount() {
this.handleResize.cancel();
}

mountNode = null;

handleResize = debounce(() => {
this.updateWidth(window.innerWidth);
const win = ownerWindow(ReactDOM.findDOMNode(this.mountNode));
Copy link
Contributor Author

@ianschmitz ianschmitz Feb 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This appears to work better. I'm still not 100% confident in its function.

findDOMNode(this.mountNode) always returns null from componentDidMount when a width or initialWidth prop isn't provided (such as in the docs), since we render null when width isn't defined yet in props or state.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. The logic is broken. I'm surprised the CI is green.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can leave the withWidth file unchanged so we can merge the other changes right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. When findDOMNode returns null the ownerWindow function just defaults to returning the global window object.

this.updateWidth(win.innerWidth);
}, resizeInterval);

updateWidth(innerWidth) {
Expand Down Expand Up @@ -102,7 +108,13 @@ const withWidth = (options = {}) => Component => {
}

return (
<EventListener target="window" onResize={this.handleResize}>
<EventListener
target="window"
onResize={this.handleResize}
ref={node => {
this.mountNode = 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.

I'm second guessing this. Maybe i'm just tired. Let me double check that this works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.mountNode will be a reference to the EventListener instance. I am then doing ownerDocument(this.mountNode) which is wrong since EventListener will never have an ownerDocument property on it.

}}
>
<Component {...more} {...props} />
</EventListener>
);
Expand Down