Remove most uses of isNavigationMode in contentOnly logic #72043
Conversation
tellthemachines requested review from andrewserong, getdave, jeryj, ramonjd, scruffian and talldan tellthemachines requested review from ajitbohra and fabiankaegy as code owners 
| Size Change: -432 B (-0.02%) Total Size: 1.96 MB
ℹ️ View Unchanged
|
| Hmm, something I did here caused all blocks in posts to go into contentOnly mode (which is why the e2e are failing 💀) Update: I think it's fixed now. |
| 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. |
I think that can all be removed if we're removing nav mode. Should be quite a lot of code. Happy for it to be in another PR if you want to split things up. |
tellthemachines force-pushed the try/remove-nav-mode-useage branch from 2d0495e to f9a7454 Compare 
| What's the best way to test here? With the contentOnly experiment on? Write mode or none of them? Those failing e2e tests are pretty flakey when I run them locally 🙃 For the editor > various > write design canvas tests, the editor canvas is completely blank with I run them in the playwright UI 🤔 |
Best way to test is with contentOnly patterns experiment - make sure it works as expected - and also with no experiment on, nothing should have changed. The goal is to remove the write mode toggle/experiment, so it doesn't matter whether it works or not! The write/design tests need removing, or maybe reworking into contentOnly pattern tests (I'm not sure we have any of those yet?) Edit: actually it might make things easier if I remove the write mode experiment in this PR. I'll try that. |
Gotcha. I guess that some of them might be relevant for selecting child blocks. Maybe we can disable them/migrate them to a new suite. 🤔 |
| Flaky tests detected in d8c39ce. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18303264792
|
Actually, looking at how the experiment flag is tied up with That way, this PR just removes the checks in components that are also used when patterns are in contentOnly mode. There are a few small changes to behaviour when just the patterns experiment is on, such as ensuring "copy" and "cut" options don't appear on contentOnly blocks inside patterns, that make the patterns experiment more consistent with our contentOnly goals. This PR is now ready for review! |
| */ | ||
| const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | ||
| | ||
| test.describe( 'Write/Design mode', () => { |
There was a problem hiding this comment.
I've disabled these for now instead of removing them because I think we can rewrite them to apply to contentOnly patterns instead. It might be better to wait until the feature is a bit more stable to do that though!
| // we expect 4 items in the options menu | ||
| await expect( optionsMenu ).toHaveCount( 4 ); | ||
| // we expect 2 items in the options menu: Duplicate and Delete. | ||
| await expect( optionsMenu ).toHaveCount( 2 ); |
There was a problem hiding this comment.
This is an expected change because patterns in contentOnly mode no longer have "cut" and "copy" options.
andrewserong left a comment
andrewserong left a comment There was a problem hiding this comment.
Nice work looking to remove this!
I'm curious about the changes to cut and copy, and to removing the additional formatting options. Are they intentional removals, in the sense that we'd like content only mode to be simpler than it is on trunk? For some of the cases where we're removing the isNavigationMode check, would it be better to remove the whole check altogether rather than tying it to content only mode (which is a little more restrictive than what we currently have)?
I'm mostly wondering whether in some of these cases, it'd be better to allow the behaviour in all cases, rather than removing them. For example, for copy and cut, we already allow duplicate, add before/after, and delete, so if they're available, I wasn't sure why we'd remove copy and cut as well 🤔
Similarly, why do we want to restrict the additional formatting options like strikethrough, subscript and superscript, etc. Does it feel strange to have to click to edit a pattern to perform some of those formatting options?
| // Shifts the toolbar to make room for the parent block selector. | ||
| const classes = clsx( 'block-editor-block-contextual-toolbar', { | ||
| 'has-parent': showParentSelector, | ||
| 'is-inverted-toolbar': isNavigationMode && ! hasFixedToolbar, |
There was a problem hiding this comment.
You might already be imagining a different PR for it, but if this classname is removed, we can probably remove the CSS here, too?
There was a problem hiding this comment.
Yes that makes sense!
Yes, my reasoning is that we want contentOnly mode in patterns to function along the same lines as it was to with the "write mode" toggle. Cut/copy and formatting were originally removed for write mode in #71063 and #71058 and there's no reason to assume the requirements changed 😅 (though requirements haven't always been very clear for this feature, so I guess it's possible they might change) There was also the intention for write mode to disable moving blocks around (#67408), though it was never successfully implemented. I think it makes sense if we consider that placement/layout is part of design. From that perspective, "copy" "cut" and "paste" are similar operations to "move", so I think that was why they were removed. |
| Nice one, thanks for clarifying the thinking there @tellthemachines, that all sounds reasonable to me! |
Are there any exceptions? For some patterns the first "content" block (para/heading) has the context menus:   🤔 |
talldan left a comment
talldan left a comment There was a problem hiding this comment.
Thanks for handling this, it's testing well for me. 🎉
The only thing I wonder is if the experiment toggle should also be removed as part of the PR since it leaves write mode unusable. Not a big issue, but it might need to be addressed prior to this code shipping in a gutenberg release.
@ramonjd How do we repro that? I do see Duplicate / Add Before / Add After / Delete, which seems like a possible bug, but I think it's also an issue in trunk. I'm not seeing the other options from your screenshot: |
I could only reproduce with those two patterns in the screenshot, e.g.,. "Services, 3 columns". I don't trust my env. I'm seeing few things that others aren't today. Thanks for double checking. |
I was initially going to remove it but then decided not to (see here) I plan to follow this one up with a second PR to remove the experiment, the toggle and any remaining selectors not in use. |
Hmm if the parent is a section root we allow inserting blocks there. The difference I see between blocks that get the menu and not is they're at the root of the pattern container, so it might be that. I guess that doesn't make sense with contentOnly patterns so we might want to remove the functionality, but better addressed as a separate PR perhaps. |
| Thanks for the reviews, folks! I'm going to go ahead and merge this so I can start working on the follow-ups. |
…s#72043) Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
What?
Starts addressing #71555.
This PR only removes the
isNavigationModecheck in places where we want the logic to apply tocontentOnlymode in general:block toolbar options menu
Removes "copy" and "cut" options for blocks in contentOnly mode (the options menu only appears on children of content blocks, such as: Images inside a Gallery; List items; Button; Paragraphs inside a Cover. An example from the "Call to action with grid layout with products and link" TT5 pattern:
block toolbar
rich text
Reduces editing tools to essential only in contentOnly mode so e.g. a Paragraph inside a pattern only has basic tools now:
isSectionBlock
Removes
isNavigationModefrom the logic (shouldn't be any difference to contentOnly mode in patterns).canInsertBlockTypeUnmemoized and canRemoveBlock
Removes
isNavigationModefrom conditions for allowing insertion/removal in contentOnly mode.Site logo and Site title
Removes
isNavigationModefrom conditions for displaying editing tools in contentOnly mode.What this PR doesn't address
getBlockEditingModeandderivedNavModeBlockEditingModes(maybe we can remove this altogether? Or do we want to transfer any of that logic to contentOnly patterns?)DisableNonPageContentBlocks(again, not quite sure what we want to keep or chuck out here?)I think all these things can be addressed as follow-ups to keep the PRs as easy to review as possible :)
Testing