Skip to content

Commit

Permalink
[fixed] Don't try to access .ownerDocument on null
Browse files Browse the repository at this point in the history
We previously did not take into account that React.findDOMNode(this)
can return null, (for example if you have a modal and only want to
render the overlay). We now fallback to the root document in that
case.

ee0382e
  • Loading branch information
martijnvermaat committed Apr 6, 2015
1 parent a58cff9 commit 7175431
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/AffixMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ const AffixMixin = {
this._onWindowScrollListener =
EventListener.listen(window, 'scroll', this.checkPosition);
this._onDocumentClickListener =
EventListener.listen(React.findDOMNode(this).ownerDocument, 'click', this.checkPositionWithEventLoop);
EventListener.listen(domUtils.ownerDocument(this), 'click', this.checkPositionWithEventLoop);
},

componentWillUnmount() {
Expand Down
3 changes: 2 additions & 1 deletion src/DropdownStateMixin.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react';
import domUtils from './utils/domUtils';
import EventListener from './utils/EventListener';

/**
Expand Down Expand Up @@ -56,7 +57,7 @@ const DropdownStateMixin = {
},

bindRootCloseHandlers() {
let doc = React.findDOMNode(this).ownerDocument;
let doc = domUtils.ownerDocument(this);

this._onDocumentClickListener =
EventListener.listen(doc, 'click', this.handleDocumentClick);
Expand Down
4 changes: 3 additions & 1 deletion src/FadeMixin.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react';
import domUtils from './utils/domUtils';

// TODO: listen for onTransitionEnd to remove el
function getElementsAndSelf (root, classes){
Expand Down Expand Up @@ -57,7 +58,8 @@ export default {

componentWillUnmount: function () {
let els = getElementsAndSelf(React.findDOMNode(this), ['fade']),
container = (this.props.container && React.findDOMNode(this.props.container)) || React.findDOMNode(this).ownerDocument.body;
container = (this.props.container && React.findDOMNode(this.props.container)) ||
domUtils.ownerDocument(this).body;

if (els.length) {
this._fadeOutEl = document.createElement('div');
Expand Down
9 changes: 6 additions & 3 deletions src/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import classSet from 'classnames';
import BootstrapMixin from './BootstrapMixin';
import FadeMixin from './FadeMixin';
import domUtils from './utils/domUtils';
import EventListener from './utils/EventListener';


Expand Down Expand Up @@ -128,9 +129,10 @@ const Modal = React.createClass({

componentDidMount() {
this._onDocumentKeyupListener =
EventListener.listen(React.findDOMNode(this).ownerDocument, 'keyup', this.handleDocumentKeyUp);
EventListener.listen(domUtils.ownerDocument(this), 'keyup', this.handleDocumentKeyUp);

let container = (this.props.container && React.findDOMNode(this.props.container)) || React.findDOMNode(this).ownerDocument.body;
let container = (this.props.container && React.findDOMNode(this.props.container)) ||
domUtils.ownerDocument(this).body;
container.className += container.className.length ? ' modal-open' : 'modal-open';

if (this.props.backdrop) {
Expand All @@ -146,7 +148,8 @@ const Modal = React.createClass({

componentWillUnmount() {
this._onDocumentKeyupListener.remove();
let container = (this.props.container && React.findDOMNode(this.props.container)) || React.findDOMNode(this).ownerDocument.body;
let container = (this.props.container && React.findDOMNode(this.props.container)) ||
domUtils.ownerDocument(this).body;
container.className = container.className.replace(/ ?modal-open/, '');
},

Expand Down
3 changes: 2 additions & 1 deletion src/OverlayMixin.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import CustomPropTypes from './utils/CustomPropTypes';
import domUtils from './utils/domUtils';

export default {
propTypes: {
Expand Down Expand Up @@ -63,6 +64,6 @@ export default {
},

getContainerDOMNode() {
return React.findDOMNode(this.props.container || React.findDOMNode(this).ownerDocument.body);
return React.findDOMNode(this.props.container) || domUtils.ownerDocument(this).body;
}
};
20 changes: 17 additions & 3 deletions src/utils/domUtils.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,24 @@
import React from 'react';

/**
* Get elements owner document
*
* @param {ReactComponent|HTMLElement} componentOrElement
* @returns {HTMLElement}
*/
function ownerDocument(componentOrElement) {
let elem = React.findDOMNode(componentOrElement);
return (elem && elem.ownerDocument) || document;
}

/**
* Shortcut to compute element style
*
* @param {HTMLElement} elem
* @returns {CssStyle}
*/
function getComputedStyles(elem) {
return elem.ownerDocument.defaultView.getComputedStyle(elem, null);
return ownerDocument(elem).defaultView.getComputedStyle(elem, null);
}

/**
Expand All @@ -21,7 +34,7 @@ function getOffset(DOMNode) {
return window.jQuery(DOMNode).offset();
}

let docElem = DOMNode.ownerDocument.documentElement;
let docElem = ownerDocument(DOMNode).documentElement;
let box = { top: 0, left: 0 };

// If we don't have gBCR, just use 0,0 rather than error
Expand Down Expand Up @@ -89,7 +102,7 @@ function getPosition(elem, offsetParent) {
* @returns {HTMLElement}
*/
function offsetParentFunc(elem) {
let docElem = elem.ownerDocument.documentElement;
let docElem = ownerDocument(elem).documentElement;
let offsetParent = elem.offsetParent || docElem;

while ( offsetParent && ( offsetParent.nodeName !== 'HTML' &&
Expand All @@ -101,6 +114,7 @@ function offsetParentFunc(elem) {
}

export default {
ownerDocument: ownerDocument,
getComputedStyles: getComputedStyles,
getOffset: getOffset,
getPosition: getPosition,
Expand Down
20 changes: 20 additions & 0 deletions test/OverlayMixinSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,24 @@ describe('OverlayMixin', function () {

assert.equal(instance.refs.overlay.getOverlayDOMNode(), null);
});

it('Should render only an overlay', function() {
let OnlyOverlay = React.createClass({
mixins: [OverlayMixin],

render: function() {
return null;
},

renderOverlay: function() {
return this.props.overlay;
}
});

let overlayInstance = ReactTestUtils.renderIntoDocument(
<OnlyOverlay overlay={<div id="test1" />} />
);

assert.equal(overlayInstance.getOverlayDOMNode().nodeName, 'DIV');
});
});

0 comments on commit 7175431

Please sign in to comment.