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

Fix: Vertical toolbar placement in Zoom Out mode #65370

Closed
wants to merge 3 commits into from
Closed

Fix: Vertical toolbar placement in Zoom Out mode #65370

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46f80e8
Fix vertical toolbar placement issue in zoom out mode
AhmarZaidi Sep 16, 2024
4b30720
Merge branch 'trunk' into fix/vertical-toolbar-placement-zoom-out
AhmarZaidi Sep 16, 2024
398081d
Add documentation for custom mutation observer
AhmarZaidi Sep 16, 2024
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
44 changes: 39 additions & 5 deletions packages/components/src/popover/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,11 +287,45 @@ const UnforwardedPopover = (
? undefined
: normalizedPlacementFromProps,
middleware,
whileElementsMounted: ( referenceParam, floatingParam, updateParam ) =>
autoUpdate( referenceParam, floatingParam, updateParam, {
layoutShift: false,
animationFrame: true,
} ),
whileElementsMounted: (
referenceParam,
floatingParam,
updateParam
) => {
const cleanupAutoUpdate = autoUpdate(
referenceParam,
floatingParam,
updateParam,
{
layoutShift: false,
animationFrame: true,
}
);

// Observe mutations on the parent div of the inserter to update toolbar position.
const targetElement = document.querySelector(
'.interface-interface-skeleton__body'
Copy link
Member

Choose a reason for hiding this comment

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

The Popover component is part of @wordpress/components's API. interface-interface-skeleton__body is declared within InterfaceSkeleton, which is part of @wordpress/interface.

However, @wordpress/components does not know about @wordpress/interface at all, nor it should care about the inserter, so tracking mutations of an unknown, unrelated to Popover element doesn't make much sense.

If this is the right fix, it likely needs to be part of InterfaceSkeleton's implementation, or the inserter, and not part of Popover's internals.

I'm not convinced this is the right fix, anyway. This looks like something that should be resolved within floating-ui, cc @ciampo and @jsnajdr who know its internals better than me.

Copy link
Contributor

@ciampo ciampo Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, Marin is right. The @wordpress/components package shouldn't be aware of the @wordpress/interface package (since it's part of the Gutenberg app), and can't be tightly coupled to any of its references.

We should look for another way to fix this.

My gut feeling tells me that the bug is currently caused by popover anchor updates not being tracked correctly and used to re-render the popover.

Copy link
Contributor Author

@AhmarZaidi AhmarZaidi Sep 24, 2024 •
edited by ciampo
Loading

Choose a reason for hiding this comment

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

Hey @ciampo & @tyxla,
Thanks for the review!

I understand now that interface-interface-skeleton__body is part of @wordpress/interface and code from @wordpress/components should not interact/depend on it.

The popover toolbar ref points to the currently selected block div, so that appears to be set correctly but as mentioned by @ciampo, the block div does not register layout changes when the inserter is toggled, thus it's not updating the position of toolbar.

Since, the inserter is added to the interface-interface-skeleton__body, I added a mutation observer to it. It works but it's not the right solution. I have also tested putting layoutShift to true but that doesn't seem to work.

gutenberg/packages/components/src/popover/index.tsx

Line 292 in 2a80118

layoutShift: false,

Will explore more in the suggested direction to find another fix for this issue.

);
Comment on lines +306 to +308
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we shouldn't work directly with DOM. Have you considered using a ref instead?


if ( targetElement ) {
const mutationObserver = new MutationObserver( () => {
updateParam();
} );

mutationObserver.observe( targetElement, {
childList: true,
subtree: true,
attributes: true,
} );

return () => {
cleanupAutoUpdate();
mutationObserver.disconnect();
};
}

return cleanupAutoUpdate;
},
} );

const arrowCallbackRef = useCallback(
Expand Down
Loading