fix: Refactor and fix AppLayout and side navigation panel state and behaviour

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

@@ -4,6 +4,7 @@ import React from 'react';
 import SideNavigation, { SideNavigationProps } from '~components/side-navigation';
 import AppLayout from '~components/app-layout';
 import Badge from '~components/badge';
+import labels from '../app-layout/utils/labels';
 
 import logoSmall from './logos/logo-small.svg';
 
@@ -101,12 +102,16 @@ const items: SideNavigationProps.Item[] = [
 ];
 
 export default function SideNavigationPage() {
+  const [open, setOpen] = React.useState(true);
+
   return (
     <AppLayout
-      toolsHide={true}
-      navigationOpen={true}
+      navigationOpen={open}
+      onNavigationChange={({ detail }) => {
+        setOpen(detail.open);
+      }}
       contentType="form"
-      ariaLabels={{ navigationClose: 'Close' }}
+      ariaLabels={labels}
       navigation={
         <SideNavigation
           activeHref="#/"

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

@@ -1,6 +1,6 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
-import React from 'react';
+import React, { useState } from 'react';
 import { act } from 'react-dom/test-utils';
 import {
   describeEachThemeAppLayout,
@@ -22,12 +22,33 @@ import testUtilsStyles from '../../../lib/components/app-layout/test-classes/sty
 
 import visualRefreshRefactoredStyles from '../../../lib/components/app-layout/visual-refresh/styles.css.js';
 import { findUpUntil } from '../../../lib/components/internal/utils/dom';
+import SideNavigation from '../../../lib/components/side-navigation';
 
 jest.mock('@cloudscape-design/component-toolkit/internal', () => ({
   ...jest.requireActual('@cloudscape-design/component-toolkit/internal'),
   isMotionDisabled: jest.fn().mockReturnValue(true),
 }));
 
+function AppLayoutWithControlledNavigation({
+  initialNavigationOpen,
+  navigation,
+}: {
+  initialNavigationOpen: boolean;
+  navigation: React.ReactNode;
+}) {
+  const [navigationOpen, setNavigationOpen] = useState(initialNavigationOpen);
+
+  return (
+    <AppLayout
+      navigationOpen={navigationOpen}
+      onNavigationChange={({ detail }) => {
+        setNavigationOpen(detail.open);
+      }}
+      navigation={navigation}
+    />
+  );
+}
+
 describeEachThemeAppLayout(true, theme => {
   // In refactored Visual Refresh different styles are used compared to Classic
   const mobileBarClassName = theme === 'refresh' ? testUtilsStyles['mobile-bar'] : toolbarStyles['mobile-bar'];
@@ -46,6 +67,37 @@ describeEachThemeAppLayout(true, theme => {
     expect(wrapper.findToolsToggle().getElement()).toBeEnabled();
   });
 
+  test('AppLayout with controlled navigation has navigation forcely closed on initial load', () => {
+    const { wrapper } = renderComponent(
+      <AppLayoutWithControlledNavigation
+        initialNavigationOpen={true}
+        navigation={
+          <>
+            <h1>Navigation</h1>
+            <a href="test">Link</a>
+          </>
+        }
+      />
+    );
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+  });
+
+  test('AppLayout with uncontrolled navigation has navigation forcely closed on initial load', () => {
+    const { wrapper } = renderComponent(
+      <AppLayout
+        navigation={
+          <>
+            <h1>Navigation</h1>
+            <a href="test">Link</a>
+          </>
+        }
+      />
+    );
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+  });
+
   test('renders open navigation state', () => {
     const { wrapper } = renderComponent(<AppLayout navigationOpen={true} onNavigationChange={() => {}} />);
     expect(wrapper.findNavigation()).toBeTruthy();
@@ -86,30 +138,58 @@ describeEachThemeAppLayout(true, theme => {
   });
 
   test('closes navigation when clicking on links', () => {
-    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayout
-        navigationOpen={true}
-        onNavigationChange={onNavigationChange}
+      <AppLayoutWithControlledNavigation
+        initialNavigationOpen={true}
         navigation={
           <>
             <h1>Navigation</h1>
-            <a href="#">Link</a>
+            <a href="test">Link</a>
           </>
         }
       />
     );
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+
+    wrapper.findNavigationToggle().click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
+
     wrapper.findNavigation().find('a')!.click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+  });
+
+  test('closes navigation when clicking on a link in the Side Navigation component', () => {
+    const { wrapper } = renderComponent(
+      <AppLayoutWithControlledNavigation
+        initialNavigationOpen={true}
+        navigation={
+          <SideNavigation
+            items={[
+              {
+                type: 'link',
+                text: 'Page 1',
+                href: '#/page1',
+              },
+            ]}
+          />
+        }
+      />
+    );
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+
+    wrapper.findNavigationToggle().click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
 
-    expect(onNavigationChange).toHaveBeenCalledWith(expect.objectContaining({ detail: { open: false } }));
+    wrapper.findNavigation().find('a')!.click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
   });
 
   test('does not close navigation when anchor without href was clicked', () => {
-    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayout
-        navigationOpen={true}
-        onNavigationChange={onNavigationChange}
+      <AppLayoutWithControlledNavigation
+        initialNavigationOpen={true}
         navigation={
           <>
             <h1>Navigation</h1>
@@ -118,28 +198,36 @@ describeEachThemeAppLayout(true, theme => {
         }
       />
     );
-    wrapper.findNavigation().find('a')!.click();
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+
+    wrapper.findNavigationToggle().click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
 
-    expect(onNavigationChange).not.toHaveBeenCalled();
+    wrapper.findNavigation().find('a')!.click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
   });
 
   test('does not close navigation when other elements were clicked', () => {
-    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayout
-        navigationOpen={true}
-        onNavigationChange={onNavigationChange}
+      <AppLayoutWithControlledNavigation
+        initialNavigationOpen={true}
         navigation={
           <>
             <h1>Navigation</h1>
-            <a href="#">Link</a>
+            <a>Link</a>
           </>
         }
       />
     );
-    wrapper.findNavigation().find('h1')!.click();
+    // AppLayout forcely closes the navigation on the first load on mobile, so the main content is visible
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
 
-    expect(onNavigationChange).not.toHaveBeenCalled();
+    wrapper.findNavigationToggle().click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
+
+    wrapper.findNavigation().find('h1')!.click();
+    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
   });
 
   test('does not close tools when clicking on any element', () => {

src/app-layout/__tests__/utils.tsx

@@ -76,9 +76,9 @@ export function describeEachThemeAppLayout(isMobile: boolean, callback: (theme:
   }
 }
 
-export function describeEachAppLayout(callback: () => void) {
+export function describeEachAppLayout(callback: (size: 'desktop' | 'mobile') => void) {
   for (const theme of ['refresh', 'classic']) {
-    for (const size of ['desktop', 'mobile']) {
+    for (const size of ['desktop', 'mobile'] as const) {
       describe(`Theme=${theme}, Size=${size}`, () => {
         beforeEach(() => {
           (useMobile as jest.Mock).mockReturnValue(size === 'mobile');
@@ -88,7 +88,7 @@ export function describeEachAppLayout(callback: () => void) {
           (useMobile as jest.Mock).mockReset();
           (useVisualRefresh as jest.Mock).mockReset();
         });
-        callback();
+        callback(size);
       });
     }
   }

src/app-layout/index.tsx

@@ -170,14 +170,11 @@ const OldAppLayout = React.forwardRef(
       setLastInteraction: setDrawerLastInteraction,
     } = useDrawerFocusControl([activeDrawer?.resizable], toolsOpen || activeDrawer !== undefined, true);
 
-    const onNavigationToggle = useCallback(
-      (open: boolean) => {
-        setNavigationOpen(open);
-        focusNavButtons();
-        fireNonCancelableEvent(onNavigationChange, { open });
-      },
-      [setNavigationOpen, onNavigationChange, focusNavButtons]
-    );
+    const onNavigationToggle = useStableCallback((open: boolean) => {
+      setNavigationOpen(open);
+      focusNavButtons();
+      fireNonCancelableEvent(onNavigationChange, { open });
+    });
     const onToolsToggle = useCallback(
       (open: boolean) => {
         setToolsOpen(open);
@@ -197,6 +194,13 @@ const OldAppLayout = React.forwardRef(
       }
     };
 
+    useEffect(() => {
+      // Close navigation drawer on mobile so that the main content is visible
+      if (isMobile) {
+        onNavigationToggle(false);
+      }
+    }, [isMobile, onNavigationToggle]);
+
     const navigationVisible = !navigationHide && navigationOpen;
     const toolsVisible = !toolsHide && toolsOpen;
 

src/app-layout/visual-refresh/context.tsx

@@ -26,7 +26,7 @@ import { SplitPanelSideToggleProps } from '../../internal/context/split-panel-co
 import { useObservedElement } from '../utils/use-observed-element';
 import { useMobile } from '../../internal/hooks/use-mobile';
 import { DrawerItem, InternalDrawerProps } from '../drawer/interfaces';
-import { warnOnce } from '@cloudscape-design/component-toolkit/internal';
+import { useStableCallback, warnOnce } from '@cloudscape-design/component-toolkit/internal';
 import useResize from '../utils/use-resize';
 import styles from './styles.css.js';
 import { useContainerQuery } from '@cloudscape-design/component-toolkit';
@@ -182,14 +182,18 @@ export const AppLayoutInternalsProvider = React.forwardRef(
 
     const { refs: navigationRefs, setFocus: focusNavButtons } = useFocusControl(isNavigationOpen);
 
-    const handleNavigationClick = useCallback(
-      function handleNavigationChange(isOpen: boolean) {
-        setIsNavigationOpen(isOpen);
-        focusNavButtons();
-        fireNonCancelableEvent(props.onNavigationChange, { open: isOpen });
-      },
-      [props.onNavigationChange, setIsNavigationOpen, focusNavButtons]
-    );
+    const handleNavigationClick = useStableCallback(function handleNavigationChange(isOpen: boolean) {
+      setIsNavigationOpen(isOpen);
+      focusNavButtons();
+      fireNonCancelableEvent(props.onNavigationChange, { open: isOpen });
+    });
+
+    useEffect(() => {
+      // Close navigation drawer on mobile so that the main content is visible
+      if (isMobile) {
+        handleNavigationClick(false);
+      }
+    }, [isMobile, handleNavigationClick]);
 
     /**
      * The useControllable hook will set the default value and manage either

src/expandable-section/expandable-section-header.tsx

@@ -95,14 +95,15 @@ const ExpandableNavigationHeader = ({
   icon,
 }: ExpandableNavigationHeaderProps) => {
   return (
-    <div id={id} className={clsx(className, styles['click-target'])} onClick={onClick}>
+    <div id={id} className={clsx(className, styles['click-target'])}>
       <button
         className={clsx(styles['icon-container'], styles['expand-button'])}
         aria-labelledby={ariaLabelledBy}
         aria-label={ariaLabel}
         aria-controls={ariaControls}
         aria-expanded={expanded}
         type="button"
+        onClick={onClick}
       >
         {icon}
       </button>

src/side-navigation/internal.tsx

@@ -227,9 +227,6 @@ function Link({ definition, expanded, activeHref, fireFollow }: LinkProps) {
 
   const onClick = useCallback(
     (event: React.MouseEvent) => {
-      // Prevent the click event from toggling outer expandable sections.
-      event.stopPropagation();
-
       if (isPlainLeftClick(event)) {
         fireFollow(definition, event);
       }