From bd12ea733ad3dc9ddbb74c45beb27e12ae9ae50a Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Thu, 22 Jun 2023 09:29:24 -0700 Subject: [PATCH] Fix autolock field to accept decimals in Firefox (#19653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/_locales/de/messages.json | 3 -- app/_locales/el/messages.json | 3 -- app/_locales/en/messages.json | 4 +- app/_locales/es/messages.json | 3 -- app/_locales/es_419/messages.json | 3 -- app/_locales/fr/messages.json | 3 -- app/_locales/hi/messages.json | 3 -- app/_locales/id/messages.json | 3 -- app/_locales/it/messages.json | 3 -- app/_locales/ja/messages.json | 3 -- app/_locales/ko/messages.json | 3 -- app/_locales/ph/messages.json | 3 -- app/_locales/pt/messages.json | 3 -- app/_locales/pt_BR/messages.json | 3 -- app/_locales/ru/messages.json | 3 -- app/_locales/tl/messages.json | 3 -- app/_locales/tr/messages.json | 3 -- app/_locales/vi/messages.json | 3 -- app/_locales/zh_CN/messages.json | 3 -- app/_locales/zh_TW/messages.json | 3 -- app/scripts/controllers/app-state.js | 3 +- shared/constants/preferences.ts | 2 + test/e2e/tests/auto-lock.spec.js | 2 +- ui/ducks/metamask/metamask.js | 3 +- ui/pages/routes/routes.container.js | 4 +- .../advanced-tab/advanced-tab.component.js | 49 +++++++++++++------ .../advanced-tab.component.test.js | 13 +++-- .../advanced-tab/advanced-tab.container.js | 3 +- 28 files changed, 59 insertions(+), 81 deletions(-) diff --git a/app/_locales/de/messages.json b/app/_locales/de/messages.json index 1525a731a..248f25f40 100644 --- a/app/_locales/de/messages.json +++ b/app/_locales/de/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Ausloggen" }, - "lockTimeTooGreat": { - "message": "Sperrzeit ist zu groß" - }, "logo": { "message": "$1-Logo", "description": "$1 is the name of the ticker" diff --git a/app/_locales/el/messages.json b/app/_locales/el/messages.json index c70d365c9..9361b3725 100644 --- a/app/_locales/el/messages.json +++ b/app/_locales/el/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Αποσύνδεση" }, - "lockTimeTooGreat": { - "message": "Ο χρόνος κλειδώματος είναι πολύ μεγάλος" - }, "logo": { "message": "Λογότυπο $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 71a426f5a..b0ef001ba 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -2171,8 +2171,8 @@ "lockMetaMask": { "message": "Lock MetaMask" }, - "lockTimeTooGreat": { - "message": "Lock time is too great" + "lockTimeInvalid": { + "message": "Lock time must be a number between 0 and 10080" }, "logo": { "message": "$1 logo", diff --git a/app/_locales/es/messages.json b/app/_locales/es/messages.json index 474b8997b..387497661 100644 --- a/app/_locales/es/messages.json +++ b/app/_locales/es/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Bloquear" }, - "lockTimeTooGreat": { - "message": "El tiempo de bloqueo es demasiado largo" - }, "logo": { "message": "Logo de $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/es_419/messages.json b/app/_locales/es_419/messages.json index 93431b1b2..6562f505a 100644 --- a/app/_locales/es_419/messages.json +++ b/app/_locales/es_419/messages.json @@ -1269,9 +1269,6 @@ "lock": { "message": "Bloquear" }, - "lockTimeTooGreat": { - "message": "El tiempo de bloqueo es demasiado largo" - }, "low": { "message": "Baja" }, diff --git a/app/_locales/fr/messages.json b/app/_locales/fr/messages.json index 7b179078f..ea020f961 100644 --- a/app/_locales/fr/messages.json +++ b/app/_locales/fr/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Déconnexion" }, - "lockTimeTooGreat": { - "message": "Le temps de verrouillage est trop important" - }, "logo": { "message": "Logo $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/hi/messages.json b/app/_locales/hi/messages.json index 69f1099a3..5a6757562 100644 --- a/app/_locales/hi/messages.json +++ b/app/_locales/hi/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "लॉक" }, - "lockTimeTooGreat": { - "message": "लॉक समय बहुत अधिक है" - }, "logo": { "message": "$1 लोगो", "description": "$1 is the name of the ticker" diff --git a/app/_locales/id/messages.json b/app/_locales/id/messages.json index 23fe1f555..da9df8643 100644 --- a/app/_locales/id/messages.json +++ b/app/_locales/id/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Kunci" }, - "lockTimeTooGreat": { - "message": "Lock time terlalu besar" - }, "logo": { "message": "Logo $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/it/messages.json b/app/_locales/it/messages.json index 4ba71fa64..d18255efd 100644 --- a/app/_locales/it/messages.json +++ b/app/_locales/it/messages.json @@ -1117,9 +1117,6 @@ "lock": { "message": "Disconnetti" }, - "lockTimeTooGreat": { - "message": "Tempo di inattività troppo lungo" - }, "mainnet": { "message": "Rete Ethereum Principale" }, diff --git a/app/_locales/ja/messages.json b/app/_locales/ja/messages.json index 74e387c46..fee446d3e 100644 --- a/app/_locales/ja/messages.json +++ b/app/_locales/ja/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "ロック" }, - "lockTimeTooGreat": { - "message": "ロック時間が長すぎます" - }, "logo": { "message": "$1 ロゴ", "description": "$1 is the name of the ticker" diff --git a/app/_locales/ko/messages.json b/app/_locales/ko/messages.json index c6a89341b..46e74e3da 100644 --- a/app/_locales/ko/messages.json +++ b/app/_locales/ko/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "잠금" }, - "lockTimeTooGreat": { - "message": "잠금 시간이 너무 깁니다" - }, "logo": { "message": "$1 로고", "description": "$1 is the name of the ticker" diff --git a/app/_locales/ph/messages.json b/app/_locales/ph/messages.json index c2c399b48..8b0947625 100644 --- a/app/_locales/ph/messages.json +++ b/app/_locales/ph/messages.json @@ -816,9 +816,6 @@ "lock": { "message": "I-lock" }, - "lockTimeTooGreat": { - "message": "Masyadong matagal ang oras ng pag-lock" - }, "mainnet": { "message": "Ethereum Mainnet" }, diff --git a/app/_locales/pt/messages.json b/app/_locales/pt/messages.json index 58665feaf..1c26903ed 100644 --- a/app/_locales/pt/messages.json +++ b/app/_locales/pt/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Sair" }, - "lockTimeTooGreat": { - "message": "O tempo de bloqueio é longo demais" - }, "logo": { "message": "Logotipo do $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/pt_BR/messages.json b/app/_locales/pt_BR/messages.json index 24e06aec1..c299dbfed 100644 --- a/app/_locales/pt_BR/messages.json +++ b/app/_locales/pt_BR/messages.json @@ -1269,9 +1269,6 @@ "lock": { "message": "Bloquear" }, - "lockTimeTooGreat": { - "message": "O tempo de bloqueio é longo demais" - }, "low": { "message": "Baixa" }, diff --git a/app/_locales/ru/messages.json b/app/_locales/ru/messages.json index b236dde70..8fedcfa31 100644 --- a/app/_locales/ru/messages.json +++ b/app/_locales/ru/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Заблокировать" }, - "lockTimeTooGreat": { - "message": "Время блокировки слишком велико" - }, "logo": { "message": "логотип $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/tl/messages.json b/app/_locales/tl/messages.json index 2bac2eae2..9318c44cf 100644 --- a/app/_locales/tl/messages.json +++ b/app/_locales/tl/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "I-lock" }, - "lockTimeTooGreat": { - "message": "Masyadong matagal ang oras ng pag-lock" - }, "logo": { "message": "$1 logo", "description": "$1 is the name of the ticker" diff --git a/app/_locales/tr/messages.json b/app/_locales/tr/messages.json index fdaaedffc..0e5bab809 100644 --- a/app/_locales/tr/messages.json +++ b/app/_locales/tr/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Kilitle" }, - "lockTimeTooGreat": { - "message": "Kilitleme süresi çok fazla" - }, "logo": { "message": "$1 logosu", "description": "$1 is the name of the ticker" diff --git a/app/_locales/vi/messages.json b/app/_locales/vi/messages.json index a1bc5ca94..89b4aff06 100644 --- a/app/_locales/vi/messages.json +++ b/app/_locales/vi/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "Khóa" }, - "lockTimeTooGreat": { - "message": "Thời gian khóa quá lớn" - }, "logo": { "message": "Logo $1", "description": "$1 is the name of the ticker" diff --git a/app/_locales/zh_CN/messages.json b/app/_locales/zh_CN/messages.json index 60ccc7859..43ea94627 100644 --- a/app/_locales/zh_CN/messages.json +++ b/app/_locales/zh_CN/messages.json @@ -1863,9 +1863,6 @@ "lock": { "message": "注销" }, - "lockTimeTooGreat": { - "message": "锁定时间过长" - }, "logo": { "message": "$1标志", "description": "$1 is the name of the ticker" diff --git a/app/_locales/zh_TW/messages.json b/app/_locales/zh_TW/messages.json index e6e45115b..914c3f3fe 100644 --- a/app/_locales/zh_TW/messages.json +++ b/app/_locales/zh_TW/messages.json @@ -821,9 +821,6 @@ "lock": { "message": "鎖定" }, - "lockTimeTooGreat": { - "message": "鎖定時間過長" - }, "mainnet": { "message": "以太坊 主網路" }, diff --git a/app/scripts/controllers/app-state.js b/app/scripts/controllers/app-state.js index 722f1f56a..1bbf4a4f8 100644 --- a/app/scripts/controllers/app-state.js +++ b/app/scripts/controllers/app-state.js @@ -13,6 +13,7 @@ import { POLLING_TOKEN_ENVIRONMENT_TYPES, ORIGIN_METAMASK, } from '../../../shared/constants/app'; +import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../shared/constants/preferences'; export default class AppStateController extends EventEmitter { /** @@ -32,7 +33,7 @@ export default class AppStateController extends EventEmitter { this.onInactiveTimeout = onInactiveTimeout || (() => undefined); this.store = new ObservableStore({ - timeoutMinutes: 0, + timeoutMinutes: DEFAULT_AUTO_LOCK_TIME_LIMIT, connectedStatusPopoverHasBeenShown: true, defaultHomeActiveTabName: null, browserEnvironment: {}, diff --git a/shared/constants/preferences.ts b/shared/constants/preferences.ts index fab436736..9a37b0281 100644 --- a/shared/constants/preferences.ts +++ b/shared/constants/preferences.ts @@ -3,3 +3,5 @@ export enum ThemeType { dark = 'dark', os = 'os', } + +export const DEFAULT_AUTO_LOCK_TIME_LIMIT = 0; diff --git a/test/e2e/tests/auto-lock.spec.js b/test/e2e/tests/auto-lock.spec.js index 71a25966b..40a1009f2 100644 --- a/test/e2e/tests/auto-lock.spec.js +++ b/test/e2e/tests/auto-lock.spec.js @@ -38,7 +38,7 @@ describe('Auto-Lock Timer', function () { await autoLockTimerInput.fill(10081); await driver.waitForSelector({ css: '#autoTimeout-helper-text', - text: 'Lock time is too great', + text: 'Lock time must be a number between 0 and 10080', }); await autoLockTimerInput.fill(sixSecsInMins); await driver.assertElementNotPresent('#autoTimeout-helper-text'); diff --git a/ui/ducks/metamask/metamask.js b/ui/ducks/metamask/metamask.js index 0c853e74d..832c08d4c 100644 --- a/ui/ducks/metamask/metamask.js +++ b/ui/ducks/metamask/metamask.js @@ -15,6 +15,7 @@ import { updateTransactionGasFees } from '../../store/actions'; import { setCustomGasLimit, setCustomGasPrice } from '../gas/gas.duck'; import { KeyringType } from '../../../shared/constants/keyring'; +import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../shared/constants/preferences'; import { isEqualCaseInsensitive } from '../../../shared/modules/string-utils'; import { stripHexPrefix } from '../../../shared/modules/hexstring-utils'; import { decGWEIToHexWEI } from '../../../shared/modules/conversion.utils'; @@ -37,7 +38,7 @@ const initialState = { currentLocale: '', currentBlockGasLimit: '', preferences: { - autoLockTimeLimit: undefined, + autoLockTimeLimit: DEFAULT_AUTO_LOCK_TIME_LIMIT, showFiatInTestnets: false, showTestNetworks: false, useNativeCurrencyAsPrimaryCurrency: true, diff --git a/ui/pages/routes/routes.container.js b/ui/pages/routes/routes.container.js index 9e1e6660c..ba1c33777 100644 --- a/ui/pages/routes/routes.container.js +++ b/ui/pages/routes/routes.container.js @@ -25,12 +25,14 @@ import { pageChanged } from '../../ducks/history/history'; import { prepareToLeaveSwaps } from '../../ducks/swaps/swaps'; import { getSendStage } from '../../ducks/send'; import { getProviderConfig } from '../../ducks/metamask/metamask'; +import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../shared/constants/preferences'; import Routes from './routes.component'; function mapStateToProps(state) { const { appState } = state; const { alertOpen, alertMessage, isLoading, loadingMessage } = appState; - const { autoLockTimeLimit = 0 } = getPreferences(state); + const { autoLockTimeLimit = DEFAULT_AUTO_LOCK_TIME_LIMIT } = + getPreferences(state); const { completedOnboarding } = state.metamask; return { diff --git a/ui/pages/settings/advanced-tab/advanced-tab.component.js b/ui/pages/settings/advanced-tab/advanced-tab.component.js index 837eb0d0d..95c5250a9 100644 --- a/ui/pages/settings/advanced-tab/advanced-tab.component.js +++ b/ui/pages/settings/advanced-tab/advanced-tab.component.js @@ -22,6 +22,7 @@ import { MetaMetricsEventCategory, MetaMetricsEventName, } from '../../../../shared/constants/metametrics'; +import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../../shared/constants/preferences'; import { exportAsFile } from '../../../helpers/utils/export-utils'; import ActionableMessage from '../../../components/ui/actionable-message'; import ZENDESK_URLS from '../../../helpers/constants/zendesk-url'; @@ -72,6 +73,7 @@ export default class AdvancedTab extends PureComponent { state = { autoLockTimeLimit: this.props.autoLockTimeLimit, + autoLockTimeLimitBeforeNormalization: this.props.autoLockTimeLimit, lockTimeError: '', showLedgerTransportWarning: false, showResultMessage: false, @@ -379,11 +381,10 @@ export default class AdvancedTab extends PureComponent {
this.handleLockChange(e.target.value)} error={lockTimeError} fullWidth @@ -601,21 +602,41 @@ export default class AdvancedTab extends PureComponent { ); } - handleLockChange(time) { + handleLockChange(autoLockTimeLimitBeforeNormalization) { const { t } = this.context; - const autoLockTimeLimit = Math.max(Number(time), 0); - this.setState(() => { - let lockTimeError = ''; + if (autoLockTimeLimitBeforeNormalization === '') { + this.setState({ + autoLockTimeLimitBeforeNormalization, + autoLockTimeLimit: DEFAULT_AUTO_LOCK_TIME_LIMIT.toString(), + lockTimeError: '', + }); + return; + } - if (autoLockTimeLimit > 10080) { - lockTimeError = t('lockTimeTooGreat'); - } + const autoLockTimeLimitAfterNormalization = Number( + autoLockTimeLimitBeforeNormalization, + ); - return { - autoLockTimeLimit, - lockTimeError, - }; + if ( + Number.isNaN(autoLockTimeLimitAfterNormalization) || + autoLockTimeLimitAfterNormalization < 0 || + autoLockTimeLimitAfterNormalization > 10080 + ) { + this.setState({ + autoLockTimeLimitBeforeNormalization, + autoLockTimeLimit: null, + lockTimeError: t('lockTimeInvalid'), + }); + return; + } + + const autoLockTimeLimit = autoLockTimeLimitAfterNormalization; + + this.setState({ + autoLockTimeLimitBeforeNormalization, + autoLockTimeLimit, + lockTimeError: '', }); } diff --git a/ui/pages/settings/advanced-tab/advanced-tab.component.test.js b/ui/pages/settings/advanced-tab/advanced-tab.component.test.js index 06bd51011..da26ebfdc 100644 --- a/ui/pages/settings/advanced-tab/advanced-tab.component.test.js +++ b/ui/pages/settings/advanced-tab/advanced-tab.component.test.js @@ -31,14 +31,21 @@ describe('AdvancedTab Component', () => { expect(restoreFile).toBeInTheDocument(); }); - it('should update autoLockTimeLimit', () => { + it('should default the auto-lockout time to 0', () => { + const { queryByTestId } = renderWithProvider(, mockStore); + const autoLockoutTime = queryByTestId('auto-lockout-time'); + + expect(autoLockoutTime).toHaveValue('0'); + }); + + it('should update the auto-lockout time', () => { const { queryByTestId } = renderWithProvider(, mockStore); const autoLockoutTime = queryByTestId('auto-lockout-time'); const autoLockoutButton = queryByTestId('auto-lockout-button'); - fireEvent.change(autoLockoutTime, { target: { value: 1440 } }); + fireEvent.change(autoLockoutTime, { target: { value: '1440' } }); - expect(autoLockoutTime).toHaveValue(1440); + expect(autoLockoutTime).toHaveValue('1440'); fireEvent.click(autoLockoutButton); diff --git a/ui/pages/settings/advanced-tab/advanced-tab.container.js b/ui/pages/settings/advanced-tab/advanced-tab.container.js index 539e60b33..55cb59490 100644 --- a/ui/pages/settings/advanced-tab/advanced-tab.container.js +++ b/ui/pages/settings/advanced-tab/advanced-tab.container.js @@ -17,6 +17,7 @@ import { } from '../../../store/actions'; import { getPreferences } from '../../../selectors'; import { doesUserHaveALedgerAccount } from '../../../ducks/metamask/metamask'; +import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../../shared/constants/preferences'; import AdvancedTab from './advanced-tab.component'; export const mapStateToProps = (state) => { @@ -37,7 +38,7 @@ export const mapStateToProps = (state) => { const { showFiatInTestnets, showTestNetworks, - autoLockTimeLimit = 0, + autoLockTimeLimit = DEFAULT_AUTO_LOCK_TIME_LIMIT, } = getPreferences(state); const userHasALedgerAccount = doesUserHaveALedgerAccount(state);