Fix moving block popover when zoomed out #65981
Closed
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
draganescu
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +54 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
getdave
reviewed
Oct 9, 2024
There was a problem hiding this comment.
In my initial testing this seems to work well.
I think it would be good to add some context via comments in each situation where we're adding an observer as this code could be quite confusing to a future contributor.
Otherwise this does resolve the issue.
One concern - I'm not sure about performance implications of mutation observers at a root level. Is it just the html element that is observed (rather than all it's child nodes)?
Comment on lines
+75
to
+77
| zoomedOutObserver.observe( selectedElement.closest( 'html' ), { | ||
| attributes: true, | ||
| } ); |
There was a problem hiding this comment.
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, | |
| } ); |
There was a problem hiding this comment.
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 🤷🏻
… iframe, which is the element that will have its attributes change if the workspave is shrunk
draganescu
force-pushed
the
fix/moving-block-popover-zoom-out-editor-chrome
branch
from
68650fb
to
1ad4134
Compare
|
Although adding one more obeserver may fix the issue, I believe it's not fixing the root cause for the glitch. I left a comment with my findings in the related issue |
kevin940726
reviewed
Oct 10, 2024
| const zoomedOutObserver = new window.MutationObserver( | ||
| forcePopoverRecompute | ||
| ); | ||
| zoomedOutObserver.observe( previousElement.closest( 'html' ), { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah I see. I misinterpreted your comment. Comment always good
|
I am not too thrilled about adding such an observer but unless someone refactors how zoom out works foundationally (how do we scale the viewport at the size it had before scaling) then this fix could be fine for 6.7 and fixing the UI displacement bug. |
|
Overall if a more comprehensive solution cannot be found, then this could act as a patch. But we'd need to commit to fixing the root of the Issue in the 6.8 cycle. |
|
Noting that in my testing the bug will be resolved by #66034. However, we need to be confident that that PR is stable enough for 6.7. |
|
This is outdated by #66034 🚀 |
|
Thank you for all your work on these vertical toolbar issues @draganescu. It's very much appreciated even if they didn't end up making it into the release. |
getdave
removed
the
Backport to WP 6.7 Beta/RC
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?
Fixes #65188
Why?
The anchor of block popover for both the toolbar and the inserter only recomputes if the slection has any transform, but when zoomed out the whole container is transformed and the anchor of the block popover remains behind.
How?
Added more observers 🤷🏻
I am thinking we could have some sort of event to subscribe to?
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
fixed-moving-block-anchor.mp4