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

Jump to bottom

Rewrite DOMPropertyOperations-test in terms of public API #10281

Merged
merged 3 commits into from
Aug 7, 2017
Merged

Rewrite DOMPropertyOperations-test in terms of public API #10281

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5587271
Rewrite DOMPropertyOperations-test in terms of public API
gaearon Jul 26, 2017
005f835
Add more assertions
gaearon Aug 5, 2017
8e5c758
Address review feedback
gaearon Aug 5, 2017
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
275 changes: 90 additions & 185 deletions src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,67 +12,48 @@
'use strict';

describe('DOMPropertyOperations', () => {
var DOMPropertyOperations;
var DOMProperty;
var ReactDOMComponentTree;
var React;
var ReactDOM;

beforeEach(() => {
jest.resetModules();
require('ReactDOMInjection');

// TODO: can we express this test with only public API?
DOMPropertyOperations = require('DOMPropertyOperations');
DOMProperty = require('DOMProperty');
ReactDOMComponentTree = require('ReactDOMComponentTree');
React = require('react');
ReactDOM = require('react-dom');
});

describe('setValueForProperty', () => {
var stubNode;
var stubInstance;

beforeEach(() => {
stubNode = document.createElement('div');
stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);
});

it('should set values as properties by default', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'title', 'Tip!');
expect(stubNode.title).toBe('Tip!');
var container = document.createElement('div');
ReactDOM.render(<div title="Tip!" />, container);
expect(container.firstChild.title).toBe('Tip!');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the advantage to using ReactDOM.render over TestUtils.renderIntoDocument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

None. Just seems a bit more flexible.


it('should set values as attributes if necessary', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'role', '#');
expect(stubNode.getAttribute('role')).toBe('#');
expect(stubNode.role).toBeUndefined();
var container = document.createElement('div');
ReactDOM.render(<div role="#" />, container);
expect(container.firstChild.getAttribute('role')).toBe('#');
expect(container.firstChild.role).toBeUndefined();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking: I didn't see a test that covers attributes that require property assignment. I think it would be covered by a test that uses multiple with a select, but the behavior is probably covered by ReactDOMSelect-test.


it('should set values as namespace attributes if necessary', () => {
spyOn(stubNode, 'setAttributeNS');
DOMPropertyOperations.setValueForProperty(
stubNode,
'xlinkHref',
'about:blank',
);
expect(stubNode.setAttributeNS.calls.count()).toBe(1);
expect(stubNode.setAttributeNS.calls.argsFor(0)).toEqual([
'http://www.w3.org/1999/xlink',
'xlink:href',
'about:blank',
]);
var container = document.createElement('svg');
ReactDOM.render(<image xlinkHref="about:blank" />, container);
expect(
container.firstChild.getAttributeNS(
'http://www.w3.org/1999/xlink',
'href',
),
).toBe('about:blank');
});

it('should set values as boolean properties', () => {
DOMPropertyOperations.setValueForProperty(
stubNode,
'disabled',
'disabled',
);
expect(stubNode.getAttribute('disabled')).toBe('');
DOMPropertyOperations.setValueForProperty(stubNode, 'disabled', true);
expect(stubNode.getAttribute('disabled')).toBe('');
DOMPropertyOperations.setValueForProperty(stubNode, 'disabled', false);
expect(stubNode.getAttribute('disabled')).toBe(null);
var container = document.createElement('div');
ReactDOM.render(<div disabled="disabled" />, container);
expect(container.firstChild.getAttribute('disabled')).toBe('');
ReactDOM.render(<div disabled={true} />, container);
expect(container.firstChild.getAttribute('disabled')).toBe('');
ReactDOM.render(<div disabled={false} />, container);
expect(container.firstChild.getAttribute('disabled')).toBe(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this test cover null too? Is that overkill?

});

it('should convert attribute values to string first', () => {
Expand All @@ -83,181 +64,105 @@ describe('DOMPropertyOperations', () => {
return '<html>';
},
};
DOMPropertyOperations.setValueForProperty(stubNode, 'role', obj);
expect(stubNode.getAttribute('role')).toBe('<html>');
var container = document.createElement('div');
ReactDOM.render(<div role={obj} />, container);
expect(container.firstChild.getAttribute('role')).toBe('<html>');
});

it('should not remove empty attributes for special properties', () => {
stubNode = document.createElement('input');
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
var container = document.createElement('div');
ReactDOM.render(<input value="" />, container);
// JSDOM does not behave correctly for attributes/properties
//expect(stubNode.getAttribute('value')).toBe('');
expect(stubNode.value).toBe('');
//expect(container.firstChild.getAttribute('value')).toBe('');
expect(container.firstChild.value).toBe('');
});

it('should remove for falsey boolean properties', () => {
DOMPropertyOperations.setValueForProperty(
stubNode,
'allowFullScreen',
false,
);
expect(stubNode.hasAttribute('allowFullScreen')).toBe(false);
var container = document.createElement('div');
ReactDOM.render(<div allowFullScreen={false} />, container);
expect(container.firstChild.hasAttribute('allowFullScreen')).toBe(false);
});

it('should remove when setting custom attr to null', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'data-foo', 'bar');
expect(stubNode.hasAttribute('data-foo')).toBe(true);
DOMPropertyOperations.setValueForProperty(stubNode, 'data-foo', null);
expect(stubNode.hasAttribute('data-foo')).toBe(false);
});

it('should use mutation method where applicable', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this test. It seems like the only mutation method we use is value, and AFAIK there are plenty tests about it specifically. Internally at FB we don’t inject custom properties with mutation methods either.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍. Long term, I believe we can remove mutation method all together if we circle back to #10150

var foobarSetter = jest.fn();
// inject foobar DOM property
DOMProperty.injection.injectDOMPropertyConfig({
Properties: {foobar: null},
DOMMutationMethods: {
foobar: foobarSetter,
},
});

DOMPropertyOperations.setValueForProperty(
stubNode,
'foobar',
'cows say moo',
);

expect(foobarSetter.mock.calls.length).toBe(1);
expect(foobarSetter.mock.calls[0][0]).toBe(stubNode);
expect(foobarSetter.mock.calls[0][1]).toBe('cows say moo');
var container = document.createElement('div');
ReactDOM.render(<div data-foo="bar" />, container);
expect(container.firstChild.hasAttribute('data-foo')).toBe(true);
ReactDOM.render(<div data-foo={null} />, container);
expect(container.firstChild.hasAttribute('data-foo')).toBe(false);
});

it('should set className to empty string instead of null', () => {
DOMPropertyOperations.setValueForProperty(
stubNode,
'className',
'selected',
);
expect(stubNode.className).toBe('selected');

DOMPropertyOperations.setValueForProperty(stubNode, 'className', null);
var container = document.createElement('div');
ReactDOM.render(<div className="selected" />, container);
expect(container.firstChild.className).toBe('selected');
ReactDOM.render(<div className={null} />, container);
// className should be '', not 'null' or null (which becomes 'null' in
// some browsers)
expect(stubNode.className).toBe('');
expect(stubNode.getAttribute('class')).toBe(null);
expect(container.firstChild.className).toBe('');
expect(container.firstChild.getAttribute('class')).toBe(null);
});

it('should remove property properly for boolean properties', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'hidden', true);
expect(stubNode.hasAttribute('hidden')).toBe(true);

DOMPropertyOperations.setValueForProperty(stubNode, 'hidden', false);
expect(stubNode.hasAttribute('hidden')).toBe(false);
});

