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 issue where signing a message sometimes doesn't allow you to scroll to the bottom #15788

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

tayvano
Copy link
Contributor

@tayvano tayvano commented Sep 10, 2022

done in a way to make it as mergeable as possible

Explanation

Fixes the longstanding annoying as fucking fuck issue where you would scroll to the bottom but the sign message button would not become activated.

the cause is a combination of how javascript determines the size and scroll of things and the way css determines the size and scroll of things and the units of measurements used in each and the way browsers, operating systems, and user settings (zoom levels) impact and convert one size to another, combined with how rems are converted to pixels, combined with small rounding errors, combined with the fact that the size of things is not clearly defined with sizing things via css's flexbox combined with the fact that no one in the history of ever understands how overflow works.

but we just avoid all of that nonsense by sidestepping it and removing the border from the footer of the signature view so that there is no variation in how anyone calculates height because no one ever includes a shadow in the height of an element but they do sometimes and sometimes not include the border in the height of an element.

so i just swapped the border attribute with an inset shadow attribute, thereby fixing the most annoying bug ever while not visually changing how it looks or doing anything that could impact another view by more than 1 pixel.

It should be noted

if there are other views impacted by this bug that don't use the .signature-request-footer class, this fix needs to be applied to those views as well. technically though, the fix for that will be to ensure we don't have two separate components/css classes doing the exact same thing. 🤦‍♀️

More Information

I'm sure there are literally dozens of cards, issues, sub issues, slack convos and twitter convos about this. idaf, sorry. this fixes one of the most annoying ux papercuts that currently exists and anyone who actually uses metamask knows the exact issue so im not going to waste anyones time pulling them.

that said, someone please respond to the following tweets and in the #opensea channel when this goes live so that people know we actually care about them and metamask indeed does not have its head so far up its ass that we dont know the annoying things about our product. barbara should know other people who have asked about this who will be happy when this fix is live.

https://twitter.com/trent_vanepps/status/1568659077303238657

https://twitter.com/gremplin/status/1568656560544694273

Manual Testing Steps

  1. go to https://metamask.github.io/test-dapp/
  2. click sign under sign typed data v4 and scroll to the bottom
  3. the button should turn blue

the hardest thing about testing this is ensuring its fixed bc itll only appear on certain screen sizes and densities and browser zoom levels. the easiest way to ensure this is fixed is by ensuring its broken and then changing nothing and ensuring its no longer broken. for me, only zooming out in metamask broke it but because zoom level is per window, my normal metamask doesnt change my test metamask.

the easiest way to minimize the variables between "ensuring its broken" and "ensuring its not broken" is to load up this fixed version, add the css that was breaking it (border-top), and ensure its broken. Then remove that css you just added and ensure its fixed. For me, that looked like this:

  1. go to https://metamask.github.io/test-dapp/
  2. click sign under sign typed data v4
  3. before scrolling to the bottom, right click, on the space between the two buttons and click inspect. The sign button should be disabled/light blue
  4. under styles on the right of chrome devtools click under the .signature-request-footer and paste the following as one chunk and hit enter

border-top: 1px solid var(--color-border-muted);

Should look like this:

Screen Shot 2022-09-10 at 1 30 10 PM

this adds the buggy css and bc we just added it on top of the fix, you should be able to see that the top "border" of the button container is visually slightly thicker.

  1. Now, scroll to the bottom of the message in attempt to sign and ensure that the button is still disabled/light blue. If it triggers and becomes clickable, hit ctrl/cmd - (zoom out one level), cancel the metamask pop up, and return to step 2 and repeat. If its still disabled, cancel the metamask pop up, and continue to next step.

  2. Click on Sign under Sign Typed Data V4 again to trigger a new MM popup. The border should look normal, not slightly thicker like before.

  3. Scroll all the way down. The button should activate so you can sign. If it does, merge this bad boy and tell twitter when it will be live and lfg 🚀

…roll all the way to the bottom

done in a way to make it as mergeable as possible
@tayvano tayvano requested a review from a team as a code owner September 10, 2022 20:47
@tayvano tayvano requested a review from ryanml September 10, 2022 20:47
@github-actions
Copy link
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.

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tayvano thanks for this fix! I tested this a few different ways via different resolutions/zoom levels per your specifications, and all seemed to be well. I did push one fix as there was a linter error. It would be great to get another pair of eyeballs on this or two so we can be certain and get this thing shipped. 👍

@metamaskbot
Copy link
Collaborator

Builds ready [deea560]
Page Load Metrics (1678 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85124101105
domContentLoaded15091991164513464
load15092091167814469
domInteractive15091991164513464

highlights:

storybook

@ryanml
Copy link
Contributor

ryanml commented Sep 11, 2022

Related: #15267

@tayvano
Copy link
Contributor Author

tayvano commented Sep 11, 2022

Thanks Ryan for the quick action on this and especially the linter fix. That's what I get for thinking I couldn't fuck up a single-line change and opening the PR via github.com. 😂 🙈

@ryanml ryanml requested a review from a team September 12, 2022 04:24
@NidhiKJha
Copy link
Member

LGTM! 🚀

@ryanml ryanml merged commit 45916b9 into develop Sep 12, 2022
@ryanml ryanml deleted the tayvano-patch-1 branch September 12, 2022 15:56
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2022
@PeterYinusa PeterYinusa added this to the v10.19.0 milestone Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants