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

set value property explicitly for "option" element even if value is empty #6228

Merged
merged 2 commits into from
Apr 5, 2016
Merged

set value property explicitly for "option" element even if value is empty #6228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea15fa3
set value property explicitly for "option" element even if value is e…
everdimension Mar 9, 2016
28a97db
update to set value property with empty string; add test; update Reac…
everdimension Mar 10, 2016
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
4 changes: 3 additions & 1 deletion src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,10 @@ var DOMPropertyOperations = {
var propName = propertyInfo.propertyName;
// Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
// property type before comparing; only `value` does and is string.
// Must set `value` property if it is not null and not yet set.
Copy link
Member

Choose a reason for hiding this comment

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

You call out value as being special here but this will apply to other things too (eg selected which is also MUST_USE_PROPERTY). Is that your intention?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not saying it shouldn't be changed, but it only applies to HAS_SIDE_EFFECTS which is only used by value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zpao It's a question of being either clear or general. In any case, I'm not aware of other attributes which may need to be handled similarly. And currently it is the only one. The previous comment line mentions this, so I believe writing value makes it more clear for others reading this code.

if (!propertyInfo.hasSideEffects ||
('' + node[propName]) !== ('' + value)) {
('' + node[propName]) !== ('' + value) ||
!node.hasAttribute(propertyInfo.attributeName)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propName] = value;
Expand Down
8 changes: 8 additions & 0 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 @@ -221,6 +221,14 @@ describe('DOMPropertyOperations', function() {
expect(stubNode.getAttribute('role')).toBe('<html>');
});

it('should not remove empty attributes for special properties', function() {
stubNode = document.createElement('input');
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
// JSDOM does not behave correctly for attributes/properties
//expect(stubNode.getAttribute('value')).toBe('');
expect(stubNode.value).toBe('');
});

it('should remove for falsey boolean properties', function() {
DOMPropertyOperations.setValueForProperty(
stubNode,
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,10 +763,10 @@ describe('ReactDOMComponent', function() {
expect(nodeValueSetter.mock.calls.length).toBe(2);

ReactDOM.render(<div value="" />, container);
expect(nodeValueSetter.mock.calls.length).toBe(2);
expect(nodeValueSetter.mock.calls.length).toBe(3);

ReactDOM.render(<div />, container);
expect(nodeValueSetter.mock.calls.length).toBe(2);
expect(nodeValueSetter.mock.calls.length).toBe(3);
});

it('should not incur unnecessary DOM mutations for boolean properties', function() {
Expand Down