experimental/SelectPanel: Update anchored position after showModal #4396
Conversation
🦋 Changeset detectedLatest commit: 15c0853 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
size-limit report 📦
|
| sx={{ | ||
| // to avoid a visible position shift, we delay the overlay animating-in | ||
| // to wait until the correct position is set (see useAnchoredPosition above for more) | ||
| animationDelay: '16ms', |
There was a problem hiding this comment.
Note for reviewer: This isn't scientific at all, but we need to pick a number, so based it off a 60Hz refresh rate 😅
From https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame,
The frequency of calls to the callback function will generally match the display refresh rate. The most common refresh rate is 60hz, (60 cycles/frames per second), though 75hz, 120hz, and 144hz are also widely used. requestAnimationFrame() calls are paused in most browsers when running in background tabs or hidden iframes, in order to improve performance and battery life.
There was a problem hiding this comment.
An alternative approach would be to "watch" for dialog open. Either in a recursive timeout or intersection observer, but that felt overkill!
| /* Dialog */ | ||
| const dialogRef = React.useRef<HTMLDialogElement>(null) | ||
|
|
||
| /* Anchored position */ |
There was a problem hiding this comment.
Note for reviewer: This function already existed, just moved it above
| animation: overlay-appear ${animationDuration}ms ${get('animation.easeOutCubic')}; | ||
| animation-fill-mode: forwards; | ||
| opacity: 0; |
There was a problem hiding this comment.
Note for reviewer: Need to add this if we want to delay animation
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
siddharthkp
changed the title
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
broccolinisoup
left a comment
broccolinisoup
left a comment
There was a problem hiding this comment.
Same problem as we had on tooltip! I think delaying it with an animation makes sense to me.
| if (internalOpen) { | ||
| dialogRef.current?.showModal() | ||
| updatePosition() // update the position once the dialog is open | ||
| } else if (dialogRef.current?.open) dialogRef.current.close() |
There was a problem hiding this comment.
This could be inlined
| } else if (dialogRef.current?.open) dialogRef.current.close() | |
| } else dialogRef.current?.close() |
siddharthkp
added
the
integration-tests: recommended
|
Note for self: This change seems to break tests (both in this repo and dotcom) where we instantly check if Overlay contents are visible (while they are animating) Need to reduce the surface area of this change just to SelectPanel |
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
siddharthkp
removed
the
integration-tests: recommended
The problem:
When an html
<dialog>is not "open", it has the dimensions of 0px by 0px, which means the anchored position is not able to calculate if the dialog is going beyond the window boundary.1.problem.mov
Solution
We need to update position after
dialog.showModal()has "opened" the dialog and given it dimensions. This, however, creates a new problem of visible layout shift which can look janky.In Safari, you see the dialog move but it's done before you can tell what happened:
2.position.shift.mov
In Chrome, you can really see the page glitching because for a tiny moment, Chrome tries to horizontally scroll the page to make sure the dialog is visible:
2.position.shift.chrome.mov
Polish
We can solve this by adding a minuscule delay to the animating-in, this gives us just enough time to set the correct position (one tick)
3.gefixed.mov