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

Fix <input> with type date/time/etc. issue on mobile browsers #7397

Merged
merged 2 commits into from
Aug 2, 2016
Merged

Fix <input> with type date/time/etc. issue on mobile browsers #7397

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
9cf33a5
fixed <input> with type date/time/etc. issue on iOS safari
Aug 1, 2016
c6f8c32
supported color type and improved comments
Aug 2, 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
22 changes: 20 additions & 2 deletions src/renderers/dom/client/wrappers/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,26 @@ var ReactDOMInput = {
// are not resetable nodes so this operation doesn't matter and actually
// removes browser-default values (eg "Submit Query") when no value is
// provided.
if (props.type !== 'submit' && props.type !== 'reset') {
node.value = node.value;

switch (props.type) {
case 'submit':
case 'reset':
break;
case 'color':
case 'date':
Copy link
Collaborator

@sophiebits sophiebits Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add 'color'? Broken in Android 4.4.4 browser.

case 'datetime':
case 'datetime-local':
case 'month':
case 'time':
case 'week':
// This fixes the no-show issue on iOS Safari and Android Chrome:
// https://github.com/facebook/react/issues/7233
node.value = '';
Copy link

Choose a reason for hiding this comment

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

What's the reason for this assignment since the next line reassigns it to defaultValue? Also, line 249 looks like a no-op as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It fixes a bug where iOS Safari and Android Chrome do not show the value. The one on line 249 detaches .value from .defaultValue (by default, changing .defaultValue affects .value but they work independently as soon as .value is ever set, including to its existing value).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Neat. I need to check to see if this influences my Chrome backspace issue PR (#7359)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like no. At least from my test case:

https://gist.github.com/nhunzaker/d31817ffd48279b4bf1f7513dd84f400

I don't want to make you all linger too much on this. I wish I understood how the detachment works better. Is it possible that this case is display-only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, do you have a specific question? The correct browser behavior is:

.value always reflects the current displayed value of a field and what would be used if the form is submitted.

.defaultValue is always the same as the HTML value attribute (and get/setAttribute 'value'). When a form "reset" button is clicked, .value reverts to .defaultValue.

Initially, they are linked so that changing the value attribute or setting .defaultValue also changes value. Once value changes (either due to .value = or the user interacting), they function independently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, though you've just now answered it. I am sorry to have forced a quick lecture on the DOM :).

I wanted to figure out if the delinking here eliminated the need for the fix I added to #7359 where defaultValue is only reassigned if it is different.

From a quick test outside of React, I thought this PR would make it unnecessary. However it isn't, and I became confused. It looks like, no matter what, value and checked get assigned in ReactDOMInput.getHostProps, and subsequently get sent out as DOM mutations. Even for uncontrolled inputs.

So that's what I'm working through now. No need to continue talking about it here, but I'd like to avoid being able to do the comparison check and wanted to better understand the solution in this PR.

node.value = node.defaultValue;
break;
default:
node.value = node.value;
break;
}

// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
Expand Down
53 changes: 53 additions & 0 deletions src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,21 @@ describe('ReactDOMInput', function() {
expect(node.defaultValue).toBe('1');
});

it('should update `defaultValue` for uncontrolled date/time input', function() {
var container = document.createElement('div');

var node = ReactDOM.render(<input type="date" defaultValue="1980-01-01" />, container);

expect(node.value).toBe('1980-01-01');

ReactDOM.render(<input type="date" defaultValue="2000-01-01" />, container);

expect(node.value).toBe('1980-01-01');
expect(node.defaultValue).toBe('2000-01-01');

ReactDOM.render(<input type="date" />, container);
});

it('should take `defaultValue` when changing to uncontrolled input', function() {
var container = document.createElement('div');

Expand Down Expand Up @@ -790,4 +805,42 @@ describe('ReactDOMInput', function() {
expect(node.value).toEqual('Test');
});

it('resets value of date/time input to fix bugs in iOS Safari', function() {
// https://github.com/facebook/react/issues/7233
if (!ReactDOMFeatureFlags.useCreateElement) {
return;
}

function strify(x) {
return JSON.stringify(x, null, 2);
}

var log = [];
var originalCreateElement = document.createElement;
spyOn(document, 'createElement').and.callFake(function(type) {
var el = originalCreateElement.apply(this, arguments);
if (type === 'input') {
Object.defineProperty(el, 'value', {
set: function(val) {
log.push(`node.value = ${strify(val)}`);
},
});
spyOn(el, 'setAttribute').and.callFake(function(name, val) {
log.push(`node.setAttribute(${strify(name)}, ${strify(val)})`);
});
}
return el;
});

ReactTestUtils.renderIntoDocument(<input type="date" defaultValue="1980-01-01" />);
expect(log).toEqual([
'node.setAttribute("data-reactroot", "")',
'node.setAttribute("type", "date")',
'node.setAttribute("value", "1980-01-01")',
'node.value = ""',
'node.value = ""',
'node.setAttribute("checked", "")',
'node.setAttribute("checked", "")',
]);
});
});