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

Wrong DOM dimensions after call to mount #1434

Closed
pronebird opened this issue Dec 15, 2017 · 8 comments
Closed

Wrong DOM dimensions after call to mount #1434

pronebird opened this issue Dec 15, 2017 · 8 comments

Comments

@pronebird
Copy link

DOM dimensions are invalid after call to mount():

const test = mount(
  <div style={{ height: 100, overflow: 'hidden' }}></div>
);
expect(test.getDOMNode().clientHeight).to.be.equal(100); // reports 0 != 100

Running the same in browser works:

<html>
<body>
  <div style="height: 100px; overflow: hidden"></div>
  <script>alert(document.querySelector('div').clientHeight);</script>
</body>
</html>

Adding timeout after mount doesn't help either. It seems that enzyme is broken. I use electron-mocha to run tests in native environment.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2017

It's not that enzyme is broken; getDOMNode() is returning you a DOM node. The issue is that the DOM node itself is reporting a height of zero, probably because it's not actually visible. clientHeight doesn't test the style, it tests the actual height.

This looks like a minimal test case - can you provide your actual code, so we can see what you're trying to test? Generally you shouldn't need the DOM node.

@michaelpapworth
Copy link

michaelpapworth commented Jan 5, 2018

I'm having a similar issue myself where clientHeight returns undefined instead of the actual height of the DOMElement. This code works as expected in the browser.

Component

class MyWidget extends React.Component {
    constructor() {
        super();
        this.state = { contentHeight: 0 };
    }

    componentWillReceiveProps(nextProps) {
        this.setState(() => ({
            contentHeight: nextProps.open ? this.content.firstChild.clientHeight : 0
        }));
    }

    render() {
        const { controlId, open, trigger, children } = this.props;
        return (
            <div>
                <button tabIndex="0" aria-controls={controlId}>
                    { cloneElement(trigger, { open }) }
                </button>
                <div
                    id={controlId}
                    ref={(element) => {
                        this.content = element;
                    }}
                    aria-hidden={open}
                    style={{ maxHeight: this.state.contentHeight }}
                >
                    { cloneElement(children, { open }) }
                </div>
            </div>
        );
    }
}

However, when mounting the component and simulating a prop change, the clientHeight of the child div reports undefined.

Unit Test

// Arrange
const wrapper = mount(
    <MyWidget trigger={<span>click me</span>} open={false}>
        <div style={{ height: 90, padding: 5 }}>Lorum Ipsum</div>
    </MyWidget>
);

// Act
wrapper.setProps({ open: true });

// Assert
expect(wrapper.children('div').prop('style')).to.have.property('maxHeight', 100);

// Result
AssertionError: expected { maxHeight: undefined } to have a property 'maxHeight' of 100, but got undefined

I'm not entirely convinced that enzyme is the cause of this since it feels like its related to the underlying tooling or even JSDOM, but I suspect you've far more experience than I with react-dom/test-tools and may be able to target an issue in the appropriate direction. For additional context I was led here from facebook/react#11855

@ljharb
Copy link
Member

ljharb commented Jan 5, 2018

@michaelpapworth I agree that it's likely related to jsdom. Have you tried running enzyme in an actual browser to confirm?

@pronebird
Copy link
Author

pronebird commented Jan 5, 2018

Do you use JSDOM in enzyme? We had tons of problems with it before, so we switched to running the actual browser with electron-mocha.

My mocha test setup looks as following:

let container: ?HTMLElement;

function renderIntoDocument(instance) {
  if(!container) {
    container = document.createElement('div');
    if(!document.documentElement) {
      throw new Error('document.documentElement cannot be null.');
    }
    document.documentElement.appendChild(container);
  }
  return ReactDOM.render(instance, container);
}

// unmount container and clean up DOM
afterEach(() => {
  if(container) {
    ReactDOM.unmountComponentAtNode(container);
    container = null;
  }
});

it('should report the correct clientHeight', () => {
  const component = renderIntoDocument(
    <MyMegaComponent>
      <div style={{ height: 100 }}></div>
    </MyMegaComponent>
   );
   const domNode = ReactDOM.findDOMNode(component);
   expect(domNode).to.have.property('clientHeight', 100);
});

clientHeight is then returns the correct values in the actual test cases. Apparently can't use enzyme with any of this.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2018

Yes, we use jsdom with enzyme and it works fine. However, we don't test for things like clientHeight, because it's not the responsibility of component unit tests to ensure the browser, or React is working.

In other words, I'm suggesting that's not a good or useful test.

@pronebird
Copy link
Author

pronebird commented Jan 7, 2018

In my case I test the visual component that sets the proper visual presentation.

The component itself uses clientHeight to find the height of the underlying DOM. Given that clientHeight is undefined the internal state is corrupted.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2018

enzyme isn’t an appropriate visual testing tool; I’d recommend something like happo for that (for visual diffs).

Separately, all you need to test there is that you rendered a div with a style height of 100; the clientHeight is guaranteed there because of how React and the DOM work - so all you’d need to do is assert on the style.

@ljharb ljharb closed this as completed Jul 7, 2018
@pronebird
Copy link
Author

@ljharb makes sense! Thank you! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants