-
Notifications
You must be signed in to change notification settings - Fork 230
feat(overlay): replace native showModal() for performance optimization #5307
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
Conversation
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
b2d28a0
to
50ff91c
Compare
59b702f
to
4e2228c
Compare
6ff9288
to
810eaa7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a fast follow ticket for the skipped flaky tests, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
she already confirmed the voiceover worked after cache fix. Nikki and I also confirmed this works so removing the changes requested review to unblock merging.
#5307) * chore: add test page * chore: add comment * chore: add a fast page (no dom complexity) * chore: update test page * chore: update stuff * chore: use popover as dialog wrapper * chore: try focus trap * chore: fix focus trap * chore: add Escape event listener to dismiss modal * chore: add AI JSDocs * chore: fixing some focustrap behaviour * chore: adding some comments * chore: fix some styles * chore: fix comment * chore: check if is topmost overlay before closing * chore: add aria modal * chore: display block fine * chore: update focusable slectors based on focus-trap * chore: remove overlay dialog functionality * chore: fix stuff * chore: fix stuff * chore: ensure all types are correct * chore: remove Safari focus ring handling from HoverController and related tests * chore: update aria-modal handling to include 'page' type overlays * chore: add focus trapping tests for modal and page overlay types * chore: debugging broken ci tests * chore: try adding parallelism * revert: remove parallelism from chrome * chore: improved overlay test * fix: test with waitUntill removing nextFrame * chore: correct button reference in overlay trigger test * chore: removed unnecessary lint updates * chore: add proper stories * chore: update overlay doc * chore: skip some flaky tests in ci * chore: update golden image hash * fix(overlay): ensure picker opens correctly in a modal in safari * chore: add missing package dependency * chore: update golden image hash * fix(overlay): ensure picker opens correctly in a modal in safari * chore: update golden image hash * chore: skip action group mouse test --------- Co-authored-by: TarunAdobe <ttomar@adobe.com> Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
TODO
forcePopover - is a popover in mobile
- action-menu-responsive.test.tsforcePopover - is a popover in mobile
- picker-responsive.test.tsOverlay Trigger - extended - manages placement on scroll
- overlay-trigger-extended.tsOverlay Trigger - extended - occludes content behind the overlay
- overlay-trigger-extended.tsDescription
Context about the problem
Opening or closing a modal dialog in Spectrum Web Components is slow (up to 5+ seconds on lower-end devices).
You can verify it here by opening the modal overlay and setting 4×+ CPU throttling in Chrome DevTools.
The slowdown occurs when using the native
dialog.showModal()
API in all browsers with a large or complex DOM. We’re particularly concerned about Chromium-based browsers, which are dominant across the Android ecosystem where entry-level devices are more common. This is a known Chromium issue: opening a modal viashowModal()
triggers inert mode, which causes a full style recalculation across all non-dialog content. After reviewing the Chromium implementation, the “setting inertness” step appears to be the primary bottleneck.You can observe the performance difference between
showModal()
andshow()
in this CodePen:👉 https://codepen.io/rubencarvalho/pen/VYwgMwa
And a second demo confirming that
inert
propagation itself is expensive:👉 https://codepen.io/rubencarvalho/pen/MYWLEPg
I’ve followed up on the Chromium bug report and will continue tracking progress.
The path forward
To work around the performance issues caused by
showModal()
, in this PR, we reimplement the native functionality using the existing popover logic:popover.showPopover()
instead ofshowModal()
;This gives us the visual and stacking behavior we want without triggering the expensive full-document inertness.
However, by avoiding
showModal()
, we lose some of the browser’s built-in functionality. We needed to manually reimplement the following behaviors:aria-modal="true"
, ensuring screen-readers don't read content in the supposedly inert DOM)Related issue(s)
How has this been tested?
No changes to the perceived overlay behaviour
Test focus trap on modal
Test focus trap on page overlay
Test VO or other screen assist (e.g. use the VoiceOver rotor on Mac)
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.