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.

Sign up for GitHub

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

Popover: enable auto-updating every animation frame #43617

Merged
merged 4 commits into from
Aug 29, 2022
Merged

Popover: enable auto-updating every animation frame #43617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95a070f
Popover: enable autoupdating every animation frame
ciampo Aug 25, 2022
b661a49
Remove unnecessary __unobserveElement prop
ciampo Aug 25, 2022
2b91aef
Remove unnecessary scroll listener on iframe
ciampo Aug 25, 2022
d164ae8
CHANGELOG
ciampo Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/block-popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ function BlockPopover(
// Render in the old slot if needed for backward compatibility,
// otherwise render in place (not in the default popover slot).
__unstableSlotName={ __unstablePopoverSlot || null }
// Observe movement for block animations (especially horizontal).
__unstableObserveElement={ selectedElement }
__unstableForcePosition
__unstableShift
{ ...props }
Expand Down
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fix

- `Popover`: enable auto-updating every animation frame ([#43617](https://github.com/WordPress/gutenberg/pull/43617)).

### Internal

- Refactor `FocalPointPicker` to function component ([#39168](https://github.com/WordPress/gutenberg/pull/39168)).
Expand Down
52 changes: 17 additions & 35 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ const Popover = (
expandOnMobile,
onFocusOutside,
__unstableSlotName = SLOT_NAME,
__unstableObserveElement,
__unstableForcePosition = false,
__unstableShift = false,
...contentProps
Expand Down Expand Up @@ -374,59 +373,42 @@ const Popover = (
return autoUpdate(
resultingReferenceRef,
refs.floating.current,
update
update,
{
animationFrame: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that you did some follow-up work to this, so I was wondering if this is still necessary. The reason I'm asking is because this line is a source of a large impact on performance degradation potentially.

It now happens in the "select" and "inserter open" metrics after this PR #42722 but it can also happen I guess for other interactions when there's a big list of popovers.

So can we remove it or is it still necessary? Are there alternatives?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can be opt-in (or opt-out). However, I think it's generally a bad practice to render a large list of popovers at the same time. The performance regression in #42722 is a bit extreme that we render 1000 empty paragraphs in the test, but I do think we can refactor it in a follow-up to improve this.

From the floating-ui doc:

This is optimized for performance but can still be costly.

This option should be used sparingly...

I guess it makes sense to make it an opt-in in the next major release?

Copy link
Contributor Author

@ciampo ciampo Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced mainly to fix #42725, I expect that bug (and any other nested popover) to happen again if we undo this change — maybe it could be just enabled for the block toolbar?

While I agree that calling the getBoundingClientRect function every animation frame is not ideal, I would also argue that the main reason while this can be a noticeable regression is that we're rendering hundreds of popovers at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turned out that the main issue for the performance regression is not this, so I'm ok not doing anything here and keeping it as but we should keep an eye on this any way as mentioned on their docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense

}
);
// 'reference' and 'refs.floating' are refs and don't need to be listed
// as dependencies (see https://github.com/WordPress/gutenberg/pull/41612)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ anchorRef, anchorRect, getAnchorRect, update ] );

// This is only needed for a smooth transition when moving blocks.
useLayoutEffect( () => {
if ( ! __unstableObserveElement ) {
return;
}
const observer = new window.MutationObserver( update );
observer.observe( __unstableObserveElement, { attributes: true } );

return () => {
observer.disconnect();
};
}, [ __unstableObserveElement, update ] );

// If the reference element is in a different ownerDocument (e.g. iFrame),
// 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 ( referenceOwnerDocument === document ) {
const referenceAndFloatingHaveSameDocument =
referenceOwnerDocument === document;
const hasFrameElement =
!! referenceOwnerDocument?.defaultView?.frameElement;

if ( referenceAndFloatingHaveSameDocument || ! hasFrameElement ) {
frameOffsetRef.current = undefined;
return;
}

const { defaultView } = referenceOwnerDocument;

referenceOwnerDocument.addEventListener( 'scroll', update );
const updateFrameOffset = () => {
frameOffsetRef.current = getFrameOffset( referenceOwnerDocument );
update();
};
defaultView.addEventListener( 'resize', updateFrameOffset );

let updateFrameOffset;
const hasFrameElement =
!! referenceOwnerDocument?.defaultView?.frameElement;
if ( hasFrameElement ) {
updateFrameOffset = () => {
frameOffsetRef.current = getFrameOffset(
referenceOwnerDocument
);
update();
};
updateFrameOffset();
defaultView.addEventListener( 'resize', updateFrameOffset );
}
updateFrameOffset();

return () => {
referenceOwnerDocument.removeEventListener( 'scroll', update );

if ( updateFrameOffset ) {
defaultView.removeEventListener( 'resize', updateFrameOffset );
}
defaultView.removeEventListener( 'resize', updateFrameOffset );
};
}, [ referenceOwnerDocument, update ] );

Expand Down
1 change: 0 additions & 1 deletion packages/components/src/popover/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ export default {
options: AVAILABLE_POSITIONS,
},
__unstableSlotName: { control: { type: null } },
__unstableObserveElement: { control: { type: null } },
__unstableForcePosition: { control: { type: 'boolean' } },
__unstableShift: { control: { type: 'boolean' } },
},
Expand Down