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: prevent scroll adjusting number inputs #1280

Merged
merged 7 commits into from
Dec 6, 2022
Merged

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 29, 2022

What it solves

Resolves #1234

How this PR fixes it

A new NumberField component has been created (isolating the common type and autoComplete attributes). It blurs the field onWheel.

A ref is used as TextField doesn't work with currentTarget.

How to test it

Ensure the following fields work as expected, with validation and preventing scroll from editing the value:

  • Transaction filter: amount, nonce
  • Spending limit amount
  • Send asset amount
  • Advanced transaction parameters: nonce, safeTxGas, wallet nonce, gas limit, maxPriorityFeePerGas, maxFeePerGas

@iamacook iamacook requested a review from usame-algan November 29, 2022 15:24
@github-actions
Copy link

github-actions bot commented Nov 29, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

@usame-algan
Copy link
Member

How does this compare to the alternative that is suggested in the MUI docs i.e. not using type="number" but instead using a pattern argument?

@iamacook iamacook changed the title fix: blur number inputs onWheel events fix: prevent scroll adjusting number inputs Nov 29, 2022
@iamacook
Copy link
Member Author

How does this compare to the alternative that is suggested in the MUI docs i.e. not using type="number" but instead using a pattern argument?

It blurred the input on scroll.

The GOV.UK Design System team make a very compelling argument for the proposed implementation. As it's the suggested implementation by MUI as well, I think we should use it.

@usame-algan
Copy link
Member

The GOV.UK Design System team make a very compelling argument for the proposed implementation. As it's the suggested implementation by MUI as well, I think we should use it.

One issue I noticed with it is that the validation and pattern is not aligned anymore e.g. inputs like +12 and 1e2 are allowed by our validation logic but not by the pattern. This leads to the UI not showing an error but the user can't submit the form. Afaiu the inputmode numeric is specifically for digits 0-9 so changing the pattern would probably not be wise.

Also, should we use the new field for the Amount input as well?

@iamacook
Copy link
Member Author

iamacook commented Nov 30, 2022

One issue I noticed with it is that the validation and pattern is not aligned anymore e.g. inputs like +12 and 1e2 are allowed by our validation logic but not by the pattern. This leads to the UI not showing an error but the user can't submit the form. Afaiu the inputmode numeric is specifically for digits 0-9 so changing the pattern would probably not be wise.

As this issue is purely related to scroll prevention, I would suggest we revert to my initial implementation that blurred the input on scroll input and research then this further. What do you think?

@github-actions
Copy link

github-actions bot commented Nov 30, 2022

Branch preview

✅ Deploy successful!

https://scroll-number-inputs--webcore.review-web-core.5afe.dev

@iamacook iamacook merged commit 637fd61 into dev Dec 6, 2022
@iamacook iamacook deleted the scroll-number-inputs branch December 6, 2022 12:06
@katspaugh katspaugh mentioned this pull request Dec 9, 2022
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.

Prevent scroll from altering type="number" fields
2 participants