Repository navigation
Navigation: Add a new option that toggles submenus always open - #74653
Merged
Merged
Conversation
scruffian
added
[Type] Enhancement
|
Size Change: +1.67 kB (+0.05%) Total Size: 3.09 MB
ℹ️ View Unchanged
|
scruffian
changed the title
jeryj
force-pushed
the
try/vertical-show-submenu
branch
from
January 16, 2026 15:38
0af0a89 to
128bcb2
Compare
jeryj
marked this pull request as ready for review
January 16, 2026 18:55
jeryj
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar and
spacedmonkey
as code owners
January 16, 2026 18:55
|
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. |
|
Flaky tests detected in 4318e57. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/21266452359
|
scruffian
commented
Jan 19, 2026
| .wp-block-navigation__responsive-container-content { | ||
| // Don't crop the focus style. | ||
| overflow: visible; | ||
| &:where(:not(.disable-default-overlay)) { |
Contributor
Author
There was a problem hiding this comment.
It looks like the only change here is adding this wrapper.
commented
Jan 19, 2026
Contributor
Author
|
One thing to note about this change is that it's independent of the navigation overlay experiment. This code becomes immediately exposed to users and provides an additional option for the block. Personally I think this adds value, but if we are concerned about surfacing this we could consider removing the UI for this change. |
scruffian
force-pushed
the
try/vertical-show-submenu
branch
from
January 20, 2026 12:58
6aebff1 to
fa70921
Compare
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the navigation block's openSubmenusOnClick boolean attribute to a new submenuVisibility attribute with three modes: "hover" (default), "click", and "always". This change enables building navigation menus that match the default overlay design by always showing submenus when using vertical orientation.
Changes:
- Replaced
openSubmenusOnClickboolean withsubmenuVisibilitystring enum ("hover", "click", "always") - Added CSS styling for the new "always" visibility mode with automatic indentation and vertical layout
- Implemented attribute migration hook to convert existing blocks from the old to new attribute
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/block-library/src/navigation/block.json | Removed default value from deprecated openSubmenusOnClick, added new submenuVisibility attribute with enum validation |
| packages/block-library/src/navigation/edit/index.js | Updated UI to use ToggleGroupControl for submenu visibility, added logic to force vertical orientation when "always" is selected |
| packages/block-library/src/navigation/edit/use-migrate-attributes.js | New hook to migrate openSubmenusOnClick to submenuVisibility |
| packages/block-library/src/navigation/index.php | Updated backend logic to handle both old and new attributes for backward compatibility |
| packages/block-library/src/navigation-submenu/index.php | Added rendering logic for "always" visibility mode including conditional aria-expanded attributes |
| packages/block-library/src/navigation-submenu/edit.js | Added open-on-always class when in always mode |
| packages/block-library/src/navigation/style.scss | Added comprehensive CSS for always-visible submenus with proper spacing and layout |
| lib/experimental/overlay-patterns.php | Updated default overlay pattern to use submenuVisibility: "always" |
| test/integration/fixtures/blocks/* | Updated deprecation test fixtures to include explicit openSubmenusOnClick: false |
| test/e2e/specs/editor/blocks/navigation-colors.spec.js | Updated overlay visibility control selector to be more specific |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+27
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useEffect } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * Hook to migrate deprecated attributes to their new equivalents. | ||
| * | ||
| * @param {Object} attributes Block attributes. | ||
| * @param {Function} setAttributes Function to update block attributes. | ||
| */ | ||
| export default function useMigrateAttributes( attributes, setAttributes ) { | ||
| const { openSubmenusOnClick, submenuVisibility } = attributes; | ||
|
|
||
| // Migrate openSubmenusOnClick to submenuVisibility | ||
| useEffect( () => { | ||
| if ( | ||
| openSubmenusOnClick !== undefined && | ||
| submenuVisibility === undefined | ||
| ) { | ||
| setAttributes( { | ||
| submenuVisibility: openSubmenusOnClick ? 'click' : 'hover', | ||
| openSubmenusOnClick: undefined, | ||
| } ); | ||
| } | ||
| }, [ openSubmenusOnClick, submenuVisibility, setAttributes ] ); | ||
| } |
There was a problem hiding this comment.
The new useMigrateAttributes hook lacks test coverage. Similar hooks in this directory such as use-create-overlay have dedicated test files. Add a test file at packages/block-library/src/navigation/edit/test/use-migrate-attributes.js to verify the migration logic from openSubmenusOnClick to submenuVisibility.
Comment on lines
+804
to
+809
| } else if ( value === 'click' ) { | ||
| // When switching to "click", arrow must be shown (needed for clicking) | ||
| newAttributes.showSubmenuIcon = true; | ||
| } else { | ||
| // When switching to "hover", show the arrow to avoid showing the accessibility notice | ||
| newAttributes.showSubmenuIcon = true; |
There was a problem hiding this comment.
When "always" submenu visibility is selected, showSubmenuIcon is set to false but this change is unconditional. If a user had previously set showSubmenuIcon to true, switching away from "always" mode should ideally restore the previous value rather than forcing it to true. Consider storing the previous value or allowing users to independently control this setting when not in "always" mode.
Suggested change
| } else if ( value === 'click' ) { | |
| // When switching to "click", arrow must be shown (needed for clicking) | |
| newAttributes.showSubmenuIcon = true; | |
| } else { | |
| // When switching to "hover", show the arrow to avoid showing the accessibility notice | |
| newAttributes.showSubmenuIcon = true; |
Contributor
There was a problem hiding this comment.
I think this is an edge case, and I'd prefer we opt to set it back to an accessible option (true) than leave it off. If we decide it's worth it to implement storing their previous value, we can do that in a follow up.
| "openSubmenusOnClick": { | ||
| "type": "boolean", | ||
| "default": false | ||
| "type": "boolean" |
There was a problem hiding this comment.
The removal of the default value for openSubmenusOnClick may cause issues with backward compatibility. When blocks don't explicitly define this attribute and submenuVisibility is also undefined, the migration hook in use-migrate-attributes.js won't trigger (since openSubmenusOnClick !== undefined will be false). Consider keeping the default value or ensuring the migration logic handles undefined values properly.
Suggested change
| "type": "boolean" | |
| "type": "boolean", | |
| "default": true |
Contributor
There was a problem hiding this comment.
I chatted with Claude about this, and it seems removing the default (as this PR does) is correct. We should not set a default, and definitely not a true default:
The Full Lifecycle
BEFORE this PR (with "default": false):
1. User creates Navigation block
2. openSubmenusOnClick gets value false (from default)
3. Block is saved to database
4. Serializer skips openSubmenusOnClick: false because it matches the default
5. Database contains: <!-- wp:navigation /--> (no openSubmenusOnClick attribute)
AFTER this PR (loading old block without default):
1. Parser loads block from database
2. No openSubmenusOnClick in saved attributes
3. Parser looks for default in block.json - there isn't one anymore
4. Sets openSubmenusOnClick: undefined
5. Migration check: openSubmenusOnClick !== undefined → FALSE (migration doesn't run)
6. Both openSubmenusOnClick and submenuVisibility are undefined
7. Fallback logic: submenuVisibility || (openSubmenusOnClick ? 'click' : 'hover')
8. Result: undefined || (undefined ? 'click' : 'hover') → 'hover' ✓
Evidence from the Commits
From commit fa70921f5ac:
"The attribute is being retired in favor of submenuVisibility, so we're not adding a default value to prevent serialization."
The test fixture changes confirm this:
- core__navigation.json: Removed "openSubmenusOnClick": false (was never serialized)
- core__navigation__deprecated-v4/v5: Added "openSubmenusOnClick": false (explicitly included in deprecated versions for testing)
You're Right - It Actually Works!
Blocks created before this change:
- Old blocks with default false: NOT saved in DB → Load as undefined → Fallback handles it correctly as 'hover' ✓
- Old blocks with explicit true: Saved in DB → Migration runs → 'click' ✓
- Old blocks with explicit false: Saved in DB → Migration runs → 'hover' ✓
jeryj
reviewed
Jan 20, 2026
| [ getColorClassName( 'background-color', backgroundColor ) ]: | ||
| !! backgroundColor, | ||
| 'open-on-click': openSubmenusOnClick, | ||
| 'open-on-always': submenuVisibility === 'always', |
Contributor
There was a problem hiding this comment.
Should this be open-always instead of open-on-always?
Comment on lines
+804
to
+809
| } else if ( value === 'click' ) { | ||
| // When switching to "click", arrow must be shown (needed for clicking) | ||
| newAttributes.showSubmenuIcon = true; | ||
| } else { | ||
| // When switching to "hover", show the arrow to avoid showing the accessibility notice | ||
| newAttributes.showSubmenuIcon = true; |
Contributor
There was a problem hiding this comment.
I think this is an edge case, and I'd prefer we opt to set it back to an accessible option (true) than leave it off. If we decide it's worth it to implement storing their previous value, we can do that in a follow up.
| "openSubmenusOnClick": { | ||
| "type": "boolean", | ||
| "default": false | ||
| "type": "boolean" |
Contributor
There was a problem hiding this comment.
I chatted with Claude about this, and it seems removing the default (as this PR does) is correct. We should not set a default, and definitely not a true default:
The Full Lifecycle
BEFORE this PR (with "default": false):
1. User creates Navigation block
2. openSubmenusOnClick gets value false (from default)
3. Block is saved to database
4. Serializer skips openSubmenusOnClick: false because it matches the default
5. Database contains: <!-- wp:navigation /--> (no openSubmenusOnClick attribute)
AFTER this PR (loading old block without default):
1. Parser loads block from database
2. No openSubmenusOnClick in saved attributes
3. Parser looks for default in block.json - there isn't one anymore
4. Sets openSubmenusOnClick: undefined
5. Migration check: openSubmenusOnClick !== undefined → FALSE (migration doesn't run)
6. Both openSubmenusOnClick and submenuVisibility are undefined
7. Fallback logic: submenuVisibility || (openSubmenusOnClick ? 'click' : 'hover')
8. Result: undefined || (undefined ? 'click' : 'hover') → 'hover' ✓
Evidence from the Commits
From commit fa70921f5ac:
"The attribute is being retired in favor of submenuVisibility, so we're not adding a default value to prevent serialization."
The test fixture changes confirm this:
- core__navigation.json: Removed "openSubmenusOnClick": false (was never serialized)
- core__navigation__deprecated-v4/v5: Added "openSubmenusOnClick": false (explicitly included in deprecated versions for testing)
You're Right - It Actually Works!
Blocks created before this change:
- Old blocks with default false: NOT saved in DB → Load as undefined → Fallback handles it correctly as 'hover' ✓
- Old blocks with explicit true: Saved in DB → Migration runs → 'click' ✓
- Old blocks with explicit false: Saved in DB → Migration runs → 'hover' ✓
jeryj
reviewed
Jan 20, 2026
| @@ -1,3 +1,3 @@ | |||
| <!-- wp:navigation {"layout":{"type":"flex","orientation":"horizontal"}} --> | |||
| <!-- wp:navigation {"openSubmenusOnClick":false,"layout":{"type":"flex","orientation":"horizontal"}} --> | |||
getdave
reviewed
Jan 22, 2026
left a comment
Contributor
There was a problem hiding this comment.
I'm torn about this.
Testing it it's actually quite nice that things reset and stop you reaching a potentially broken state.
On the other hand it's adding more "magic" logic to maintain and forces our design decisions on to the users.
An alternative for us to consider is adding warning messages inline to the controls which display if an invalid state is enabled.
I'd like to get some more opinions on the best approach. What precedent is there in the wider repo for this kind of automated behaviours?
Comment on lines
+306
to
+314
| // Reset submenuVisibility to default if orientation changes to horizontal while "always" is selected | ||
| useEffect( () => { | ||
| if ( orientation === 'horizontal' && submenuVisibility === 'always' ) { | ||
| setAttributes( { | ||
| submenuVisibility: undefined, | ||
| showSubmenuIcon: true, | ||
| } ); | ||
| } | ||
| }, [ orientation, submenuVisibility, setAttributes ] ); |
Contributor
There was a problem hiding this comment.
Do we really want to be this opinionated. Whilst it "protects" users from a certain scenario it also forces people to accept an decision made by us rather than allow them to configure and attempt it themselves.
Let's think carefully about this.
Contributor
There was a problem hiding this comment.
This opinionated setting isn't new, fwiw. Here's an example:
Screen.Recording.2026-01-22.at.8.25.39.AM.mov
- Start from Show on click = false
- Show Icons = false
- Switch to Show on click = true
- Show Icons becomes true
- Switch to Show on click = false
- Show Icons remains true
The Icons are fine other than the centered option. If you center the menus, icons are not styled correctly, so we decided to remove it rather than fix that state for now. Fixing it will likely mean adding markup, which feels riskier to bundle into this PR.
Comment on lines
+40
to
+46
| it( 'should fall back to "hover" when both submenuVisibility and openSubmenusOnClick are undefined', () => { | ||
| expect( getSubmenuVisibility( {} ) ).toBe( 'hover' ); | ||
| } ); | ||
|
|
||
| it( 'should handle empty attributes object', () => { | ||
| expect( getSubmenuVisibility( {} ) ).toBe( 'hover' ); | ||
| } ); |
Contributor
There was a problem hiding this comment.
Seems like duplication? Why not add specific values such as null or undefined to actually stress test the logic?
Contributor
There was a problem hiding this comment.
I don't think these tests are actually very useful. The old one didn't have coverage. I'm fine removing these for now.
commented
Jan 22, 2026
Contributor
The thing I really don't like about not having the "magic" logic is this flow: Screen.Recording.2026-01-22.at.1.02.54.PM.movAs a user, I'd be annoyed that I got to a warning state on an unrelated toggle when interacting with the Hover/Click/Always options. The correct fix for this is to address showing the arrows when on the always option. If we have that working, then we don't need to have this "magic" logic. Unfortunately, I think that problem will require adding HTML, which I'm loathe to do.
The existing component had an automated value setting for the show icon option already: Screen.Recording.2026-01-22.at.8.25.39.AM.mov |
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
added 11 commits
January 22, 2026 15:56
jeryj
force-pushed
the
try/vertical-show-submenu
branch
from
January 22, 2026 21:56
754d5cf to
073f1a7
Compare
The show submenu icon rendrs the icon within a button, and that button has interactivity. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
commented
Jan 23, 2026
Contributor
Author
|
This is looking really good to me. I think we should bring it in. I would approve it, except that it's my PR! |
This was referenced Jan 23, 2026
3 of 6 tasks
This was referenced Feb 11, 2026
commented
Jun 18, 2026
Contributor
|
Is the UI supposed to look like this with three headers in a row (Display > Submenus > Submenu Visibility) ? |
commented
Jun 25, 2026
Contributor
|
@talldan - It's expected based on how it all stacks up. It's the Submenus section, and I think we need to keep that to help define the Show arrow option. We could move the Show arrow up to above Submenu Visibility to break it up more. Would that help? |
This file contains hidden or 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
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?
Migrates
openSubmenusOnClickto asubmenuVisibilityattribute to the Navigation block with three modes:Part of #74590
This adds a block deprecation to a v7 of the navigation block.
Why?
The default navigation overlay for a site is built via CSS and does not have a user-controlled UI for editing it. Now that we have a customizable navigation overlay template, we need to offer a way to build a menu that looks like the default overlay menu.
How?
submenuVisibilityand migratesopenSubmenusOnClickto use this new attribute.We also evaluated other options for having an editor canvas-representation of the overlay before landing on this route:
Other notes:
Testing Instructions
Screenshots or screencast