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 moving block popover when zoomed out #65981

Closed
wants to merge 1 commit into from
Closed

Fix moving block popover when zoomed out #65981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions packages/block-editor/src/components/block-popover/inbetween.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,16 @@ function BlockPopoverInbetween( {
const observer = new window.MutationObserver( forcePopoverRecompute );
observer.observe( previousElement, { attributes: true } );

const zoomedOutObserver = new window.MutationObserver(
forcePopoverRecompute
);
zoomedOutObserver.observe( previousElement.closest( 'html' ), {
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be a bit too aggressive to recompute the popover position on every attribute change. In addition, I don't think it's immediately clear why we want to observe only the html element for the zoom out mode. Another idea would be using ResizeObserver in a similar way, which I believe we already have one somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that we are observing the html element only. This is because the default for the subtree option of the .observe() method is false.

Set to true to extend monitoring to the entire subtree of nodes rooted at target. All of the other properties are then extended to all of the nodes in the subtree instead of applying solely to the target node.

Therefore this should only be recomputing if attributes change on the html node itself. How often does that realistically happen? Probably quite infrequently.

I could be wrong but it's worth noting.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mentioned this above:

I don't think it's immediately clear why we want to observe only the html element for the zoom out mode.

A comment above would be nice so people won't accidentally break it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. I misinterpreted your comment. Comment always good

attributes: true,
} );

return () => {
observer.disconnect();
zoomedOutObserver.disconnect();
};
}, [ previousElement ] );

Expand All @@ -192,8 +200,16 @@ function BlockPopoverInbetween( {
const observer = new window.MutationObserver( forcePopoverRecompute );
observer.observe( nextElement, { attributes: true } );

const zoomedOutObserver = new window.MutationObserver(
forcePopoverRecompute
);
zoomedOutObserver.observe( nextElement.closest( 'html' ), {
attributes: true,
} );

return () => {
observer.disconnect();
zoomedOutObserver.disconnect();
};
}, [ nextElement ] );

Expand Down
8 changes: 8 additions & 0 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 @@ -69,8 +69,16 @@ function BlockPopover(
);
observer.observe( selectedElement, { attributes: true } );

const zoomedOutObserver = new window.MutationObserver(
forceRecomputePopoverDimensions
);
zoomedOutObserver.observe( selectedElement.closest( 'html' ), {
attributes: true,
} );
Comment on lines +75 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
zoomedOutObserver.observe( selectedElement.closest( 'html' ), {
attributes: true,
} );
// The iframe of the editor canvas may be shifted by JS-based animations
// necessitating a need to recompute the positioning of the toolbar to account
// for the movement. The editor canvas's `html` tag is where the attributes are
// modified during animation so by observing changes here it's possible to force
// the toolbar to recompute as required.
zoomedOutObserver.observe( selectedElement.closest( 'html' ), {
attributes: true,
} );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're really eager with these comments lately. In this case the new code is next to the old code doing the same thing. If the old code was understandable I think the new one is too. The name of the variable is also an indicator of why the observer is there 🤷🏻


return () => {
observer.disconnect();
zoomedOutObserver.disconnect();
};
}, [ selectedElement ] );

Expand Down
Loading