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(overlay): leverage "transition-behavior" to persist top-layer content while closing #4050

Merged
merged 4 commits into from Mar 4, 2024

Conversation

Westbrook
Copy link
Collaborator

@Westbrook Westbrook commented Feb 16, 2024

Description

Currently when the browser closers an Overlay for us, it will fall off of the #top-layer immediately and allow the Overlay content to paint behind standard page content while its close animation is playing.

transition-behavior, in association with the overlay property, allows the content to stay on the #top-layer through the course of the animation.

TO DO

  • decide whether the open Picker demo should continue to be forward facing and test this specific feature, while being broken in Firefox/WebKit, or whether it should be normalized for "working as expected" everywhere

How has this been tested?

  • Test case 1
    1. Go here (Currently Chromium only, more info)
    2. Open the Picker (it should be by default)
    3. Click away from the Menu, so the Picker closes
    4. See that the Menu does not appear "behind" the second Picker while its close animation plays

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook requested a review from a team February 16, 2024 19:48
Copy link

github-actions bot commented Feb 16, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.95 0.98 0.97
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 242.609 kB 227.587 kB 227.564 kB 🏆
Scripts 59.416 kB 54.452 kB 🏆 54.546 kB
Stylesheet 50.724 kB 41.284 kB 41.178 kB 🏆
Document 5.757 kB 5.139 kB 5.128 kB 🏆
Third Party 126.712 kB 126.712 kB 126.712 kB

Request Count

Category Latest Main Branch
Total 42 🏆 43 43
Scripts 34 🏆 35 35
Stylesheet 5 5 5
Document 1 1 1
Third Party 2 2 2

@Westbrook Westbrook marked this pull request as draft February 16, 2024 19:54
Copy link

github-actions bot commented Feb 16, 2024

Tachometer results

Chrome

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 489 kB 69.09ms - 71.40ms - unsure 🔍
-0% - +4%
-0.19ms - +2.66ms
branch 474 kB 68.18ms - 69.84ms unsure 🔍
-4% - +0%
-2.66ms - +0.19ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 655 kB 162.26ms - 165.89ms - unsure 🔍
-0% - +3%
-0.27ms - +4.56ms
branch 635 kB 160.34ms - 163.52ms unsure 🔍
-3% - +0%
-4.56ms - +0.27ms
-

combobox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 706 kB 37.80ms - 38.23ms - unsure 🔍
-0% - +2%
-0.08ms - +0.62ms
branch 695 kB 37.47ms - 38.02ms unsure 🔍
-2% - +0%
-0.62ms - +0.08ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 211.56ms - 216.57ms - slower ❌
0% - 3%
0.63ms - 6.75ms
branch 465 kB 208.62ms - 212.13ms faster ✔
0% - 3%
0.63ms - 6.75ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 494 kB 54.10ms - 54.95ms - unsure 🔍
-2% - +1%
-0.89ms - +0.31ms
branch 486 kB 54.38ms - 55.24ms unsure 🔍
-1% - +2%
-0.31ms - +0.89ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 518 kB 549.77ms - 559.65ms - unsure 🔍
-1% - +2%
-4.07ms - +9.65ms
branch 501 kB 547.16ms - 556.68ms unsure 🔍
-2% - +1%
-9.65ms - +4.07ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 386 kB 21.34ms - 21.61ms - unsure 🔍
-0% - +2%
-0.01ms - +0.51ms
branch 373 kB 21.00ms - 21.45ms unsure 🔍
-2% - +0%
-0.51ms - +0.01ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 722 kB 1858.58ms - 1861.23ms - faster ✔
0% - 1%
6.39ms - 10.07ms
branch 708 kB 1866.86ms - 1869.42ms slower ❌
0% - 1%
6.39ms - 10.07ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 559 kB 43.37ms - 44.20ms - slower ❌
0% - 3%
0.21ms - 1.26ms
branch 540 kB 42.73ms - 43.36ms faster ✔
0% - 3%
0.21ms - 1.26ms
-
Firefox

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 489 kB 145.71ms - 151.49ms - unsure 🔍
-3% - +3%
-4.21ms - +4.69ms
branch 474 kB 144.98ms - 151.74ms unsure 🔍
-3% - +3%
-4.69ms - +4.21ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 655 kB 321.92ms - 331.00ms - unsure 🔍
-1% - +2%
-4.47ms - +8.03ms
branch 635 kB 320.39ms - 328.97ms unsure 🔍
-2% - +1%
-8.03ms - +4.47ms
-

