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

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

@@ -101,10 +101,14 @@ 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' }}
       navigation={

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,
@@ -19,12 +19,33 @@ import toolbarStyles from '../../../lib/components/app-layout/mobile-toolbar/sty
 import testUtilsStyles from '../../../lib/components/app-layout/test-classes/styles.css.js';
 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'];
@@ -43,6 +64,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();
@@ -83,30 +135,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>
@@ -115,28 +195,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

@@ -75,7 +75,7 @@ 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']) {
       describe(`Theme=${theme}, Size=${size}`, () => {
@@ -87,7 +87,7 @@ export function describeEachAppLayout(callback: () => void) {
           (useMobile as jest.Mock).mockReset();
           (useVisualRefresh as jest.Mock).mockReset();
         });
-        callback();
+        callback(size as 'desktop' | 'mobile');
       });
     }
   }

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';
@@ -178,14 +178,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

@@ -29,6 +29,7 @@ interface ExpandableDefaultHeaderProps {
 
 interface ExpandableNavigationHeaderProps extends Omit<ExpandableDefaultHeaderProps, 'onKeyUp' | 'onKeyDown'> {
   ariaLabelledBy?: string;
+  disableExpandChangeOnHeaderTextClick?: boolean;
 }
 
 interface ExpandableHeaderTextWrapperProps extends ExpandableDefaultHeaderProps {
@@ -49,6 +50,7 @@ interface ExpandableSectionHeaderProps extends Omit<ExpandableDefaultHeaderProps
   headerActions?: ReactNode;
   headingTagOverride?: ExpandableSectionProps.HeadingTag;
   ariaLabelledBy?: string;
+  disableExpandChangeOnHeaderTextClick?: boolean;
 }
 
 const ExpandableDeprecatedHeader = ({
@@ -93,20 +95,29 @@ const ExpandableNavigationHeader = ({
   expanded,
   children,
   icon,
+  disableExpandChangeOnHeaderTextClick,
 }: ExpandableNavigationHeaderProps) => {
+  // By using `disableExpandChangeOnHeaderTextClick`, clicking the header text will not toggle the expandable section.
+  // We use this for the "expandable-link-group" in the side navigation.
   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>
-      {children}
+      <span
+        className={styles['click-target-content']}
+        onClick={disableExpandChangeOnHeaderTextClick ? () => {} : onClick}
+      >
+        {children}
+      </span>
     </div>
   );
 };
@@ -221,6 +232,7 @@ export const ExpandableSectionHeader = ({
   onKeyUp,
   onKeyDown,
   onClick,
+  disableExpandChangeOnHeaderTextClick,
 }: ExpandableSectionHeaderProps) => {
   const icon = (
     <InternalIcon
@@ -256,6 +268,7 @@ export const ExpandableSectionHeader = ({
       <ExpandableNavigationHeader
         className={clsx(className, wrapperClassName)}
         ariaLabelledBy={ariaLabelledBy}
+        disableExpandChangeOnHeaderTextClick={disableExpandChangeOnHeaderTextClick}
         {...defaultHeaderProps}
       >
         {headerText ?? header}

src/expandable-section/internal.tsx

@@ -18,7 +18,8 @@ import { ExpandableSectionHeader } from './expandable-section-header';
 import { InternalBaseComponentProps } from '../internal/hooks/use-base-component';
 import { variantSupportsDescription } from './utils';
 
-type InternalExpandableSectionProps = ExpandableSectionProps & InternalBaseComponentProps;
+type InternalExpandableSectionProps = ExpandableSectionProps &
+  InternalBaseComponentProps & { __disableExpandChangeOnHeaderTextClick?: boolean };
 
 export default function InternalExpandableSection({
   expanded: controlledExpanded,
@@ -35,6 +36,7 @@ export default function InternalExpandableSection({
   headingTagOverride,
   disableContentPaddings,
   headerAriaLabel,
+  __disableExpandChangeOnHeaderTextClick = false,
   __internalRootRef,
   ...props
 }: InternalExpandableSectionProps) {
@@ -113,6 +115,7 @@ export default function InternalExpandableSection({
           headerInfo={headerInfo}
           headerActions={headerActions}
           headingTagOverride={headingTagOverride}
+          disableExpandChangeOnHeaderTextClick={__disableExpandChangeOnHeaderTextClick}
           {...triggerProps}
         />
       }

src/expandable-section/styles.scss

@@ -228,6 +228,9 @@ $icon-total-space-medium: calc(#{$icon-width-medium} + #{$icon-margin-left} + #{
 
 .click-target {
   cursor: pointer;
+  &-content {
+    display: inherit;
+  }
   &:not(.wrapper-container):not(.header-container-button):hover {
     color: awsui.$color-text-expandable-section-hover;
   }