it('should remove property properly even with different name', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this test. I couldn’t find any cases where property name would differ in practice—neither in master nor internally at FB. If we add such case we should add an integration test for that specific property instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this doesn't happen for any prop that must be assigned as a DOM property. The only thing we alias are a few HTML attribute names and a bunch of SVG attribute names.

// Suppose 'foobar' is a property that corresponds to the underlying
// 'className' property:
DOMProperty.injection.injectDOMPropertyConfig({
Properties: {foobar: DOMProperty.injection.MUST_USE_PROPERTY},
DOMPropertyNames: {
foobar: 'className',
},
DOMAttributeNames: {
foobar: 'class',
},
});

DOMPropertyOperations.setValueForProperty(stubNode, 'foobar', 'selected');
expect(stubNode.className).toBe('selected');

DOMPropertyOperations.setValueForProperty(stubNode, 'foobar', null);
// className should be '', not 'null' or null (which becomes 'null' in
// some browsers)
expect(stubNode.className).toBe('');
var container = document.createElement('div');
ReactDOM.render(<div hidden={true} />, container);
expect(container.firstChild.hasAttribute('hidden')).toBe(true);
ReactDOM.render(<div hidden={false} />, container);
expect(container.firstChild.hasAttribute('hidden')).toBe(false);
});
});

describe('value mutation method', function() {
it('should update an empty attribute to zero', function() {
var stubNode = document.createElement('input');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

stubNode.setAttribute('type', 'radio');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
spyOn(stubNode, 'setAttribute');
DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0);

expect(stubNode.setAttribute.calls.count()).toBe(1);
var container = document.createElement('div');
ReactDOM.render(
<input type="radio" value="" onChange={function() {}} />,
container,
);
spyOn(container.firstChild, 'setAttribute');
ReactDOM.render(
<input type="radio" value={0} onChange={function() {}} />,
container,
);
expect(container.firstChild.setAttribute.calls.count()).toBe(1);
});

it('should always assign the value attribute for non-inputs', function() {
var stubNode = document.createElement('progress');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

spyOn(stubNode, 'setAttribute');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30);
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30');

expect(stubNode.setAttribute.calls.count()).toBe(2);
var container = document.createElement('div');
ReactDOM.render(<progress />, container);
spyOn(container.firstChild, 'setAttribute');
ReactDOM.render(<progress value={30} />, container);
ReactDOM.render(<progress value="30" />, container);
expect(container.firstChild.setAttribute.calls.count()).toBe(2);
});
});

describe('deleteValueForProperty', () => {
var stubNode;
var stubInstance;

beforeEach(() => {
stubNode = document.createElement('div');
stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);
});

it('should remove attributes for normal properties', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'title', 'foo');
expect(stubNode.getAttribute('title')).toBe('foo');
expect(stubNode.title).toBe('foo');

DOMPropertyOperations.deleteValueForProperty(stubNode, 'title');
expect(stubNode.getAttribute('title')).toBe(null);
var container = document.createElement('div');
ReactDOM.render(<div title="foo" />, container);
expect(container.firstChild.getAttribute('title')).toBe('foo');
expect(container.firstChild.title).toBe('foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is existing test behavior, so take it or leave it, but I think line 143 is essentially testing that JSDOM syncs up the title property with the title attribute. Could line 143 go away?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal with line 146.

ReactDOM.render(<div />, container);
expect(container.firstChild.getAttribute('title')).toBe(null);
// JSDOM does not behave correctly for attributes/properties
//expect(stubNode.title).toBe('');
//expect(container.firstChild.title).toBe('');
});

it('should not remove attributes for special properties', () => {
stubNode = document.createElement('input');
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

stubNode.setAttribute('value', 'foo');

DOMPropertyOperations.deleteValueForProperty(stubNode, 'value');
// JSDOM does not behave correctly for attributes/properties
//expect(stubNode.getAttribute('value')).toBe('foo');
expect(stubNode.value).toBe('');
Copy link
Collaborator Author

@gaearon gaearon Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The diff got a bit messed up. The new test is:

     var container = document.createElement('div');
      spyOn(console, 'error');
      ReactDOM.render(
        <input type="text" value="foo" onChange={function() {}} />,
        container,
      );
      ReactDOM.render(
        <input type="text" onChange={function() {}} />,
        container,
      );
      expect(container.firstChild.getAttribute('value')).toBe('foo');
      expect(console.error.calls.count()).toBe(1);
      expect(console.error.calls.argsFor(0)[0]).toContain(
        'A component is changing a controlled input of type text to be uncontrolled',
        );

But I’m not sure this is right. As you can see, the last line in the old test:

      // JSDOM does not behave correctly for attributes/properties
      //expect(stubNode.getAttribute('value')).toBe('foo');
      expect(stubNode.value).toBe('');

was actually not true anymore. However uncommented line works:

expect(container.firstChild.getAttribute('value')).toBe('foo');

I’m not sure what’s going on here, and whether my test reproduces this case, or not.
What is this really testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Answering to myself: this test was written in #6228, seemingly to fix #6219. Perhaps I can use fiddle from that issue to recreate the original problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new test seems wrong to me, the second render should be <input ... value="" /> to be truthful to the original test-case (it's about preserving the initial attribute value). But AFAIK this changed way back when you made the attribute always reflect the property for input value? So this should fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why did the test pass then, if it’s wrong? Is it because we were testing internals rather than public API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In other words, I think my test is not faithful to the original test, but is faithful to the actual behavior in 15.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test might be a victim of changes in how value is assigned over the churn trying to fix bugs in controlled inputs. Your change is a good one, but I wonder it is covered by the round of tests starting at:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js#L873

});

it('should not leave all options selected when deleting multiple', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn’t figure out how to write this test in terms of public API. I tried rendering <select multiple> with both value and defaultValue, and then rendering <select> without multiple but couldn’t get this test to pass. If I need to bring it back, can somebody please explain how? Or maybe this is better suited for DOM fixtures.

Copy link
Collaborator Author

@gaearon gaearon Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

cc @syranide who wrote this test originally in #1510

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure what you're asking. The key thing is that multiple individual options will still have selected set to true in the DOM when removing multiple as an attribute in some browsers. So you need to render with multiple and multiple options selected, then remove multiple and verify that only one option is set in the DOM. The logical way would probably be using an uncontrolled select (because that also avoids controlled behavior potentially fixing it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. I tried rendering uncontrolled select and then rendering it without multiple, and these assertions failed. I'll try again.

Copy link
Contributor

@syranide syranide Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@gaearon Doesn't that mean the current implementation is broken then? (EDIT: if we care about fixing it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately I don't know 🤷‍♀️ We have the old unit test but it's not clear to me whether it is testing the right thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The key thing is that multiple individual options will still have selected set to true in the DOM when removing multiple as an attribute in some browsers.

If this behavior is inconsistent, and JSDOM does not exhibit it, do we need to make this a DOM Test Fixture and explore the surface area of browsers that demonstrate it?

stubNode = document.createElement('select');
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

stubNode.multiple = true;
stubNode.appendChild(document.createElement('option'));
stubNode.appendChild(document.createElement('option'));
stubNode.options[0].selected = true;
stubNode.options[1].selected = true;

DOMPropertyOperations.deleteValueForProperty(stubNode, 'multiple');
expect(stubNode.getAttribute('multiple')).toBe(null);
expect(stubNode.multiple).toBe(false);

expect(stubNode.options[0].selected && stubNode.options[1].selected).toBe(
false,
var container = document.createElement('div');
spyOn(console, 'error');
ReactDOM.render(
<input type="text" value="foo" onChange={function() {}} />,
container,
);
ReactDOM.render(
<input type="text" onChange={function() {}} />,
container,
);
expect(container.firstChild.getAttribute('value')).toBe('foo');
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toContain(
'A component is changing a controlled input of type text to be uncontrolled',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this providing any additional coverage over the uncontrolled/controlled switch tests in `ReactDOMInput?

});
Copy link
Contributor

Choose a reason for hiding this comment

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

If this test is giving us trouble via unit testing, I think we should drop it and consider writing a DOM fixture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, please lump this into the chat above about selects.

});
Expand Down