combobox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 706 kB 71.21ms - 78.35ms - slower ❌
13% - 24%
8.12ms - 15.40ms
branch 695 kB 62.27ms - 63.77ms faster ✔
12% - 20%
8.12ms - 15.40ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 442.25ms - 452.51ms - unsure 🔍
-2% - +2%
-7.60ms - +10.28ms
branch 465 kB 438.71ms - 453.37ms unsure 🔍
-2% - +2%
-10.28ms - +7.60ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 589 kB 113.55ms - 118.09ms - unsure 🔍
-2% - +3%
-2.47ms - +2.99ms
branch 579 kB 114.04ms - 117.08ms unsure 🔍
-3% - +2%
-2.99ms - +2.47ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 518 kB 1026.30ms - 1055.86ms - unsure 🔍
-1% - +2%
-8.22ms - +23.86ms
branch 501 kB 1027.04ms - 1039.48ms unsure 🔍
-2% - +1%
-23.86ms - +8.22ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 386 kB 44.31ms - 47.85ms - unsure 🔍
-2% - +10%
-0.98ms - +4.18ms
branch 373 kB 42.61ms - 46.35ms unsure 🔍
-9% - +2%
-4.18ms - +0.98ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 722 kB 1642.21ms - 1651.15ms - faster ✔
0% - 1%
1.46ms - 13.14ms
branch 708 kB 1650.23ms - 1657.73ms slower ❌
0% - 1%
1.46ms - 13.14ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 654 kB 90.64ms - 95.28ms - unsure 🔍
-1% - +5%
-1.02ms - +4.86ms
branch 641 kB 89.23ms - 92.85ms unsure 🔍
-5% - +1%
-4.86ms - +1.02ms
-

@Westbrook Westbrook force-pushed the transition-behavior branch 3 times, most recently from c258044 to 297a79b Compare February 21, 2024 04:15
@Westbrook Westbrook force-pushed the transition-behavior branch 5 times, most recently from 169a032 to 82aab96 Compare March 4, 2024 02:02
@Westbrook Westbrook marked this pull request as ready for review March 4, 2024 02:24
@hunterloftis
Copy link
Contributor

In FF (123.0), the lower (closed) picker renders over the upper (open) picker:

image

@Westbrook
Copy link
Collaborator Author

@hunterloftis this is expected as it falls outside of the support matrix of the polyfill we deliver the Overlay with when popover is not supported. See:

image

Do you think we should roll back that change in the demos? Firefox is tracking v125 support for popover and is currently at v123.

@hunterloftis
Copy link
Contributor

@Westbrook ah I see, it's failing due to the backdrop filter.

Personally I was thrown by the demo (it looked like a bug, rather than a demonstration of an edge case). I think it's fine to leave it as long as we make it clear that it's intending to demonstrate a cross-browser limitation. Naming it something other than "open," or putting text around the demo itself describing what it's showing.

hunterloftis
hunterloftis previously approved these changes Mar 4, 2024
packages/overlay/src/Overlay.ts Outdated Show resolved Hide resolved
packages/overlay/src/OverlayPopover.ts Show resolved Hide resolved
@Westbrook Westbrook merged commit e3dea14 into main Mar 4, 2024
49 checks passed
@Westbrook Westbrook deleted the transition-behavior branch March 4, 2024 22:32
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.

None yet

2 participants