-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Edit Mode: Prevent editable text selection on first click #65702
base: trunk
Are you sure you want to change the base?
Conversation
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: -106 B (-0.01%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will work well.
I'm noticing some inconsistencies in the interactions though.
For example, I can click directly on a Heading block and it will drop me into editing mode on that block immediately. Other blocks such as image seem to better respect the click once to activate/unlock section, click again to activate the specific block.
Personally I like the following flow:
- click section once - retains standard interaction model of Zoom Out but with a "flash" to highlight the editable blocks
- click section again - unlock/active the section for editing. All content blocks are editable.
- click outside section - section is locked again.
Screen.Capture.on.2024-09-27.at.11-36-40.mp4
@getdave the behavior you found I think happens because toggling zoom out also toggles design mode 🤷🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this behavior, makes it possible for the zoom out mode to be removed, makes code easier to read too imo.
We need some design feedback if this should go as a behavior now for zoom out, given it's being shipped.
I added another commit that I would for it to be reviewed:
|
Testing this makes it more obvious we need to do some merging of modes before the next release, because one can independently change mode from tools and if one does that in zoom out, you cannot get back to zoom out. Here is a video using 6.7 nightly (the same works with GTB trunk) overlapping-mode-controls.mp4 |
What's missing in the current PR? I think we can land it no? |
Its still a little unpredictable. I can still select inner blocks on first click for example (see video) select-bug.mp4 |
647401b
to
90dcc61
Compare
@@ -42,12 +45,9 @@ export function useShowBlockTools() { | |||
editorMode === 'edit' && | |||
isEmptyDefaultBlock; | |||
const isZoomOut = editorMode === 'zoom-out'; | |||
const _showZoomOutToolbar = | |||
isZoomOut && | |||
block?.attributes?.align === 'full' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should not remove this condition in this PR, as this leaves blocks with no full with with no toolbar at all. Also #65627 is likely to land and also fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getdave the behavior you found I think happens because toggling zoom out also toggles design mode 🤷🏻
I think it happens because there is an inconsistency in the PR. The PR title specifically states the goal is to "prevent editable text selection on first click".
My review above shows there is an inconsistency in the behaviour which @SaxonF has also found. I checked out the PR again to make sure I could still replicate it and sure enough...
Screen.Capture.on.2024-09-30.at.09-39-16.mp4
What's missing in the current PR? I think we can land it no?
I think we need to fix this before the PR is merged.
Ok, so it turns out that the inconsistency is actually a bug that already exists in trunk and that @ellatrix is fixing here. #65662 Basically, if you have a "stack" or "grid" and a paragraph within it, selecting the "stack" or "grid" automatically focuses the paragraph. It's visible here but it's actually unrelated. I'm going to review the other PR and try to land it. |
Works pretty good. One caveat that we need to account for: now that you can select the nested blocks to edit, you can double-click to select all the text contents of a block, but that disengages zoom out (due to double-click to exit zoom out). Can be a follow-up. CleanShot.2024-09-30.at.09.41.53.mp4 |
Only for flex groups in Safari, btw, not all groups. I guess depending on the theme that may be all groups :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great now, with one exception: the section root is 1st selected. Ideally in zoom out clicking a section immediately selects the section, as it is now the 1st click always selects the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a solid move forward.
One thing I think we'd need to improve is when editing a Page we should allow for editing the Post Title in Write
mode. It feels unusual to have that restricted - especially when you are not zoomed out.
But that could easily be a follow up.
Can you share more details here, For me, sections are selected first (and not the root of the sections) |
Noticed that I was able to turn on Design mode but that it didn't respond when in zoom out. I think we should disable this option if folks can't switch to this while in zoom out: disable.being.able.to.switch.to.design.mov |
Yes, this is already the case in trunk if I'm not wrong. I think that's one of the questions that we want to answer as a follow-up:
|
Got it. In this case, I think if we allow switching between modes while in zoom out for now, we should also show the full controls when in Design mode with this PR. Don't mean to distract though if this is outside the scope. |
two-click-select.mp4 |
@draganescu Would you mind sharing the content of that page (or is it some existing page in 2025 or something)? |
803b908
to
baf4093
Compare
baf4093
to
b33cdf3
Compare
Flaky tests detected in b33cdf3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11461263367
|
Yeah the exacts steps to reproduce would help, I can't reproduce it either. |
@ellatrix good catch, it's fixed now and yes in trunk, it's not possible to select children of sections. |
@@ -63,7 +62,6 @@ function useShallowMemo( value ) { | |||
*/ | |||
export default function useNestedSettingsUpdate( | |||
clientId, | |||
parentLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: why is this no longer changes and what required the changes to the selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I did a refactor to how "templateLock" works. Instead of doing the "inheritance" of template lock when setting the templateLock in the state, I moved the inheritance to the getTemplateLock selector instead.
The reason is that we want to know precisely what container have an explicit templateLock and what container inherit templateLock. Because the ones that have an explicit "contentOnly" templateLock are considered sections but not the ones within them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at some point we may want to investigate these selectors (and the other few similar recursive selectors), I suspect they're not performant, which can be seen in the difference between the "preview" block selector and the normal one.
Yes, I think we should probably remove the double click now. Can do either here or in a follow-up. |
IMO we should remove it entirely. I think it's inconsistent with other interactions we have around blocks tbh. |
With the ability to edit text directly when zoomed, it feels like double click might now be redundant. Just noting that this is not the case with WP 6.7, as the Zoom Out experience in |
Thanks for sharing the content by DM @draganescu I managed to fix the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was so zoomed into zoom out that I missed that this PR "does the following in both modes:" - both. I would like to offer my negative opinion about having to click twice when edit mode is enabled and the user is in default view default scale. It's clumsy and serves no purpose.
Things that are editable should be immediately editable by activating them (click, focus). We are expanding the UX in pattern overrides to the whole canvas for no benefit - but at least in pattern overrides it serves the purpose that in one of those patterns only some content is editable. But in content only write mode all content is editable - only content, but all of it.
There is no point in applying a double click drill down UX to the whole canvas in default view default scale in write mode and it seems it's done here for some ideal of consistency.
Not even consistency is served though, since views, like modes, afford new affordances. To give an example a shape in a 3D view has three handles to drag on, but seen from any of the 2D views it only has two handles. And it makes sense, we don't need a 3rd handle in a 2D view, because it beats the purpose of the 2D view. Just like maybe we can have double click drill down UX in zoomed out view at small scale to favor interacting with larger areas of content.
Can we do what this PR does but only in zoomed out view? Or is this a no go? Or am I missing something fundamental?
Reported here: #65750 |
Related #65298 and #64197
What?
This PR starts the unification of "zoom-out" interaction model with the interaction model of "edit" tool/mode.
So it does the following in both modes:
For me, it does test well in both modes but would appreciate more 👀 to confirm that this behavior looks good and whether I missed some nuances.
Closes #65736
Testing Instructions
1- Insert some patterns in the post editor.
2- Enable zoom-out
3- Notice that you click sections to select them
4- Then you can click content blocks to edit text
5- Do the same test in "edit" mode.