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

[core] Remove dom-helpers dependency #14877

Merged
merged 2 commits into from Mar 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/material-ui/package.json
Expand Up @@ -45,7 +45,6 @@
"csstype": "^2.5.2",
"debounce": "^1.1.0",
"deepmerge": "^3.0.0",
"dom-helpers": "^3.2.1",
"hoist-non-react-statics": "^3.2.1",
"is-plain-object": "^2.0.4",
"normalize-scroll-left": "^0.1.2",
Expand Down
8 changes: 4 additions & 4 deletions packages/material-ui/src/Menu/Menu.js
Expand Up @@ -3,7 +3,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import getScrollbarSize from 'dom-helpers/util/scrollbarSize';
import getScrollbarSize from '../utils/getScrollbarSize';
import withStyles from '../styles/withStyles';
import withForwardedRef from '../utils/withForwardedRef';
import Popover from '../Popover';
Expand Down Expand Up @@ -74,9 +74,9 @@ class Menu extends React.Component {
// Let's ignore that piece of logic if users are already overriding the width
// of the menu.
if (menuList && element.clientHeight < menuList.clientHeight && !menuList.style.width) {
const size = `${getScrollbarSize(true)}px`;
menuList.style[theme.direction === 'rtl' ? 'paddingLeft' : 'paddingRight'] = size;
menuList.style.width = `calc(100% + ${size})`;
const scrollbarSize = `${getScrollbarSize()}px`;
menuList.style[theme.direction === 'rtl' ? 'paddingLeft' : 'paddingRight'] = scrollbarSize;
menuList.style.width = `calc(100% + ${scrollbarSize})`;
}

if (this.props.onEntering) {
Expand Down
7 changes: 3 additions & 4 deletions packages/material-ui/src/Modal/Modal.js
Expand Up @@ -13,9 +13,9 @@ import TrapFocus from './TrapFocus';
import Backdrop from '../Backdrop';
import { ariaHidden } from './manageAriaHidden';

function getContainer(container, defaultContainer) {
function getContainer(container) {
container = typeof container === 'function' ? container() : container;
return ReactDOM.findDOMNode(container) || defaultContainer;
return ReactDOM.findDOMNode(container);
}

function getHasTransition(props) {
Expand Down Expand Up @@ -97,8 +97,7 @@ class Modal extends React.Component {
}

handleOpen = () => {
const doc = this.getDoc();
const container = getContainer(this.props.container, doc.body);
const container = getContainer(this.props.container) || this.getDoc().body;

this.props.manager.add(this, container);
if (this.modalRef) {
Expand Down
9 changes: 5 additions & 4 deletions packages/material-ui/src/Modal/ModalManager.js
@@ -1,5 +1,5 @@
import css from 'dom-helpers/style';
import getScrollbarSize from 'dom-helpers/util/scrollbarSize';
import ownerWindow from '../utils/ownerWindow';
import getScrollbarSize from '../utils/getScrollbarSize';
import ownerDocument from '../utils/ownerDocument';
import isOverflowing from './isOverflowing';
import { ariaHidden, ariaHiddenSiblings } from './manageAriaHidden';
Expand All @@ -17,7 +17,8 @@ function findIndexOf(data, callback) {
}

function getPaddingRight(node) {
return parseInt(css(node, 'paddingRight') || 0, 10);
const win = ownerWindow(node);
return parseInt(win.getComputedStyle(node)['padding-right'], 10) || 0;
}

function setContainerStyle(data) {
Expand All @@ -32,7 +33,7 @@ function setContainerStyle(data) {
};

if (data.overflowing) {
const scrollbarSize = getScrollbarSize(true);
const scrollbarSize = getScrollbarSize();

// Use computed style, here to get the real padding to add our scrollbar width.
style.paddingRight = `${getPaddingRight(data.container) + scrollbarSize}px`;
Expand Down
24 changes: 20 additions & 4 deletions packages/material-ui/src/Modal/ModalManager.test.js
@@ -1,5 +1,5 @@
import { assert } from 'chai';
import getScrollbarSize from 'dom-helpers/util/scrollbarSize';
import getScrollbarSize from '../utils/getScrollbarSize';
import ModalManager from './ModalManager';

describe('ModalManager', () => {
Expand All @@ -9,6 +9,15 @@ describe('ModalManager', () => {
before(() => {
modalManager = new ModalManager();
container1 = document.createElement('div');
container1.style.padding = '20px';
Object.defineProperty(container1, 'scrollHeight', {
value: 100,
writable: false,
Copy link
Member

Choose a reason for hiding this comment

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

This will not really prevent writing. All it does is throw an error. I would rather sinon.stub this with a fake setter:

sinon.stub(container1, 'scrollHeight').get(() => 100).set(() => {})

writable: false will actually not trigger an error. Would've been nice if sinon would support this out of the box.

});
Object.defineProperty(container1, 'clientHeight', {
value: 90,
writable: false,
});
document.body.appendChild(container1);
});

Expand Down Expand Up @@ -113,15 +122,22 @@ describe('ModalManager', () => {
it('should handle the scroll', () => {
const modal = {};
const paddingRightBefore = container1.style.paddingRight;
const paddingFixedRightBefore = fixedNode.style.paddingRight;
Copy link
Member

Choose a reason for hiding this comment

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

Is this just refactoring or did the test actually break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both.

Copy link
Member

Choose a reason for hiding this comment

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

So this is a breaking change after all or only breaking in jsdom?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it's only breaking jsdom.

modalManager.add(modal, container1);
modalManager.mount(modal);
assert.strictEqual(container1.style.overflow, 'hidden');
assert.strictEqual(container1.style.paddingRight, `${getScrollbarSize()}px`);
assert.strictEqual(fixedNode.style.paddingRight, `${14 + getScrollbarSize()}px`);
assert.strictEqual(
container1.style.paddingRight,
`${parseInt(paddingRightBefore, 10) + getScrollbarSize()}px`,
);
assert.strictEqual(
fixedNode.style.paddingRight,
`${parseInt(paddingFixedRightBefore, 10) + getScrollbarSize()}px`,
);
modalManager.remove(modal);
assert.strictEqual(container1.style.overflow, '');
assert.strictEqual(container1.style.paddingRight, paddingRightBefore);
assert.strictEqual(fixedNode.style.paddingRight, '14px');
assert.strictEqual(fixedNode.style.paddingRight, paddingFixedRightBefore);
});
});

Expand Down
15 changes: 4 additions & 11 deletions packages/material-ui/src/Modal/isOverflowing.js
@@ -1,25 +1,18 @@
import isWindow from 'dom-helpers/query/isWindow';
import ownerDocument from '../utils/ownerDocument';
import ownerWindow from '../utils/ownerWindow';

export function isBody(node) {
return node && node.tagName.toLowerCase() === 'body';
}

// Do we have a vertical scroll bar?
// Do we have a vertical scrollbar?
export default function isOverflowing(container) {
const doc = ownerDocument(container);
const win = ownerWindow(doc);

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

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

return marginLeft + doc.body.clientWidth + marginRight < win.innerWidth;
return container.scrollHeight > container.clientHeight;
Copy link
Member Author

Choose a reason for hiding this comment

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

}
7 changes: 6 additions & 1 deletion packages/material-ui/src/Popover/Popover.test.js
@@ -1,7 +1,6 @@
import React from 'react';
import { assert } from 'chai';
import { spy, stub, useFakeTimers } from 'sinon';
import css from 'dom-helpers/style';
import {
createShallow,
createMount,
Expand Down Expand Up @@ -315,6 +314,12 @@ describe('<Popover />', () => {
anchorEl = window.document.createElement('div');
}

const css = (element, styles) => {
Object.keys(styles).forEach(key => {
element.style[key] = styles[key];
});
};

css(anchorEl, {
width: '50px',
height: '50px',
Expand Down
8 changes: 4 additions & 4 deletions packages/material-ui/src/Tabs/ScrollbarSize.js
Expand Up @@ -4,12 +4,12 @@ import EventListener from 'react-event-listener';
import debounce from 'debounce'; // < 1kb payload overhead when lodash/debounce is > 3kb.

const styles = {
width: 90,
height: 90,
width: 99,
height: 99,
position: 'absolute',
top: -9000,
top: -9999,
overflow: 'scroll',
// Support IE 11
// TODO Do we need this style for IE 11 support?
msOverflowStyle: 'scrollbar',
};

Expand Down
18 changes: 18 additions & 0 deletions packages/material-ui/src/utils/getScrollbarSize.js
@@ -0,0 +1,18 @@
// A change of the browser zoom change the scrollbar size.
// Credit https://github.com/twbs/bootstrap/blob/3ffe3a5d82f6f561b82ff78d82b32a7d14aed558/js/src/modal.js#L512-L519
function getScrollbarSize() {
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
const scrollDiv = document.createElement('div');
scrollDiv.style.width = '99px';
scrollDiv.style.height = '99px';
scrollDiv.style.position = 'absolute';
scrollDiv.style.top = '-9999px';
scrollDiv.style.overflow = 'scroll';

document.body.appendChild(scrollDiv);
const scrollbarSize = scrollDiv.offsetWidth - scrollDiv.clientWidth;
document.body.removeChild(scrollDiv);

return scrollbarSize;
}

export default getScrollbarSize;