feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

digiwand · 2024-09-04T00:58:47Z

Description

Refactors ScrollToBottom logic
- Renames isScrollToBottomNeeded → isScrollToBottomCompleted while inverting logic
Do not require scroll to bottom if the request is a Sign-in With Ethereum request
Do not require scroll to bottom if the signature simulation ("Estimated Changes") is shown

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3131

Manual testing steps

Test scrollable confirmation Confirm button behavior

Screenshots/Recordings

Before

After

Permit with simulation does not need to scroll to confirm, even with Blockaid warning

Sign-in With Ethereum does not need to scroll to confirm

[Existing behavior] Other scrollable confirmations will require scrolling to the bottom to confirm

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

…o check condition

…leted

github-actions · 2024-09-04T00:58:59Z

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.

…t condition

metamaskbot · 2024-09-04T02:05:34Z

Builds ready [dcf9a9c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1810 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	204	2096	1674	495	238
		domContentLoaded	1570	2045	1785	119	57
		load	1573	2102	1810	126	60
		domInteractive	15	83	36	16	8

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 368 Bytes (0.01%)
common: 0 Bytes (0.00%)

Footer is not using useConfirmContext yet throwing disabled logic off

digiwand · 2024-09-05T05:19:05Z

There is 1 flask failing test that appears to be failing on other tests

jpuri · 2024-09-05T08:37:38Z

ui/pages/confirmations/components/confirm/footer/footer.tsx

+    ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
+    mmiSubmitDisabled ||
+    ///: END:ONLY_INCLUDE_IF
+    hardwareWalletRequiresConnection;


It will be nice to create context for scroll functionality and move these logic to it - so that the logic does not goes to components like footer.

that can be done in separate task

Great suggestion!

I created a new, enhancement issue to address this here: #26970

codecov · 2024-09-05T14:01:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.17%. Comparing base (e0868ba) to head (3d47a03).
Report is 11 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26887      +/-   ##
===========================================
+ Coverage    70.17%   70.17%   +0.01%     
===========================================
  Files         1425     1425              
  Lines        49646    49648       +2     
  Branches     13891    13890       -1     
===========================================
+ Hits         34835    34839       +4     
+ Misses       14811    14809       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-09-05T14:14:31Z

Builds ready [b3d2f3c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1828 ± 122 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	234	2568	1660	505	242
		domContentLoaded	1442	2553	1804	259	124
		load	1491	2566	1828	254	122
		domInteractive	15	104	45	27	13

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 293 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarcloud · 2024-09-05T18:09:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-09-05T18:37:26Z

Builds ready [3d47a03]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1845 ± 110 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	365	2752	1782	397	191
		domContentLoaded	1632	2558	1819	197	95
		load	1668	2742	1845	228	110
		domInteractive	15	127	36	24	12

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 296 Bytes (0.00%)
common: 0 Bytes (0.00%)

digiwand added 7 commits September 3, 2024 20:08

refactor: rn isScrollToBottomNeeded-> isScrollToBottomFulfilled

66a04dd

refactor: mv conditional to isConfirmDisabled

aace264

fix: rn isScrollToBottomFulfilled -> isScrollToBottomUnfulfilled

2c59ab8

refactor: update isSIWESignatureRequest to allow Confirmation types t…

5f68231

…o check condition

feat: add ConfirmFooter scroll tests

5491bc7

refactor: rn isScrollToBottomUncompleted unfulfilled -> uncomplete

00a2998

refactor: reverse isScrollToBottomUncompleted -> isScrollToBottomComp…

fc6377b

…leted

github-actions bot added the team-confirmations Push issues to confirmations team label Sep 4, 2024

digiwand added 4 commits September 3, 2024 21:05

fix: ConfirmFooter scroll tests

c159bf0

feat: ConfirmFooter do not require scroll for SIWE

2c8673a

refactor: isPermitSignatureRequest can accept any Confirmation to tes…

9f30b98

…t condition

feat: do not require scroll if Permit Simulation is shown

dcf9a9c

digiwand added 5 commits September 3, 2024 23:11

fix: confirm snapshots

64cbdd4

Footer is not using useConfirmContext yet throwing disabled logic off

fix: restore async ConfirmPage test

7ceebad

Merge branch 'develop' into feat-update-confirm-footer-requirements

16449a8

fix: update ConfirmPage & Footer tests following merge changes

f0db665

fix: ScrollToBottom test

fb116e5

digiwand marked this pull request as ready for review September 5, 2024 03:50

digiwand requested review from a team as code owners September 5, 2024 03:50

Merge branch 'develop' into feat-update-confirm-footer-requirements

e8ecf66

jpuri reviewed Sep 5, 2024

View reviewed changes

jpuri previously approved these changes Sep 5, 2024

View reviewed changes

pedronfigueiredo previously approved these changes Sep 5, 2024

View reviewed changes

matthewwalsh0 previously approved these changes Sep 5, 2024

View reviewed changes

Merge branch 'develop' into feat-update-confirm-footer-requirements

b3d2f3c

digiwand dismissed matthewwalsh0’s stale review via b3d2f3c September 5, 2024 13:43

digiwand dismissed stale reviews from pedronfigueiredo and jpuri via b3d2f3c September 5, 2024 13:43

digiwand changed the title ~~feat: Update Confirm footer scroll-to-bottom requirements~~ feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer Sep 5, 2024

digiwand requested review from jpuri, pedronfigueiredo and matthewwalsh0 September 5, 2024 14:02

pedronfigueiredo previously approved these changes Sep 5, 2024

View reviewed changes

Merge branch 'develop' into feat-update-confirm-footer-requirements

534c3c9

digiwand dismissed pedronfigueiredo’s stale review via 534c3c9 September 5, 2024 16:40

fix: isScrollToBottomNeeded -> isScrollToBottomCompleted

3d47a03

digiwand requested a review from pedronfigueiredo September 5, 2024 18:44

vinistevam approved these changes Sep 6, 2024

View reviewed changes

OGPoyraz approved these changes Sep 6, 2024

View reviewed changes

pedronfigueiredo approved these changes Sep 6, 2024

View reviewed changes

digiwand merged commit 6deac15 into develop Sep 6, 2024
78 checks passed

digiwand deleted the feat-update-confirm-footer-requirements branch September 6, 2024 13:04

github-actions bot locked and limited conversation to collaborators Sep 6, 2024

metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 6, 2024

gauthierpetetin added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

digiwand commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024

metamaskbot commented Sep 4, 2024

digiwand commented Sep 5, 2024

jpuri Sep 5, 2024

jpuri Sep 5, 2024

digiwand Sep 6, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

metamaskbot commented Sep 5, 2024

sonarcloud bot commented Sep 5, 2024

metamaskbot commented Sep 5, 2024

feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

feat: Do not require scroll-to-bottom for SIWE or Signatures with simulations before enabling Confirm footer #26887

Conversation

digiwand commented Sep 4, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Permit with simulation does not need to scroll to confirm, even with Blockaid warning

Sign-in With Ethereum does not need to scroll to confirm

[Existing behavior] Other scrollable confirmations will require scrolling to the bottom to confirm

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Sep 4, 2024

metamaskbot commented Sep 4, 2024

digiwand commented Sep 5, 2024

jpuri Sep 5, 2024

Choose a reason for hiding this comment

jpuri Sep 5, 2024

Choose a reason for hiding this comment

digiwand Sep 6, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

metamaskbot commented Sep 5, 2024

sonarcloud bot commented Sep 5, 2024

Quality Gate passed

metamaskbot commented Sep 5, 2024

digiwand commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading