Fix autolock field to accept decimals in Firefox#19653
Merged
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
5ac3611 to
0c121aa
Compare
Codecov Report
@@ Coverage Diff @@
## develop #19653 +/- ##
===========================================
- Coverage 70.79% 70.79% -0.00%
===========================================
Files 988 988
Lines 38366 38375 +9
Branches 10042 10046 +4
===========================================
+ Hits 27161 27167 +6
- Misses 11205 11208 +3
|
Collaborator
Builds ready [0c121aa]
Page Load Metrics (1498 ± 45 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
0c121aa to
1f32af1
Compare
ae0f5f9 to
cc247be
Compare
The autolock field on the Settings screen — the field that allows users
to set the duration that MetaMask will wait for until automatically
locking — does not always accept decimal numbers. This breaks the e2e
test for this feature as it attempts to set this field to "0.1".
More specifically, the React component responsible for this field passes
whatever the user inputs through the `Number` function immediately and
then uses this to repopulate the input. Therefore, if the user enters
"3" followed by a ".", `Number("3.")` will be called. This evaluates to
the number 3, and "3" becomes the new value of the field. As a result,
the "." can never be typed.
Curiously, this behavior only happens in Firefox; Chrome seems to
keep the "." in the input field when it's typed. This happens because
`onChange` event doesn't seem to get fired until a number is typed
*after* the ".". This may be due to underlying differences in the DOM
between Chrome and Firefox.
Regardless, always passing the input through `Number` creates other odd
behavior, such as the fact that the input can never be cleared (because
`Number("")` evaluates to 0).
This commit solves these problems by saving the "raw" version of the
user's input as well as the normalized version. The raw version is
always used to populate the input, whereas the normalized version is
saved in state.
cc247be to
363a85d
Compare
Collaborator
Builds ready [363a85d]
Page Load Metrics (1569 ± 43 ms)
Bundle size diffs
|
Gudahtt
reviewed
Jun 19, 2023
Gudahtt
reviewed
Jun 19, 2023
Gudahtt
reviewed
Jun 19, 2023
Collaborator
Builds ready [511b2f2]
Page Load Metrics (1480 ± 26 ms)
Bundle size diffs
|
Collaborator
Builds ready [2069663]
Page Load Metrics (1631 ± 65 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [c7958f3]
Page Load Metrics (1629 ± 45 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
brad-decker
approved these changes
Jun 22, 2023
adonesky1
approved these changes
Jun 22, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
The autolock field on the Settings screen — the field that allows users to set the duration that MetaMask will wait for until automatically locking — does not always accept decimal numbers. This breaks the e2e test for this feature as it attempts to set this field to "0.1".
More specifically, the React component responsible for this field passes whatever the user inputs through the
Numberfunction immediately and then uses this to repopulate the input. Therefore, if the user enters "3" followed by a ".",Number("3.")will be called. This evaluates to the number 3, and "3" becomes the new value of the field. As a result, the "." can never be typed.Curiously, this behavior only happens in Firefox; Chrome seems to keep the "." in the input field when it's typed. This happens because
onChangeevent doesn't seem to get fired until a number is typed after the ".". This may be due to underlying differences in the DOM between Chrome and Firefox.Regardless, always passing the input through
Numbercreates other odd behavior, such as the fact that the input can never be cleared (becauseNumber("")evaluates to 0).This commit solves these problems by saving the "raw" version of the user's input as well as the normalized version. The raw version is always used to populate the input, whereas the normalized version is saved in state.
Screenshots/Screencaps
Before
Chrome
It's hard to see, but 1) pressing Backspace when it says "0" does nothing and 2) adding numbers after the decimal does nothing.
before-chrome.mov
Firefox
It's hard to see, but 1) pressing Backspace when it says "0" does nothing, 2) adding decimals or anything after the decimal resets the field to "0".
before-firefox.mov
After
Chrome
after-chrome.mov
Firefox
after-firefox.mov
Manual Testing Steps
Fundamentally you should ensure that if you go into Settings > Advanced, update the autolock setting to 1, save, then wait 1 minute, MetaMask should automatically lock. You should also see an error (and should not be able to save) if you enter a number past 10080.
With this change, however, you should also be able to enter 0.1, and MetaMask should autolock in 5 seconds.
Also, if you empty the field, it should default to 5 minutes (this has always been the placeholder value for this input, it's just that it never got used).
All of this behavior should be the same in both Chrome and Firefox.
Past this, all other settings on the page should work as intended.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.