Skip to content

Conversation

@YuanboXue-Amber
Copy link
Contributor

@YuanboXue-Amber YuanboXue-Amber commented Nov 6, 2023

Previous Behavior

DialogTransitionContext is always defined and has value entered for test environment (#29692). However, DialogSurface has base style opacity: 0, and when transition status is entered, it merges style opacity: 1. To apply styles DialogSurface uses makeResetStyles() & makeStyles(), so will get following:

.reset-class {
  /* ... some CSS rules */
  opacity: 0;
}
.atomic-class {
  opacity: 1;
}

In this case, tests for DialogSurface using jest-dom & .toBeVisible() can fail. This is a jsdom bug - it cannot detect style override with 'opacity' nor 'visibility' (check Stackblitz). This happens because DialogSurface has as base style opacity:0: getComputedStyles(DialogSurfaceElement).opacity in jsdom returns 0 and ignores the opacity:1 style override 💥

Odd thing is, fix #29692 is good enough for a simple test that asserts something within DialogSurface via '.toBeVisible()'. But when a random style override is added to DialogSurface, the test can fail with the same reason. This PR added a unit test case for this.

New Behavior

  • DialogTransitionContext is undefined for test environment.
  • DialogSurface adds style opacity:0 only when DialogTransitionContext is defined.

Test is green and animation works as is.

Discarded solution

I also considered simply moving opacity:0 from baseStyle to 'unmounted' state's style, but it breaks animation:

Screen.Recording.2023-11-06.at.11.57.34.mov

note that DialogSurface shows up immediately after open. This is because transition state is exited entering and then entered when Dialog is opened. Adding opacity:0 based on state is not as safe option compare to not adding it at all for test environment.

@YuanboXue-Amber YuanboXue-Amber requested a review from a team as a code owner November 6, 2023 10:47
@YuanboXue-Amber YuanboXue-Amber marked this pull request as draft November 6, 2023 10:53
@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme mount 83 81 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 628 641 5000
Button mount 316 315 5000
Field mount 1134 1156 5000
FluentProvider mount 702 718 5000
FluentProviderWithTheme mount 83 81 10 Possible regression
FluentProviderWithTheme virtual-rerender 70 70 10
FluentProviderWithTheme virtual-rerender-with-unmount 86 81 10
MakeStyles mount 901 881 50000
Persona mount 1760 1744 5000
SpinButton mount 1387 1405 5000

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 6, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 93d56c7:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
70.007 kB
20.164 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
207.417 kB
59.289 kB
react-components
react-components: FluentProvider & webLightTheme
42.291 kB
14.005 kB
react-dialog
Dialog (including children components)
94.473 kB
28.28 kB
react-portal-compat
PortalCompatProvider
6.651 kB
2.252 kB
🤖 This report was generated against aa664abe172b5ceedb63bafdc690ac2e82ab48db

@YuanboXue-Amber YuanboXue-Amber marked this pull request as ready for review November 6, 2023 11:02
@size-auditor
Copy link

size-auditor bot commented Nov 6, 2023

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: aa664abe172b5ceedb63bafdc690ac2e82ab48db (build)

@fabricteam
Copy link
Collaborator

🕵 fluentuiv9 No visual regressions between this PR and main

@YuanboXue-Amber YuanboXue-Amber merged commit bcd3039 into microsoft:master Nov 6, 2023
@YuanboXue-Amber YuanboXue-Amber deleted the fix-upg branch November 6, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants