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: cp-12.13.0 fix modal scroll bar flash #30355

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Feb 16, 2025

Description

This PR fixes a visual issue where a flashing scroll bar appears during modal animations. The issue was caused by the animation properties in the modal-content component in this recently merged PR. The fix involves:

  1. Removing overflow: auto from the modal-content container
  2. Adding overflow: hidden to the animation keyframes to prevent scroll bar flickering

This is a hotfix for the 12.13.0 release and addresses a regression identified during visual testing.

Open in GitHub Codespaces

Related issues

Fixes: #30354

Manual testing steps

  1. Open any modal in the application (e.g. Networks modal)
  2. Observe the modal animation
  3. Verify there is no flashing scroll bar during the animation
  4. Test with different screen sizes to ensure consistent behavior

Screenshots/Recordings

Before

before.mov

After

after.mov

Pre-merge author checklist

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.

Visual regression testing for all modals is being tracked in this spreadsheet:
https://docs.google.com/spreadsheets/d/1fzGHktbmnE-jDa8SxvmiTmunTOchFyAYmpBRlKAbQYo/edit?gid=700367359#gid=700367359

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Feb 16, 2025
@georgewrmarshall georgewrmarshall self-assigned this Feb 16, 2025
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.

};

return (
Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Feb 16, 2025

Choose a reason for hiding this comment

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

Fixes bug in the Modal storybook

Before

before.storybook720.mov

After

storybook.after720.mov

@georgewrmarshall georgewrmarshall changed the title fix: cp-12.13.0 Modal flashes scrollbar fix: cp-12.13.0 Fix modal scroll bar flash Feb 16, 2025
@georgewrmarshall georgewrmarshall changed the title fix: cp-12.13.0 Fix modal scroll bar flash fix: cp-12.13.0 fix modal scroll bar flash Feb 16, 2025
@georgewrmarshall georgewrmarshall marked this pull request as ready for review February 18, 2025 17:04
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner February 18, 2025 17:04
}

to {
transform: translateY(0);
opacity: 1;
overflow: hidden;
}

Choose a reason for hiding this comment

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

is it possible to update this doc/screenshot to have the children string says something along that line of ModalBody Content instead of Modal Content? Since there's a ModalContent component I actually thought it should be used instead of ModalBody from reading that ss

brianacnguyen
brianacnguyen previously approved these changes Feb 18, 2025
Copy link

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

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

Approved with a non-blocking recommendation to update doc for better clarity

@metamaskbot
Copy link
Collaborator

Builds ready [992023b]
Page Load Metrics (1672 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25419841543450216
domContentLoaded14571922164913364
load14661978167214469
domInteractive238337189
backgroundConnect884272110
firstReactRender1475352311
getState45812136
initialActions01000
loadScripts10021393116311354
setupStore661202010
uiStartup16882497191418991
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Comment on lines -8 to -9
overflow: auto;
overscroll-behavior-y: none;
Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Feb 18, 2025

Choose a reason for hiding this comment

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

Impact of removing overflow: auto

The removal of overflow: auto in the ModalContent component is designed to fix the scrollbar flickering issue without affecting properly composed modals that already handle scrolling correctly through the design system components. This includes modals structured in either of these standard ways:

// Standard Modal Structure 1
<ModalContent>
  <ModalHeader />
  <ModalBody>
    Modal Body
  </ModalBody>
</ModalContent>

// Standard Modal Structure 2  
<ModalContent>
  <ModalHeader />
  <Box className="handle-scrolling-via-css">
    Modal content
  </Box>
</ModalContent>

Potential Impact Considerations:

  • ✅ Modals using the design system components correctly will continue to work as expected
  • ⚠️ Modals that don't follow the standard composition pattern might experience layout issues where the overflow content is not visible

Verification & Testing:
To ensure compatibility, I've completed comprehensive visual regression testing of all modal implementations. The results are documented in:

@georgewrmarshall georgewrmarshall added this pull request to the merge queue Feb 18, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [b51bea9]
Page Load Metrics (1755 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34020641680326157
domContentLoaded1554192517269747
load15862085175511053
domInteractive25114402612
backgroundConnect12162373517
firstReactRender1571312110
getState561192010
initialActions00000
loadScripts1134147712748842
setupStore86119189
uiStartup18052377202212560
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Merged via the queue into main with commit 0b66f5d Feb 18, 2025
76 checks passed
@georgewrmarshall georgewrmarshall deleted the fix/30354/modal-scrollbar branch February 18, 2025 19:25
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2025
@metamaskbot metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.14.0 Issue or pull request that will be included in release 12.14.0 team-design-system All issues relating to design system in Extension
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Fix modal scroll bar flash
4 participants