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

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

Merged

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Aug 1, 2016

Fixes #7233.

In short, iOS Safari acts weird when dealing with HTML5 input types: assigning node.defaultValue doesn't let the browser update the UI, so we'll need to set node.value to a dummy value then reset it.

Reviewers: @spicyj @jimfb

Test Plan:
https://jsbin.com/sefihic/edit?js,output should work on iOS Safari and other mobile browsers (I tested with iOS Safari, iOS Chrome, and Android Browser)

@dozoisch
Copy link

dozoisch commented Aug 2, 2016

Tested in my product! Works perfectly :)! :shipit:

case 'month':
case 'time':
case 'week':
// this fixes the no-show issue on iOS Safari: https://github.com/facebook/react/issues/7233
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you say "iOS Safari and Android Chrome" and capitalize the sentence?

@sophiebits sophiebits added this to the 15-next milestone Aug 2, 2016
@ghost ghost added the CLA Signed label Aug 2, 2016
@keyz keyz changed the title Fix <input> with type date/time/etc. issue on iOS safari Fix <input> with type date/time/etc. issue on mobile browsers Aug 2, 2016
@keyz keyz merged commit 8af6f9e into facebook:master Aug 2, 2016
case 'time':
case 'week':
// this fixes the no-show issue on iOS Safari: https://github.com/facebook/react/issues/7233
node.value = '';

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.

@troydemonbreun troydemonbreun mentioned this pull request Aug 12, 2016
@zpao zpao modified the milestones: 15.3.1, 15-next Aug 12, 2016
zpao pushed a commit that referenced this pull request Aug 12, 2016
Fix <input> with type date/time/etc. issue on mobile browsers
(cherry picked from commit 8af6f9e)
@iammerrick
Copy link
Contributor

Getting peer dependency issues trying to install the release candidate. Any ideas about when 15.3.1 will land so we can plan accordingly?

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

Successfully merging this pull request may close these issues.

7 participants