Skip to content

fix: <input type=number> with spread operator breaks float data entry #3495

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

Conversation

colincasey
Copy link
Contributor

Fixes #3426 by not setting attributes which haven't changed.

This eliminates the usability issue with the cursor observed in Chrome. This may be the only browser affected since others like Firefox automatically remove the . character when deleting the fractional part of a number which avoids the series of interactions that trigger this edge case.

This change should be covered by existing tests but no sample to replicate the bug could be created though an attempt was made using the following approaches:

  • observing changes to the the cursor position

this is impossible since, according to the spec, selection capabilities on an <input type=number> element are not allowed

  • observing unexpected changes on the value field by triggering input events

@codecov-io
Copy link

codecov-io commented Sep 3, 2019

Codecov Report

Merging #3495 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3495   +/-   ##
=======================================
  Coverage   50.25%   50.25%           
=======================================
  Files           1        1           
  Lines         197      197           
=======================================
  Hits           99       99           
  Misses         98       98

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c470d00...2d06724. Read the comment docs.

Rich-Harris added a commit that referenced this pull request Sep 5, 2019
@Rich-Harris
Copy link
Member

Thank you. I was pondering an alternative fix, in #3506 — avoiding setting the input value during the update that was triggered by the binding.

I'm torn between the two approaches. One the one hand, adding the additional property lookup and comparison for every property means extra work (plus the very modest increase in bundle size). On the other hand, maybe checking the value before setting it would pay for itself, because we wouldn't be invoking setters?

@colincasey
Copy link
Contributor Author

i like your fix in #3506. it aligns better with the number input editor behavior in the browser and if setting attributes that haven't changed had no ill effect before, why introduce the overhead for this one minor edge case?

@Rich-Harris
Copy link
Member

cool — will go ahead and merge #3506. thanks

@Rich-Harris Rich-Harris closed this Sep 7, 2019
Rich-Harris added a commit that referenced this pull request Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

input "number" with ...spread operator breaks floating values
3 participants