Skip to content

Render effective Navigation submenu settings without persisting repairs #5

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.

Sign up for GitHub

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

Closed
adamziel wants to merge 1 commit into from
Closed

Render effective Navigation submenu settings without persisting repairs #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 83 additions & 46 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
__experimentalUseMultipleOriginColorsAndGradients as useMultipleOriginColorsAndGradients,
useBlockEditingMode,
BlockControls,
BlockContextProvider,
} from '@wordpress/block-editor';
import {
EntityProvider,
Expand Down Expand Up @@ -362,28 +363,14 @@ function Navigation( {
innerBlocks,
} = useInnerBlocks( clientId );

// Reset submenuVisibility to default if orientation changes to horizontal
// while "always" is selected, but only when the Navigation block or one
// of its inner blocks is being edited. Rendering related template parts
// should not mark them dirty.
useEffect( () => {
if (
( isSelected || isInnerBlockSelected ) &&
orientation === 'horizontal' &&
submenuVisibility === 'always'
) {
setAttributes( {
submenuVisibility: 'hover',
showSubmenuIcon: true,
} );
}
}, [
isSelected,
isInnerBlockSelected,
const {
submenuVisibility: effectiveSubmenuVisibility,
showSubmenuIcon: effectiveShowSubmenuIcon,
} = getEffectiveSubmenuSettings( {
orientation,
submenuVisibility,
setAttributes,
] );
showSubmenuIcon,
} );

// Use a ref to store whether we've confirmed a page-list has submenus.
// Once confirmed, we don't need to keep checking the page-list blocks.
Expand Down Expand Up @@ -766,9 +753,9 @@ function Navigation( {
);

const submenuAccessibilityNotice =
! showSubmenuIcon &&
submenuVisibility !== 'click' &&
submenuVisibility !== 'always'
! effectiveShowSubmenuIcon &&
effectiveSubmenuVisibility !== 'click' &&
effectiveSubmenuVisibility !== 'always'
? __(
'The current menu options offer reduced accessibility for users and are not recommended. Enabling either "Open on Click" or "Show arrow" offers enhanced accessibility by allowing keyboard users to browse submenus selectively.'
)
Expand Down Expand Up @@ -813,7 +800,7 @@ function Navigation( {
</h3>
<ToolsPanelItem
hasValue={ () =>
submenuVisibility !== 'hover'
effectiveSubmenuVisibility !== 'hover'
}
label={ __( 'Submenu Visibility' ) }
onDeselect={ () =>
Expand All @@ -826,7 +813,7 @@ function Navigation( {
<ToggleGroupControl
__next40pxDefaultSize
label={ __( 'Submenu Visibility' ) }
value={ submenuVisibility }
value={ effectiveSubmenuVisibility }
onChange={ ( value ) => {
const newAttributes = {
submenuVisibility: value,
Expand Down Expand Up @@ -867,29 +854,34 @@ function Navigation( {
</ToolsPanelItem>

<ToolsPanelItem
hasValue={ () => ! showSubmenuIcon }
hasValue={ () =>
! effectiveShowSubmenuIcon
}
label={ __( 'Show arrow' ) }
onDeselect={ () =>
setAttributes( {
showSubmenuIcon: true,
} )
}
isDisabled={
submenuVisibility === 'click' ||
submenuVisibility === 'always'
effectiveSubmenuVisibility ===
'click' ||
effectiveSubmenuVisibility === 'always'
}
isShownByDefault
>
<ToggleControl
checked={ showSubmenuIcon }
checked={ effectiveShowSubmenuIcon }
onChange={ ( value ) => {
setAttributes( {
showSubmenuIcon: value,
} );
} }
disabled={
submenuVisibility === 'click' ||
submenuVisibility === 'always'
effectiveSubmenuVisibility ===
'click' ||
effectiveSubmenuVisibility ===
'always'
}
label={ __( 'Show arrow' ) }
/>
Expand Down Expand Up @@ -998,11 +990,20 @@ function Navigation( {
overlay={ overlay }
onNavigateToEntityRecord={ onNavigateToEntityRecord }
>
<UnsavedInnerBlocks
createNavigationMenu={ createNavigationMenu }
blocks={ uncontrolledInnerBlocks }
hasSelection={ isSelected || isInnerBlockSelected }
/>
<BlockContextProvider
value={ {
submenuVisibility: effectiveSubmenuVisibility,
showSubmenuIcon: effectiveShowSubmenuIcon,
} }
>
<UnsavedInnerBlocks
createNavigationMenu={ createNavigationMenu }
blocks={ uncontrolledInnerBlocks }
hasSelection={
isSelected || isInnerBlockSelected
}
/>
</BlockContextProvider>
</ResponsiveWrapper>
</TagName>
</>
Expand Down Expand Up @@ -1167,16 +1168,25 @@ function Navigation( {
onNavigateToEntityRecord
}
>
{ isEntityAvailable && (
<NavigationInnerBlocks
clientId={ clientId }
hasCustomPlaceholder={
!! CustomPlaceholder
}
templateLock={ templateLock }
orientation={ orientation }
/>
) }
<BlockContextProvider
value={ {
submenuVisibility:
effectiveSubmenuVisibility,
showSubmenuIcon:
effectiveShowSubmenuIcon,
} }
>
{ isEntityAvailable && (
<NavigationInnerBlocks
clientId={ clientId }
hasCustomPlaceholder={
!! CustomPlaceholder
}
templateLock={ templateLock }
orientation={ orientation }
/>
) }
</BlockContextProvider>
</ResponsiveWrapper>
</>
) }
Expand All @@ -1193,3 +1203,30 @@ export default withColors(
{ overlayBackgroundColor: 'color' },
{ overlayTextColor: 'color' }
)( Navigation );

function getEffectiveSubmenuSettings( {
orientation,
submenuVisibility,
showSubmenuIcon,
} ) {
const resolvedSubmenuVisibility = submenuVisibility ?? 'hover';
const resolvedShowSubmenuIcon = showSubmenuIcon ?? true;

// Horizontal navigation menus cannot display always-open submenus. Treat
// older content with that combination as the horizontal default without
// writing the repaired values back to the entity.
if (
orientation === 'horizontal' &&
resolvedSubmenuVisibility === 'always'
) {
return {
submenuVisibility: 'hover',
showSubmenuIcon: true,
};
}

return {
submenuVisibility: resolvedSubmenuVisibility,
showSubmenuIcon: resolvedShowSubmenuIcon,
};
}
44 changes: 41 additions & 3 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,28 @@
return $submenu_visibility ?? 'hover';
}

/**
* Returns navigation attributes with impossible submenu settings coerced for rendering.
*
* Horizontal navigation menus cannot display always-open submenus. Older content may
* still contain that combination, so render it as the horizontal default without
* writing the repaired values back to the entity.
*
* @param array $attributes Block attributes.
* @return array Attributes to use for render behavior and block context.
*/
function block_core_navigation_get_effective_submenu_attributes( $attributes ) {

Check failure on line 53 in packages/block-library/src/navigation/index.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

Missing @SInCE tag for the "block_core_navigation_get_effective_submenu_attributes()" function.
$orientation = $attributes['layout']['orientation'] ?? 'horizontal';
$submenu_visibility = block_core_navigation_get_submenu_visibility( $attributes );

if ( 'horizontal' === $orientation && 'always' === $submenu_visibility ) {
$attributes['submenuVisibility'] = 'hover';
$attributes['showSubmenuIcon'] = true;
}

return $attributes;
}

/**
* Helper functions used to render the navigation block.
*
Expand Down Expand Up @@ -474,10 +496,22 @@
*
* @param array $attributes The block attributes.
* @param WP_Block $block The parsed block.
* @param bool $use_effective_context Whether to rebuild inline inner blocks with effective block context.
* @return WP_Block_List Returns the inner blocks for the navigation block.
*/
private static function get_inner_blocks( $attributes, $block ) {
$inner_blocks = $block->inner_blocks;
private static function get_inner_blocks(
$attributes,
$block,
$use_effective_context = false
) {
$use_parsed_inner_blocks = $use_effective_context &&
! empty( $block->parsed_block['innerBlocks'] );
$inner_blocks = $use_parsed_inner_blocks
? new WP_Block_List(
$block->parsed_block['innerBlocks'],
$attributes
)
: $block->inner_blocks;

// Ensure that blocks saved with the legacy ref attribute name (navigationMenuId) continue to render.
if ( array_key_exists( 'navigationMenuId', $attributes ) ) {
Expand Down Expand Up @@ -991,7 +1025,11 @@

unset( $attributes['rgbTextColor'], $attributes['rgbBackgroundColor'] );

$inner_blocks = static::get_inner_blocks( $attributes, $block );
$effective_attributes = block_core_navigation_get_effective_submenu_attributes( $attributes );
$use_effective_context = $effective_attributes !== $attributes;
$attributes = $effective_attributes;

$inner_blocks = static::get_inner_blocks( $attributes, $block, $use_effective_context );
// Prevent navigation blocks referencing themselves from rendering.
if ( block_core_navigation_block_tree_has_block_type(
$inner_blocks,
Expand Down
7 changes: 6 additions & 1 deletion test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
await requestUtils.createTemplate( 'wp_template_part', {
slug: 'header',
title: 'Header',
content: `<!-- wp:navigation {"ref":${ menu.id },"overlayMenu":"off","submenuVisibility":"always"} /-->`,
content: `<!-- wp:navigation {"ref":${ menu.id },"overlayMenu":"never","submenuVisibility":"always"} /-->`,
} );

await requestUtils.createTemplate( 'wp_template', {
Expand Down Expand Up @@ -113,10 +113,15 @@
editor.canvas.getByRole( 'document', {
name: 'Block: Page List',
} )
).toBeVisible();

Check failure on line 116 in test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 2

[chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 

  1) [chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 

    Retry #1 ───────────────────────────────────────────────────────────────────────────────────────
    Error: expect(locator).toBeVisible() failed

    Locator: locator('[name="editor-canvas"]').contentFrame().getByRole('document', { name: 'Block: Page List' })
    Expected: visible
    Error: strict mode violation: getByRole('document', { name: 'Block: Page List' }) resolved to 2 elements:
        1) <ul inert="true" tabindex="-1" role="document" data-title="Page List" data-type="core/page-list" aria-label="Block: Page List" id="block-c3035b3c-42bc-4ae0-b3a0-353822762781" data-block="c3035b3c-42bc-4ae0-b3a0-353822762781" class="block-editor-block-list__block wp-block is-editing-disabled wp-block-page-list open-always wp-block-page-list block-editor-block-list__layout">…</ul> aka getByRole('document', { name: 'Block: Page List', exact: true })
        2) <ul inert="true" tabindex="-1" role="document" data-title="Page List Item" data-type="core/page-list-item" aria-label="Block: Page List Item" id="block-462a889c-437b-4299-8581-e5213fa61562" data-block="462a889c-437b-4299-8581-e5213fa61562" class="block-editor-block-list__block wp-block is-editing-disabled wp-block-navigation__submenu-container wp-block-page-list-item block-editor-block-list__layout">…</ul> aka getByRole('document', { name: 'Block: Page List Item' })

    Call log:
      - Expect "toBeVisible" with timeout 5000ms
      - waiting for locator('[name="editor-canvas"]').contentFrame().getByRole('document', { name: 'Block: Page List' })


      114 | 				name: 'Block: Page List',
      115 | 			} )
    > 116 | 		).toBeVisible();
          | 		  ^
      117 |
      118 | 		await editor.canvas
      119 | 			.getByRole( 'document', { name: 'Block: Navigation' } )
        at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js:116:5

await expect.poll( () => getDirtyEntityRecords( page ) ).toEqual( [] );

await editor.canvas

Check failure on line 120 in test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 2

[chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 

  1) [chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 

    Retry #2 ───────────────────────────────────────────────────────────────────────────────────────
    TimeoutError: locator.click: Timeout 10000ms exceeded.
    Call log:
      - waiting for locator('[name="editor-canvas"]').contentFrame().getByRole('document', { name: 'Block: Navigation' })
        - locator resolved to <nav inert="true" tabindex="-1" role="document" draggable="true" data-title="Navigation" data-type="core/navigation" aria-label="Block: Navigation" id="block-a65f36f8-8e9c-4ee3-8f6f-ee2728778412" data-block="a65f36f8-8e9c-4ee3-8f6f-ee2728778412" aria-describedby="a65f36f8-8e9c-4ee3-8f6f-ee2728778412-desc" class="block-editor-block-list__block wp-block is-editing-disabled is-layout-flex wp-block-navigation-is-layout-flex wp-block-navigation">…</nav>
      - attempting click action
        2 × waiting for element to be visible, enabled and stable
          - element is visible, enabled and stable
          - scrolling into view if needed
          - done scrolling
          - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-e76b01ac-4720-4a01-961a-761e4e913aa2" data-block="e76b01ac-4720-4a01-961a-761e4e913aa2" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
        - retrying click action
        - waiting 20ms
        2 × waiting for element to be visible, enabled and stable
          - element is visible, enabled and stable
          - scrolling into view if needed
          - done scrolling
          - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-e76b01ac-4720-4a01-961a-761e4e913aa2" data-block="e76b01ac-4720-4a01-961a-761e4e913aa2" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
        - retrying click action
          - waiting 100ms
        18 × waiting for element to be visible, enabled and stable
           - element is visible, enabled and stable
           - scrolling into view if needed
           - done scrolling
           - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-e76b01ac-4720-4a01-961a-761e4e913aa2" data-block="e76b01ac-4720-4a01-961a-761e4e913aa2" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
         - retrying click action
           - waiting 500ms
        - waiting for element to be visible, enabled and stable
        - element is visible, enabled and stable
        - scrolling into view if needed


      118 | 		await editor.canvas
      119 | 			.getByRole( 'document', { name: 'Block: Navigation' } )
    > 120 | 			.click();
          | 			 ^
      121 | 		await expect.poll( () => getDirtyEntityRecords( page ) ).toEqual( [] );
      122 |
      123 | 		await editor.canvas
        at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js:120:5

Check failure on line 120 in test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 2

[chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 

  1) [chromium] › specs/editor/blocks/navigation-passive-rendering.spec.js:61:2 › Navigation passive rendering › does not dirty related template parts or navigation menus when rendered in the post editor 
    TimeoutError: locator.click: Timeout 10000ms exceeded.
    Call log:
      - waiting for locator('[name="editor-canvas"]').contentFrame().getByRole('document', { name: 'Block: Navigation' })
        - locator resolved to <nav inert="true" tabindex="-1" role="document" draggable="true" data-title="Navigation" data-type="core/navigation" aria-label="Block: Navigation" id="block-224ab23c-e0a2-4b43-bcac-01257cb048a2" data-block="224ab23c-e0a2-4b43-bcac-01257cb048a2" aria-describedby="224ab23c-e0a2-4b43-bcac-01257cb048a2-desc" class="block-editor-block-list__block wp-block is-editing-disabled is-layout-flex wp-block-navigation-is-layout-flex wp-block-navigation">…</nav>
      - attempting click action
        2 × waiting for element to be visible, enabled and stable
          - element is visible, enabled and stable
          - scrolling into view if needed
          - done scrolling
          - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-02eb2163-85d2-4b17-86eb-7e2421f2e5b5" data-block="02eb2163-85d2-4b17-86eb-7e2421f2e5b5" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
        - retrying click action
        - waiting 20ms
        2 × waiting for element to be visible, enabled and stable
          - element is visible, enabled and stable
          - scrolling into view if needed
          - done scrolling
          - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-02eb2163-85d2-4b17-86eb-7e2421f2e5b5" data-block="02eb2163-85d2-4b17-86eb-7e2421f2e5b5" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
        - retrying click action
          - waiting 100ms
        18 × waiting for element to be visible, enabled and stable
           - element is visible, enabled and stable
           - scrolling into view if needed
           - done scrolling
           - <header tabindex="0" role="document" data-title="Header" data-is-drop-zone="true" aria-label="Block: Header" data-type="core/template-part" id="block-02eb2163-85d2-4b17-86eb-7e2421f2e5b5" data-block="02eb2163-85d2-4b17-86eb-7e2421f2e5b5" class="block-editor-block-list__block wp-block has-editable-outline wp-block-template-part block-editor-block-list__layout">…</header> intercepts pointer events
         - retrying click action
           - waiting 500ms
        - waiting for element to be visible, enabled and stable
        - element is visible, enabled and stable
        - scrolling into view if needed
        - done scrolling


      118 | 		await editor.canvas
      119 | 			.getByRole( 'document', { name: 'Block: Navigation' } )
    > 120 | 			.click();
          | 			 ^
      121 | 		await expect.poll( () => getDirtyEntityRecords( page ) ).toEqual( [] );
      122 |
      123 | 		await editor.canvas
        at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js:120:5
.getByRole( 'document', { name: 'Block: Navigation' } )
.click();
await expect.poll( () => getDirtyEntityRecords( page ) ).toEqual( [] );

await editor.canvas
.getByRole( 'document', { name: 'Block: Paragraph' } )
.click();
Expand Down
Loading