Popover: make sure offset middleware always applies the latest frame offset values #43329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Type] Bug
ciampo
changed the title
ciampo
force-pushed
the
fix/popover-frame-ref
branch
from
ed95a32
to
85eb8df
Compare
ciampo
commented
Aug 17, 2022
ciampo
commented
Aug 17, 2022
|
|
||
| const middleware = [ | ||
| frameOffset.current || offset |
There was a problem hiding this comment.
Removed the frameOffset.current || offset check, since it can cause a situation where offsetMiddleware is not added/removed when needed
Example:
frameOffset.currentstarts asundefined- therefore,
offsetMiddlewareis not added to the array frameOffset.currentis assigned to an object (e.g{x: 0, y: 32 })- the change to
frameOffset.currentdoesn't cause a re-render, and thereforeoffsetMiddlewareis still not added to the list of middlewares
There was a problem hiding this comment.
Thanks for catching that, this change makes sense 👍
ciampo
commented
Aug 17, 2022
| @@ -390,33 +388,37 @@ const Popover = ( | |||
| // we need to manually update the floating's position as the reference's owner | |||
| // document scrolls. Also update the frame offset if the view resizes. | |||
| useLayoutEffect( () => { | |||
| if ( ownerDocument === document ) { | |||
| if ( referenceOwnerDocument === document ) { | |||
| frameOffsetRef.current = undefined; | |||
There was a problem hiding this comment.
Resetting frameOffsetRef.current when there's no iframe
There was a problem hiding this comment.
Yes, I guess this is possible if the popover anchor changes from something in an iframe to something outside. Good spot 👍
ciampo
commented
Aug 17, 2022
| @@ -244,7 +244,7 @@ export const WithSlotOutsideIframe = ( args ) => { | |||
| <Iframe | |||
| style={ { | |||
| width: '100%', | |||
| height: '100%', | |||
| height: '400px', | |||
There was a problem hiding this comment.
These are just some cosmetic changes for the Storybook iframe example (see PR description for more info)
|
Size Change: +17 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
This was referenced Aug 17, 2022
ciampo
commented
Aug 17, 2022
| @@ -166,7 +166,7 @@ const Popover = ( | |||
| ? positionToPlacement( position ) | |||
| : placementProp; | |||
|
|
|||
| const ownerDocument = useMemo( () => { | |||
| const referenceOwnerDocument = useMemo( () => { | |||
There was a problem hiding this comment.
Renamed to a more clear name
ciampo
commented
Aug 17, 2022
| : undefined, | ||
| offsetMiddleware( ( { placement: currentPlacement } ) => { | ||
| if ( ! frameOffsetRef.current ) { | ||
| return offset ?? 0; |
There was a problem hiding this comment.
Added a fallback, since now we're not guaranteed that offset will be defined when the middleware runs
There was a problem hiding this comment.
Another option could be to give the prop a default value. That would also mean normalizedOffset could be removed a few lines below this.
There was a problem hiding this comment.
Good point, done as part of 4bf6dfc
|
I'm working on a follow-up PR (#43335) which could hopefully solve more glitches related to the component not updating correctly, although this PR will need to be merged first |
talldan
approved these changes
Aug 18, 2022
ciampo
force-pushed
the
fix/popover-frame-ref
branch
from
85eb8df
to
4bf6dfc
Compare
|
@talldan thank you for your review! I pushed a new commit where I applied your feedback (default value for the |
Awesome, thanks! |
|
Thanks for your work here Marco! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Tracked in #42770
Fixes a regression introduced by #43172 and #43267 where the
Popoverwould sometimes not position correctly compared to its anchorWhy?
The cause of this regression is that when
frameOffsetwas refactored to be a ref object, we didn't take into account the fact that theoffsetmiddleware was being added based (also) onframeOffset.current— and therefore, since the value offrameOffset.currentwouldn't cause a re-render when changing (it's a ref), theoffsetmiddleware would not be correctly added / removed.(see #43329 (comment) for more details)
How?
This PR makes the
offsetmiddleware always present in the middleware chain, removing the conditional check. This ensures that the middleware runs every time — the resulting offset will be still based offframeOffset.currentanyway.Testing Instructions
Open Storybook and make sure that the examples work as expected
In both the site editor and the post editor:
Known issues:
The iframe storybook example is still not behaving as expected (it never did since its introduction), but it's getting better every time. The reason for this may be related to the
anchorRefobject and the fact that its changes don't always trigger hook runs as they should.Screenshots
trunk:Kapture.2022-08-17.at.17.00.21.mp4
This PR:
Kapture.2022-08-17.at.16.58.55.mp4