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

Do not assign node.value on input creation if no change will occur #12925

Merged

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented May 29, 2018

What

This commit fixes an issue where assigning an empty string to required text inputs triggers the invalid state in Firefox (~60.0.1).

It does this by first comparing the initial state value to the current value property on the text element. Doing so:

  1. Prevents the validation issue
  2. Avoids an extra DOM Mutation in some cases

I have also updated the text fixtures to include the empty string test case presented in #8395 (comment)

Test Plan

  1. Open http://react-more-required-validation-pass.surge.sh/text-inputs
  2. View the Required Input test case
  3. All inputs should appear valid
  4. On iOS and Android, date inputs should present a value

screen shot 2018-05-29 at 7 47 11 am

Tested In

  • Safari 9, 11
  • Chrome 49, 66
  • Firefox 47, 60
  • IE 9, 11
  • Edge 16, 17
  • iOS 9, 11 Safari
  • Chrome for Android

Related: #8395 (comment)

@nhunzaker
Copy link
Contributor Author

Browser testing checks out!

@nhunzaker nhunzaker requested review from jquense and aweary May 29, 2018 12:23
@pull-bot
Copy link

pull-bot commented May 29, 2018

ReactDOM: size: -0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: aa85b0f...b7c136d

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% +0.1% 623.98 KB 624.28 KB 145.15 KB 145.25 KB UMD_DEV
react-dom.production.min.js -0.0% 0.0% 93.88 KB 93.87 KB 30.37 KB 30.38 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 608.34 KB 608.65 KB 141.15 KB 141.26 KB NODE_DEV
react-dom.production.min.js -0.0% 🔺+0.1% 92.38 KB 92.37 KB 29.35 KB 29.37 KB NODE_PROD
ReactDOM-dev.js 0.0% +0.1% 630.62 KB 630.92 KB 143.89 KB 144 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 274.44 KB 274.48 KB 51.64 KB 51.66 KB FB_WWW_PROD

Generated by 🚫 dangerJS

@nhunzaker nhunzaker force-pushed the nh-more-required-validation-cases branch from b864343 to 8dd72ab Compare May 29, 2018 12:26
"size": 2230,
"gzip": 1097
"size": 2417,
"gzip": 1121
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind reverting changes to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all. Done.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Looks all right to me.

This commit fixes an issue where assigning an empty string to required
text inputs triggers the invalid state in Firefox (~60.0.1).

It does this by first comparing the initial state value to the current
value property on the text element. This:

1. Prevents the validation issue
2. Avoids an extra DOM Mutation in some cases
@nhunzaker nhunzaker force-pushed the nh-more-required-validation-cases branch from 8dd72ab to b7c136d Compare May 29, 2018 13:25
@gaearon gaearon merged commit 8aeea5a into facebook:master May 29, 2018
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 13, 2018
193: Update dependency react to v16.4.1 r=magopian a=renovate[bot]

This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1`



<details>
<summary>Release Notes</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request Jun 14, 2018
25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
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.

None yet

4 participants