Disallow dropping outside section root in Zoom Out mode #64500
Merged
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
This comment was marked as resolved.
This comment was marked as resolved.
getdave
added
[Type] Bug
getdave
changed the title
|
Size Change: +91 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
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. |
MaggieCabrera
approved these changes
Aug 14, 2024
There was a problem hiding this comment.
Cool idea! Overall it looks like a decent approach to me. One question that springs to mind: will we ever use zoomed out mode in a way where we want to be zoomed out but still allow granular drag and drop within headers and footers? From the PR description, I get the sense that the answer is "no". And even so, if we wanted to allow it at some point in the future, it looks like it'd be pretty straightforward to update this 👍
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
Comment on lines
326
to
328
| const { sectionRootClientId } = unlock( getSettings() ); | ||
|
|
||
| const isZoomOutMode = __unstableGetEditorMode() === 'zoom-out'; |
There was a problem hiding this comment.
Since these are grabbing values, I was wondering if these should either be:
- Retrieved in the traditional way of a
useSelectcall that returns an object that we then destructure to assign values, or: - Move the assignment of these values to the callback within the throttled function below?
The useBlockDropZone hook is a little unusual in that we get references to a bunch of functions that are later called in the throttled function, so I was wondering if it's good to try to keep to the method calls to happen either in a useSelect callback, or within the throttled callback, but not at the root of the hook. I think the main thing to look for here is whether we're currently calling getSettings() and __unstableGetEditorMode() more frequently than we need to 🤔
There was a problem hiding this comment.
Yes I agree thanks for flagging. Performance was my next concern if folks felt this was on track.
I've updated so that the comparisons are only called within the throttled callback. The callback dependencies have increased by x2 but one is a stable function reference and the other relates to whether or not we are in zoom out mode.
| @@ -321,6 +323,10 @@ export default function useBlockDropZone( { | |||
| stopDragging, | |||
| } = unlock( useDispatch( blockEditorStore ) ); | |||
|
|
|||
| const { sectionRootClientId } = unlock( getSettings() ); | |||
There was a problem hiding this comment.
I see the performance and unit tests seems to be failing with an error Cannot unlock an object that was not locked before.. Could that be related to this, or is it an unrelated failure?
There was a problem hiding this comment.
There's also some discussions about it here - #64141.
There was a problem hiding this comment.
Thanks @talldan. I've responded there. We need to retain the block editor setting, but it seems the implementation of keeping it private might need some work.
There was a problem hiding this comment.
I think we should look at @tjcafferkey's PR to see if we can improve the implementation of the private setting.
Co-authored-by: Andrew Serong <[email protected]>
getdave
force-pushed
the
fix/zoom-out-drop-only-within-section-root
branch
from
313dfc7 to
4e5de94
Compare
|
Flaky tests detected in 40c98ee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10402910436
|
There was a problem hiding this comment.
Thanks for the updates! Code-wise this is looking good to me, however in testing I found that in my template, for some reason I could only drag to further down the template than expected. It's a little hard to explain, but a good part of the top part of my template, which isn't part of the Header could no longer be dragged to:
2024-08-16.11.27.58.mp4
My template hierarchy is a bit all over the place, but here's a rough view in case it helps. There are a bunch of root level paragraphs in the template, and I wasn't able to drag to before or after those paragraphs with this PR applied, but could on trunk:
Are you able to replicate that? It's fairly likely I might have made a mistake in my testing somewhere 🙂
Edit: is it because my root blocks are outside of a <main> tag? I.e. each of these root blocks are before the Group block that is set to the <main> tag 🤔. I think I expected to be able to drag to anywhere within the root of the template, but just not above the Header and not below the Footer.
Yeh that will be it. In a template. Here's the logic where we determine which block is allowed to receive "drops": function getSectionRootBlock() {
if ( renderingMode === 'template-locked' ) {
return getBlocksByName( 'core/post-content' )?.[ 0 ] ?? '';
}
return (
getBlocksByName( 'core/group' ).find(
( clientId ) =>
getBlockAttributes( clientId )?.tagName === 'main'
) ?? ''
);
}So it's expected that anything outside of |
|
I feel like this is in a good place to merge. We have a wider issue of managing blocks outside the |
andrewserong
approved these changes
Aug 19, 2024
There was a problem hiding this comment.
I feel like this is in a good place to merge. We have a wider issue of managing blocks outside the
tag but I feel that should be a separate PR to avoid this one ballooning in size.
I agree. This PR is nice and simple, and there's a good inline comment, so it should be straightforward to change in follow-ups if need be. Also, since Zoom Out is still behind an experiment, this seems safe to me.
LGTM! 🚀
|
Thanks @andrewserong. This bug is affecting my work in other PRs so I agree we need to get this merged. |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
) * Conditionalise Zoom Out dragOver to account for section root * Fix comment typo Co-authored-by: Andrew Serong <[email protected]> * Refactor to only call functions inside throttled callback --------- Co-authored-by: Andrew Serong <[email protected]>
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?
Disables the ability to drop patterns outside of the section root when in Zoom Out mode.
Related to #63896
Why?
Fixes an Issue in Site Editor where it was possible to drag and drop directly on the header/footer of the template. This causes knock on effects on the appearance of / targetting of dropzones when using drag and drop in Zoom Out mode.
The issue is best illustrated when considering the site editor.
In this context the "section root" is the
<main>tag. Elements outside of that "root" include (for example)When dragging and dropping patterns it should not be possible to drop over these elements outside of the root, as the target area for the template content is the
<main>element.Note: when using Zoom Out in the Post Editor (i.e. when creating a new Page) the section root is "empty" because the "template" is the page in question. We don't have headers or footers. The user is assembling a page and therefore should be allowed to drag patterns anywhere on that page.
How?
Conditionalise the
dragOverhandler with a special case for Zoom Out mode which checks whether therootClientIdof the dropzone (i.e. thetargetRootClientId) matches thesectionRootClientId(e.g. the<main>element).If they do not match then the drop zone is not within the section root and thus the operation should not be allowed.
Testing Instructions
trunk).Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-08-14.at.12-47-48.mp4