fix: hide navigation button when toolbar variant of AppLayout has hideNavigation

pages/app-layout/controlled-navigation.page.tsx

@@ -47,25 +47,13 @@ export default function () {
 
       <SpaceBetween size="l">
         <SpaceBetween size="s" direction="horizontal">
-          <Toggle
-            id="control-navigation-open"
-            checked={navigationOpen}
-            onChange={e => setUrlParams({ navigationOpen: e.detail.checked })}
-          >
+          <Toggle checked={navigationOpen} onChange={e => setUrlParams({ navigationOpen: e.detail.checked })}>
             Navigation Open
           </Toggle>
-          <Toggle
-            id="control-navigation-hide"
-            checked={navigationHide}
-            onChange={e => setUrlParams({ navigationHide: e.detail.checked })}
-          >
+          <Toggle checked={navigationHide} onChange={e => setUrlParams({ navigationHide: e.detail.checked })}>
             Navigation Hide
           </Toggle>
-          <Toggle
-            id="control-navigation-empty"
-            checked={navigationEmpty}
-            onChange={e => setNavigationEmpty(e.detail.checked)}
-          >
+          <Toggle checked={navigationEmpty} onChange={e => setNavigationEmpty(e.detail.checked)}>
             Navigation Empty
           </Toggle>
         </SpaceBetween>
@@ -77,15 +65,13 @@ export default function () {
             Apply override
           </Checkbox>
           <Toggle
-            id="override-nav-open"
             checked={!!navOpenOverrideValue}
             disabled={!applyNavOpenOverride}
             onChange={e => setUrlParams({ navOpenOverrideValue: e.detail.checked })}
           >
             override Navigation Open
           </Toggle>
           <Button
-            id="reset-button"
             onClick={() => setResetNeeded(true)}
             disabled={resetNeeded}
             loading={resetNeeded}

src/app-layout/__tests__/app-layout-navigation.test.tsx

@@ -0,0 +1,31 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+/* eslint simple-import-sort/imports: 0 */
+import React from 'react';
+import { describeEachAppLayout, renderComponent } from './utils';
+import AppLayout from '../../../lib/components/app-layout';
+
+import visualRefreshToolbarStyles from '../../../lib/components/app-layout/visual-refresh-toolbar/skeleton/styles.css.js';
+
+describeEachAppLayout({ themes: ['refresh-toolbar'], sizes: ['desktop', 'mobile'] }, ({ size }) => {
+  test('a does not hide content when navigationOpen and navigationHide is true and onNavigationChange is overwritten', () => {
+    const mockOnNavigationChange = jest.fn();
+    const { wrapper } = renderComponent(
+      <AppLayout
+        navigationOpen={true}
+        navigationHide={true}
+        navigation={<>Mock Navigation</>}
+        onNavigationChange={mockOnNavigationChange}
+        content={<>Content</>}
+      />
+    );
+
+    expect(mockOnNavigationChange).toHaveBeenCalledTimes(size === 'mobile' ? 1 : 0);
+    expect(wrapper).not.toBeNull();
+    expect(wrapper.findNavigation()).toBeFalsy();
+    expect(wrapper.findNavigationToggle()).toBeFalsy();
+    expect(wrapper.findByClassName(visualRefreshToolbarStyles['main-landmark'])).not.toBeNull();
+    expect(wrapper.findByClassName(visualRefreshToolbarStyles['unfocusable-mobile'])).toBeNull();
+    expect(wrapper.findByClassName(visualRefreshToolbarStyles.content)?.getElement()).toBeVisible();
+  });
+});

src/app-layout/__tests__/multi-layout-props.test.tsx

@@ -0,0 +1,109 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+import React from 'react';
+
+import {
+  mergeProps,
+  SharedProps,
+} from '../../../lib/components/app-layout/visual-refresh-toolbar/multi-layout';
+
+describe('mergeMultiAppLayoutProps', () => {
+  const mockParentNavigationToggle = jest.fn();
+  const mockParentActiveDrawerChange = jest.fn();
+  const mockParentSplitPanelToggle = jest.fn();
+  const ownProps: SharedProps = {
+    forceDeduplicationType: 'primary',
+    ariaLabels: {
+      navigation: 'Navigation',
+      drawers: 'Drawers',
+    },
+    navigation: <div>Navigation</div>,
+    navigationOpen: true,
+    navigationFocusRef: React.createRef(),
+    onNavigationToggle: mockParentNavigationToggle,
+    breadcrumbs: <div>Breadcrumbs</div>,
+    activeDrawerId: 'drawer1',
+    drawers: [
+      {
+        id: 'drawer1',
+        ariaLabels: { drawerName: 'Drawer 1' },
+        content: <div>Drawer 1 Content</div>,
+      },
+    ],
+    onActiveDrawerChange: mockParentActiveDrawerChange,
+    drawersFocusRef: React.createRef(),
+    splitPanel: <div>Split Panel</div>,
+    splitPanelToggleProps: {
+      displayed: false,
+      active: false,
+      position: 'bottom',
+      controlId: 'test',
+      ariaLabel: 'test',
+    },
+    splitPanelFocusRef: React.createRef(),
+    onSplitPanelToggle: mockParentSplitPanelToggle,
+  };
+
+  const additionalPropsBase: Partial<SharedProps>[] = [
+    {
+      ariaLabels: {
+        navigation: 'New Navigation',
+      },
+      drawers: [
+        {
+          id: 'drawer2',
+          ariaLabels: { drawerName: 'Drawer 2' },
+          content: <div>Drawer 2 Content</div>,
+        },
+      ],
+      activeDrawerId: 'drawer2',
+    },
+    {
+      splitPanelToggleProps: {
+        displayed: false,
+        active: false,
+        position: 'bottom',
+        controlId: 'test',
+        ariaLabel: 'test',
+      },
+    },
+  ];
+
+  it('should merge ownProps and additionalProps correctly', () => {
+    const result = mergeProps(ownProps, additionalPropsBase);
+
+    expect(result).toEqual({
+      //asserting new aria labels overwrite existing yet preserve others
+      ariaLabels: {
+        navigation: 'New Navigation',
+        drawers: 'Drawers',
+      },
+      hasNavigation: true,
+      navigationOpen: true,
+      navigationFocusRef: ownProps.navigationFocusRef,
+      onNavigationToggle: mockParentNavigationToggle,
+      hasBreadcrumbsPortal: true,
+      hasSplitPanel: true,
+      splitPanelToggleProps: {
+        displayed: false,
+        active: false,
+        position: 'bottom',
+        controlId: 'test',
+        ariaLabel: 'test',
+      },
+      splitPanelFocusRef: ownProps.splitPanelFocusRef,
+      onSplitPanelToggle: mockParentSplitPanelToggle,
+      //asserting the ownProps drawer is not overwritten
+      activeDrawerId: ownProps.activeDrawerId,
+      drawers: ownProps.drawers,
+      drawersFocusRef: ownProps.drawersFocusRef,
+      onActiveDrawerChange: mockParentActiveDrawerChange,
+    });
+  });
+
+  it('should return null if no fields are defined, except ariaLabels', () => {
+    const result = mergeProps({ ariaLabels: {} } as SharedProps, []);
+
+    expect(result).toBeNull();
+  });
+});

src/app-layout/__tests__/multi-layout.test.tsx

@@ -75,7 +75,26 @@ describeEachAppLayout({ themes: ['refresh-toolbar'], sizes: ['desktop'] }, () =>
     expect(firstLayout.findOpenNavigationPanel()).toBeTruthy();
   });
 
-  test('merges tools from two instances', async () => {
+  test('navigationHide in primary is respected when navigation is defined when merging from two instances', async () => {
+    const { firstLayout, secondLayout } = await renderAsync(
+      <AppLayout
+        {...defaultAppLayoutProps}
+        data-testid="first"
+        navigation="testing nav"
+        navigationHide={true}
+        toolsHide={true}
+        content={
+          <AppLayout {...defaultAppLayoutProps} data-testid="second" navigationHide={true} tools="testing tools" />
+        }
+      />
+    );
+    expect(firstLayout.findNavigation()).toBeFalsy();
+    expect(firstLayout.findNavigationToggle()).toBeFalsy();
+    expect(secondLayout.findNavigation()).toBeFalsy();
+    expect(secondLayout.findNavigationToggle()).toBeFalsy();
+  });
+
+  test('merges tools from two instances with where navigationHide is true in secondary', async () => {
     const { firstLayout, secondLayout } = await renderAsync(
       <AppLayout
         {...defaultAppLayoutProps}
@@ -89,6 +108,8 @@ describeEachAppLayout({ themes: ['refresh-toolbar'], sizes: ['desktop'] }, () =>
     expect(createWrapper().findAllByClassName(testUtilStyles.tools)).toHaveLength(1);
 
     firstLayout.findToolsToggle().click();
+    expect(secondLayout.findNavigation()).toBeFalsy();
+    expect(secondLayout.findNavigationToggle()).toBeFalsy();
     expect(secondLayout.findOpenToolsPanel()).toBeTruthy();
   });
 

src/app-layout/visual-refresh-toolbar/index.tsx

@@ -224,8 +224,11 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
       focusSplitPanel: () => splitPanelFocusControl.refs.slider.current?.focus(),
     }));
 
-    const resolvedNavigation = navigationHide ? null : navigation ?? <></>;
     const resolvedStickyNotifications = !!stickyNotifications && !isMobile;
+    //navigation must be null if hidden so toolbar knows to hide the toggle button
+    const resolvedNavigation = navigationHide ? null : navigation || <></>;
+    //navigation must not be open if navigationHide is true
+    const resolvedNavigationOpen = !!resolvedNavigation && navigationOpen;
     const {
       maxDrawerSize,
       maxSplitPanelSize,
@@ -237,7 +240,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
       activeDrawerSize: activeDrawer ? activeDrawerSize : 0,
       splitPanelSize,
       minContentWidth,
-      navigationOpen: !!resolvedNavigation && navigationOpen,
+      navigationOpen: resolvedNavigationOpen,
       navigationWidth,
       placement,
       splitPanelOpen,
@@ -252,7 +255,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
         forceDeduplicationType,
         ariaLabels: ariaLabelsWithDrawers,
         navigation: resolvedNavigation,
-        navigationOpen,
+        navigationOpen: resolvedNavigationOpen,
         onNavigationToggle,
         navigationFocusRef: navigationFocusControl.refs.toggle,
         breadcrumbs,
@@ -292,7 +295,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
       breadcrumbs,
       discoveredBreadcrumbs,
       stickyNotifications: resolvedStickyNotifications,
-      navigationOpen,
+      navigationOpen: resolvedNavigationOpen,
       navigation: resolvedNavigation,
       navigationFocusControl,
       activeDrawer,
@@ -395,11 +398,11 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
         return;
       }
 
-      const activeNavigationWidth = navigationOpen ? navigationWidth : 0;
+      const activeNavigationWidth = !navigationHide && navigationOpen ? navigationWidth : 0;
       const scrollWidth = activeNavigationWidth + CONTENT_PADDING + totalActiveDrawersMinSize;
       const hasHorizontalScroll = scrollWidth > placement.inlineSize;
       if (hasHorizontalScroll) {
-        if (navigationOpen) {
+        if (!navigationHide && navigationOpen) {
           onNavigationToggle(false);
           return;
         }
@@ -410,6 +413,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
       totalActiveDrawersMinSize,
       closeFirstDrawer,
       isMobile,
+      navigationHide,
       navigationOpen,
       navigationWidth,
       onNavigationToggle,
@@ -441,7 +445,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
           // delay rendering the content until registration of this instance is complete
           content={registered ? content : null}
           navigation={resolvedNavigation && <AppLayoutNavigation appLayoutInternals={appLayoutInternals} />}
-          navigationOpen={navigationOpen}
+          navigationOpen={resolvedNavigationOpen}
           navigationWidth={navigationWidth}
           tools={drawers && drawers.length > 0 && <AppLayoutDrawer appLayoutInternals={appLayoutInternals} />}
           globalTools={

src/app-layout/visual-refresh-toolbar/multi-layout.ts

@@ -10,7 +10,7 @@ import { AppLayoutProps } from '../interfaces';
 import { Focusable } from '../utils/use-focus-control';
 import { SplitPanelToggleProps, ToolbarProps } from './toolbar';
 
-interface SharedProps {
+export interface SharedProps {
   forceDeduplicationType?: 'primary' | 'secondary' | 'suspended' | 'off';
   ariaLabels: AppLayoutProps.Labels | undefined;
   navigation: React.ReactNode;
@@ -39,7 +39,10 @@ function checkAlreadyExists(value: boolean, propName: string) {
   return false;
 }
 
-function mergeProps(ownProps: SharedProps, additionalProps: ReadonlyArray<Partial<SharedProps>>): ToolbarProps | null {
+export function mergeProps(
+  ownProps: SharedProps,
+  additionalProps: ReadonlyArray<Partial<SharedProps>>
+): ToolbarProps | null {
   const toolbar: ToolbarProps = {};
   for (const props of [ownProps, ...additionalProps]) {
     toolbar.ariaLabels = Object.assign(toolbar.ariaLabels ?? {}, props.ariaLabels);
@@ -50,6 +53,8 @@ function mergeProps(ownProps: SharedProps, additionalProps: ReadonlyArray<Partia
       toolbar.onActiveDrawerChange = props.onActiveDrawerChange;
     }
     if (props.navigation && !checkAlreadyExists(!!toolbar.hasNavigation, 'navigation')) {
+      // there is never a case where navigation will exist and a toggle will not so toolbar
+      // can use the hasNavigation here to conditionally render the navigationToggle button
       toolbar.hasNavigation = true;
       toolbar.navigationOpen = props.navigationOpen;
       toolbar.navigationFocusRef = props.navigationFocusRef;