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: (motion) Resolve Safari overflow rendering issue in Collapse component #32911

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akatrukhin
Copy link
Member

@akatrukhin akatrukhin commented Sep 25, 2024

In Safari where the overflow property was not being correctly applied during collapse/expand animations.
Safari ignores shorthand overflow: 'hidden' or { overflow } in keyframes as result visible collapsed content.
Issue: #32912

Fix

The overflow property has been split into overflowX and overflowY, both set to hidden, ensuring that Safari correctly handles the overflow behavior during animations.

Tested on:
Safari
Chrome
Firefox
Edge

No regressions were observed on other browsers.

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 25, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)
104.529 kB
32.097 kB
104.594 kB
32.106 kB
65 B
9 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
218.317 kB
63.258 kB
218.382 kB
63.267 kB
65 B
9 B
react-components
react-components: entire library
1.1 MB
272.007 kB
1.1 MB
272.017 kB
65 B
10 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
69.14 kB
20.137 kB
react-components
react-components: FluentProvider & webLightTheme
44.447 kB
14.59 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-timepicker-compat
TimePicker
107.387 kB
35.758 kB
🤖 This report was generated against 213771402438cb4e594bdd7b37b6dcbe77136447

@fredboyle
Copy link

@akatrukhin Do we have an automated visual regression test for this? It would be good to have to avoid missing issues across browsers.

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 25, 2024

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme virtual-rerender 35 35 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 653 620 5000
Button mount 301 314 5000
Field mount 1155 1143 5000
FluentProvider mount 713 730 5000
FluentProviderWithTheme mount 79 89 10
FluentProviderWithTheme virtual-rerender 35 35 10 Possible regression
FluentProviderWithTheme virtual-rerender-with-unmount 76 75 10
MakeStyles mount 852 868 50000
Persona mount 1782 1730 5000
SpinButton mount 1452 1357 5000
SwatchPicker mount 1729 1688 5000

@@ -0,0 +1,7 @@
{
Copy link
Collaborator

@fabricteam fabricteam Sep 25, 2024

Choose a reason for hiding this comment

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

🕵🏾‍♀️ visual regressions to review in the fluentuiv9 Visual Regression Report

Avatar Converged 3 screenshots
Image Name Diff(in Pixels) Image Type
Avatar Converged.Badge Mask RTL.chromium.png 3 Changed
Avatar Converged.size+inactive+badge.chromium.png 1 Changed
Avatar Converged.badgeMask.chromium.png 49 Changed
Drawer 2 screenshots
Image Name Diff(in Pixels) Image Type
Drawer.overlay drawer full.chromium.png 1147 Changed
Drawer.Full Overlay Dark Mode.chromium.png 984 Changed

@akatrukhin
Copy link
Member Author

@akatrukhin Do we have an automated visual regression test for this? It would be good to have to avoid missing issues across browsers.

Fluent UI has automated visual regression tests, but we don't have them for this component. As far as I know, these tests won't detect issues in Safari

@fredboyle
Copy link

@akatrukhin Do we have an automated visual regression test for this? It would be good to have to avoid missing issues across browsers.

Fluent UI has automated visual regression tests, but we don't have them for this component. As far as I know, these tests won't detect issues in Safari

If there isn't a visual regression test for this component then we should have one. We're likely using Playwright for testing which does have the ability to check Safari specifically. If there's a blocker for that let's document it so we can investigate a path to a solution.

Thanks.

Copy link

@mahipalsingh-capsitech mahipalsingh-capsitech left a comment

Choose a reason for hiding this comment

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

You simply write like this---

const toOpacity = 1, fromHeight = '0', toHeight = ${element.scrollHeight}px`, overflowHidden = 'hidden';

const enterKeyframes = [
{ opacity: 0, maxHeight: fromHeight, overflowX: overflowHidden, overflowY: overflowHidden },
{ opacity: toOpacity, maxHeight: toHeight, offset: 0.9999 },
{ opacity: toOpacity, maxHeight: 'unset' }
];

const exitKeyframes = [
{ opacity: toOpacity, maxHeight: toHeight },
{ opacity: 0, maxHeight: fromHeight }
];
`

Safari was not correctly applying the `overflow` property during collapse/expand animations, causing visual issues. This fix splits the shorthand `overflow` into `overflowX` and `overflowY`, which ensures proper rendering across all browsers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants