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

TestUtils.renderIntoDocument doesn't function as expected in all browsers #3789

Closed
ezequiel opened this issue Apr 30, 2015 · 11 comments
Closed
Labels

Comments

@ezequiel
Copy link

Hi,

Here is my program (jsfiddle):

var Input = React.createClass({
    render: function() {
        return (
            <input 
                {...this.props}
            />
        );
    }
});

var TestUtils = React.addons.TestUtils;
var inputInstance = 
        TestUtils.renderIntoDocument(
            <Input
                value='abc'
            />
        );

var inputDOMNode = React.findDOMNode(inputInstance);

inputDOMNode.focus();

// Place cursor at the end.
inputDOMNode.setSelectionRange(3, 3);

// Should log 3 twice.
console.log(inputDOMNode.selectionStart);
console.log(inputDOMNode.selectionEnd);

Problem: 0 is logged twice in Chrome and Firefox.
What I expected: 3 is logged twice. We see this in Safari.

The interesting part is if we use React.render(..., document.body) instead of TestUtils.renderIntoDocument the program works as expected.

I am using:

  • Chrome 42.0.2311.135
  • Firefox 37.0.2
  • Safari 8.0.5 (10600.5.17)
@ezequiel
Copy link
Author

I get it now.

setSelectionRange doesn't seem to work if the element isn't on the page. That's why rendering into document.body works as expected. I suppose it's a fluke the program ran correctly in Safari... Though that requirement isn't documented anywhere (?).

Appending to the document was commented out in this commit: ce95c3d. I guess the bigger issue is this method is a misnomer, as it doesn't actually render into the document. It certainly fooled me.

@jimfb
Copy link
Contributor

jimfb commented May 1, 2015

@spicyj @zpao does this change our thoughts on renderIntoDocument? Maybe we should attach to the dom to avoid cases like this? What cleanup were we trying to avoid when we removed that logic? Might the better solution be to improve test isolation?

@sophiebits
Copy link
Contributor

This will help avoid some cleanup effort between tests in the open-source version of React (see calls to removeNextSiblings in react/src/test/index.html).

I don't feel strongly.

@ezequiel
Copy link
Author

ezequiel commented May 2, 2015

I suppose we have two options:

  • Deprecate TestUtils.renderIntoDocument.
  • Create a new method, TestUtils.render (does anyone have a better name?) which is simply a renamed version of renderIntoDocument.

or,

  • Change TestUtils.renderIntoDocument to actually render into the document.

Option 1 seems like the best option at this point, as renderIntoDocument's current behavior hasn't received any complaints (besides mine) thus far.

edpaget pushed a commit to zooniverse/markdownz that referenced this issue Aug 4, 2015
From facebook/react#3789 in Webkit and
Psuedo-Webkit browsers calling setSelectionRange on an input not
currently in the document is fine, but breaks in Firefox and IE with a
rather nasty error.
@rupurt
Copy link

rupurt commented Nov 6, 2015

@ezequiel how do you feel about the following

  • Deprecate ReactTestUtils.renderIntoDocument
  • Create a new method called ReactTestUtils.render which uses the functionality from ReactTestUtils.renderIntoDocument
  • Create 2 new methods
    • ReactTestUtils.renderInto(component, element) - which actually renders into the DOM as a child of element
    • ReactTestUtils.cleanup(component, element) - which cleans up the given child component from element

We're running into problems when trying to test the dimensions of a component i.e. element.offsetWidth, element.offsetHeight

@rupurt
Copy link

rupurt commented Nov 6, 2015

We could even make renderInto take a method that automatically does the cleanup for us e.g.

ReactTestUtils.renderInto(
   <MyComponent>Hello!</MyComponent>,
   document.querySelector("#fixtures"),
   function(componentElm) {
      expect(componentElm.innerHTML).to.contain.text("Hello");
   }
});

@marcysutton
Copy link
Contributor

This is definitely a problem for accessibility testing. We need components to be attached to test them fully, as the current method makes the tree appear to be hidden. It would be really helpful to have blessed methods for doing this, as appending to the body and then removing messes up my other tests with "The DOM was unexpectedly mutated" errors.

@sophiebits
Copy link
Contributor

@marcysutton Thanks, that's useful feedback. If you do need to append to the document then the most straightforward and flexible solution is to call ReactDOM.render manually:

var div = document.createElement('div');
document.body.appendChild(div);
ReactDOM.render(<Foo />, div);

That's also exactly what the implementation of any test helper we might provide would do.

@1Copenut
Copy link

@sophiebits Thank you for this suggestion to use ReactDOM.render(). I had an issue similar to the one @marcysutton asked about. I was using a lot of vanilla JS traversal methods that returned null values when I used ReactTestUtils.renderIntoDocument(). Test lights are all green now.

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 19, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants