Render custom overlay template parts in Navigation block (behind experiment) #73967
Conversation
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
|
Size Change: +648 B (+0.02%) Total Size: 3.08 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 843923b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20167675151
|
jeryj
reviewed
Dec 12, 2025
Contributor
jeryj
left a comment
There was a problem hiding this comment.
I'm concerned about experimental overlay markup leaking out to the main canvas for people without the experiment on. Could the overlay specific stuff be more batched? Or have a default function that prefixes the functions and individual changes to make sure they don't get applied if the experiment is off?
| * @param array $attributes The block attributes. | ||
| * @return WP_Block_List Returns the inner blocks for the overlay template part. | ||
| */ | ||
| private static function get_overlay_blocks_from_template_part( $overlay_template_part_id, $attributes ) { |
Contributor
There was a problem hiding this comment.
I haven't worked with template parts much. This function is getting the blocks for a template by ID with sensible fallbacks, right? I'm surprised this isn't a util already.
| $responsive_container_classes = array( | ||
| 'wp-block-navigation__responsive-container', | ||
| $is_hidden_by_default ? 'hidden-by-default' : '', | ||
| $has_custom_overlay ? 'has-custom-overlay' : '', |
Contributor
There was a problem hiding this comment.
'has-custom-overlay' feels a little odd of a name since the overlay html is not within this code. Should we name it by intention, like 'disable-default-overlay'?
Contributor
There was a problem hiding this comment.
I'm rethinking this. I think the base navigation with default overlay (no custom overlay) should stay the same and not get any new classes. That way we're less likely to accidentally raise the specificity of things.
| // When menu is closed (not open), show desktop container and hide overlay container. | ||
| &:not(.is-menu-open) { | ||
| .wp-block-navigation__responsive-container-content { | ||
| .wp-block-navigation__desktop-container { |
Contributor
There was a problem hiding this comment.
This menu might display on mobile devices too (no overlay, always show menu), so we should avoid "desktop" as the name. I'm trying to think of better names but struggling to find one I really like:
- default
- full
- text
- inline
- canvas
- page
- None? Just wp-block-navigation__container and have the overlay be the odd one out?
Contributor
|
I'm able to get the
|
Contributor
Author
|
Thanks for review. I'm going to take another look at this to see if we can isolate the experiment work further to avoid leakages. |
843923b to
e5a5b71
Compare
Contributor
|
@getdave I rebased this with trunk to bring in the other recent overlay template changes |
Contributor
|
Why do we still output the HTML for the navigation block default overlay when the navigation has a custom overlay? I don't understand how the current route is more protective than if the experiment is on, and there's an overlay, not outputting unused html? |
- Add get_overlay_blocks_from_template_part() helper to retrieve blocks from overlay template parts - Add get_template_part_blocks_html() to render template part blocks without navigation container wrapper - Modify get_responsive_container_markup() to render both desktop and overlay blocks in separate containers - Add CSS rules to show/hide desktop and overlay containers based on breakpoint and menu state - Ensure overlay container is full width - Maintain backward compatibility when no overlay is selected
- Add has-custom-overlay class to responsive container when overlay template part is selected - Exclude default padding from responsive-container when custom overlay is present - Exclude default padding-top from responsive-container-content when custom overlay is present - Exclude default margin-top from responsive-dialog when custom overlay is present - Exclude default background and text colors when custom overlay is present - Simplify CSS structure by removing duplicate selectors
…lock - Add has_navigation_overlay_close_block() helper to check for core/navigation-overlay-close block - Skip checking inner blocks of navigation blocks for optimization - Conditionally render default close button only when overlay doesn't have its own close block - Hide default close button when core/navigation-overlay-close block is present in overlay template part
- Add block_core_navigation_add_directives_to_overlay_close() function following the same pattern as block_core_navigation_add_directives_to_submenu() - Use WP_HTML_Tag_Processor to find and add directives to the overlay close button - Add data-wp-on--click directive to enable closing the overlay menu - Apply directives only when overlay close block is present and navigation is interactive
- Add wp-block-navigation__desktop-container to gap: inherit rule - Ensures navigation block continues to inherit block gap CSS after adding desktop container wrapper - Overlay container excluded from gap inheritance as it controls its own spacing - Fix indentation issues in padding rules
- Add disable_overlay_menu_for_nested_navigation_blocks() function to recursively find and modify navigation blocks - Set overlayMenu attribute to 'never' for any navigation blocks found in overlay template parts - Prevents nested overlays (inception problem) when navigation blocks are included in overlay content - Applied to both theme file and database post code paths
- Add top and right spacing to wp-block-navigation__responsive-container-close when parent has has-custom-overlay class - Use clamp value matching root padding pattern to position button away from overlay edges - Only applies when default close button is rendered within a custom overlay template part
- Add is_overlay_experiment_enabled() helper to check experiment status - Make desktop container wrapper conditional on experiment being enabled - Maintain backward compatibility: without experiment, desktop navigation markup remains unchanged - Prevents CSS/layout issues for users without the experiment enabled - Add comprehensive comment explaining why this is gated
* sets default negative values for custom overlay experiment in one location * consolates code to make it harder to accidentally bleed experiment code * changes desktop class to inline so it's not screen size specific
The wrapper could cause issues with how people target their css. Sibling + selectors could break if we add a new container. To avoid this, we use the existing markup and output the navigation custom overlay at the end of the block.
198ac2a to
081cabc
Compare
If the current color matches the background of the page (common) then the focus ring is not visible. Let it default to the color set by the page instead of overriding it.
On the default overlay, there are no extra focusable elements or duplicate menus. On a custom overlay, the existing menu is hidden and the custom overlay has additiona focusable elements. This was causing a problem for focus transfer and focus trap in the modal, as it was not checking for element visibilty when determining focusability.
This aria-hidden was not tied to the overlay state, so it was fixed as hidden. Also, this should be unnecssary, as display: none will already hide it from the accessibility tree.
scruffian
reviewed
Jan 6, 2026
| * @param WP_Block_List $blocks The list of blocks to check. | ||
| * @return bool Returns true if a navigation-overlay-close block is found. | ||
| */ | ||
| private static function has_navigation_overlay_close_block( $blocks ) { |
Contributor
There was a problem hiding this comment.
I wonder if we should make this a general function for detecting whether a block contains inner blocks - we already do something like that in several places.
Contributor
Author
There was a problem hiding this comment.
Yes. I would suggest we do that in a followup as a code quality task
Contributor
There was a problem hiding this comment.
| $overlay_blocks_html = ''; | ||
| $custom_overlay_markup = ''; | ||
|
|
||
| if( $is_overlay_experiment_enabled ) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if( $is_overlay_experiment_enabled ) { | |
| if ( $is_overlay_experiment_enabled ) { |
| // Show default close button for all responsive navigation, | ||
| // unless custom overlay has its own close block. | ||
| if ( ! $has_custom_overlay_close_block ) { | ||
| $close_button_markup = sprintf( |
Contributor
There was a problem hiding this comment.
What do you think about using an overlay close block here instead of the old close button markup?
Contributor
There was a problem hiding this comment.
This would also mean that we could keep the close button inside the overlay
Contributor
There was a problem hiding this comment.
That may be a safer option. Otherwise, we run into issues like this. (Right aligned menu with floating fixed position button). By auto-inserting the overlay close block, we make sure it's positioned outside of the content flow.
Contributor
There was a problem hiding this comment.
Let's do it in a follow-up though. I think we should bring this in sooner.
Contributor
|
I'm wondering how users will set a custom background color for their overlays... |
Contributor
Setting a group block with 100VH as the root block works to have an overlay background color |
We were using three separate functions that were doing the same thing - searching the block tree for the presence of a block. This refactors that into one generic function and implements it for each.
jeryj
reviewed
Jan 6, 2026
jeryj
reviewed
Jan 6, 2026
scruffian
reviewed
Jan 7, 2026
| function block_core_navigation_block_contains_core_navigation( $inner_blocks ) { | ||
| foreach ( $inner_blocks as $block ) { | ||
| if ( 'core/navigation' === $block->name ) { | ||
| function block_core_navigation_block_tree_has_block_type( $blocks, $block_type, $skip_block_types = array() ) { |
Contributor
There was a problem hiding this comment.
Could this have a more generic name?
Contributor
There was a problem hiding this comment.
I wanted to do 'block_tree_has_block_type' but the linter says I have to prefix with the navigation stuff. Maybe we can move it outside of the navigation block for wider use?
Contributor
There was a problem hiding this comment.
yeah i think we should, but maybe in a followup
getdave
commented
Jan 7, 2026
Comment on lines
11
to
19
| public static function setUpBeforeClass(): void { | ||
| parent::setUpBeforeClass(); | ||
| // Ensure the navigation block functions are loaded. | ||
| // Only load if the specific function we need doesn't exist yet. | ||
| if ( ! function_exists( 'block_core_navigation_block_tree_has_block_type' ) ) { | ||
| require_once __DIR__ . '/../../packages/block-library/src/navigation/index.php'; | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is likely the source of test failures. This seems like an unusual pattern? Why is it needed? Can we simplify?
Contributor
There was a problem hiding this comment.
Good catch. I don't think it's necessary. AI written test that I missed it added setup.
Contributor
There was a problem hiding this comment.
I don't think we need the comment if nothing is happening there.
Fix test failure caused by directly requiring source file which bypassed the build system. The test now correctly uses prefixed function names from built files, following the build system function prefixing pattern. Changes: - Remove direct require of source file (functions loaded via lib/blocks.php) - Update function calls to use gutenberg_ prefixed names - Update @Covers annotations to reference prefixed functions See: docs/contributors/code/build-system-function-prefixing.md
|
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. |
jeryj
approved these changes
Jan 7, 2026
Contributor
jeryj
left a comment
There was a problem hiding this comment.
I think this is safe to bring in now, and iterate on. I compared the HTML of a non-overlay menu with and without the experiment on and they matched except for the aria label. We should address this in a follow-up.
aria labels get incremented within the overlay. So, if an aria-label used to say "Header Menu 2" it might now say "Header Menu 4" as it counts the menus within the overlay as well. We only want those to increment if they have an overlay (not set to overlay="never"). I looked into this for a bit today and I think that code needs improvement in general. I can apply a bandaid fix but it will be confusing based on the current code structure.
scruffian
reviewed
Jan 8, 2026
|
|
||
| $is_hidden_by_default = isset( $attributes['overlayMenu'] ) && 'always' === $attributes['overlayMenu']; | ||
|
|
||
| // Set-up variables for the custom overlay experiment. |
Contributor
There was a problem hiding this comment.
This function is getting quite long. We might want to consider breaking this into smaller functions in a followup.
scruffian
approved these changes
Jan 8, 2026
Contributor
scruffian
left a comment
There was a problem hiding this comment.
This is looking good to me, let's bring it in.
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
This PR implements rendering of custom overlay template parts in the Navigation block, addressing a significant part of #73081. When a custom overlay template part is selected, the block now renders both the standard desktop navigation blocks and the overlay template part blocks, with proper show/hide behavior based on breakpoint and menu state.
Why
This enables users to create custom overlay designs for mobile navigation while maintaining the standard desktop navigation. The overlay template part can contain any blocks, allowing for fully customized mobile menu experiences.
How
get_overlay_blocks_from_template_part()helper to retrieve blocks from overlay template partsget_responsive_container_markup()to render both desktop and overlay blocks in separate containerscore/navigation-overlay-closeblock as alternative close buttonMarkup Changes and Backwards Compatibility
Important: The desktop container wrapper (
wp-block-navigation__desktop-container) is gated behind thegutenberg-customizable-navigation-overlaysexperiment to ensure backward compatibility.<div class="wp-block-navigation__desktop-container">to enable proper show/hide behavior with overlay containersThe experiment gate prevents the markup change from affecting trunk until the feature is ready for general availability. All other functionality (overlay rendering, close button handling, etc.) only activates when an overlay is selected, which the UI prevents unless the experiment is enabled.
Default/Fallback Scenarios
core/navigation-overlay-closeblockTesting Instructions
Overlays containing Nav blocks with overlay set to "Always":
Overlays with/without Nav Overlay Close block:
core/navigation-overlay-closeblock: Create an overlay template part that includes acore/navigation-overlay-closeblock. Verify that:core/navigation-overlay-closeblock: Create an overlay template part without the close block. Verify that:With and without custom overlay:
Accessibility and SEO Considerations
Accessibility: Overlay content (including any navigation blocks within it) is present in the DOM even when visually hidden. While we use
aria-hidden="true"by default, this may not be sufficient for all assistive technologies.SEO: Search engines may index overlay content that is intended to be mobile-only, potentially causing duplicate content issues if the overlay contains navigation blocks.
Potential Mitigations
To address these concerns, we could consider:
aria-hiddenmanagement: Use Interactivity API to togglearia-hiddenbased on breakpoint and menu state (currently only set statically)hiddenattribute: Addhiddenattribute when overlay is not visible (in addition to CSS display:none)inertattribute: Useinertattribute to make overlay content non-interactive when hiddendata-noindexor similar attributes to overlay container when hiddenThese mitigations should be evaluated and potentially implemented in a follow-up PR based on testing and feedback.