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/index.tsx

@@ -197,6 +197,14 @@ const OldAppLayout = React.forwardRef(
       }
     };
 
+    useEffect(() => {
+      // Close navigation drawer on mobile so that the main content is visible
+      if (isMobile) {
+        onNavigationToggle(false);
+      }
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [isMobile]);
+
     const navigationVisible = !navigationHide && navigationOpen;
     const toolsVisible = !toolsHide && toolsOpen;
 

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

@@ -187,6 +187,14 @@ export const AppLayoutInternalsProvider = React.forwardRef(
       [props.onNavigationChange, setIsNavigationOpen, focusNavButtons]
     );
 
+    useEffect(() => {
+      // Close navigation drawer on mobile so that the main content is visible
+      if (isMobile) {
+        handleNavigationClick(false);
+      }
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [isMobile]);
+
     /**
      * The useControllable hook will set the default value and manage either
      * the controlled or uncontrolled state of the Tools drawer. The logic

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,24 @@ 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 onClick={disableExpandChangeOnHeaderTextClick ? () => {} : onClick}>{children}</span>
     </div>
   );
 };
@@ -221,6 +227,7 @@ export const ExpandableSectionHeader = ({
   onKeyUp,
   onKeyDown,
   onClick,
+  disableExpandChangeOnHeaderTextClick,
 }: ExpandableSectionHeaderProps) => {
   const icon = (
     <InternalIcon
@@ -256,6 +263,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/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);
       }
@@ -403,6 +400,9 @@ function ExpandableLinkGroup({ definition, fireFollow, fireChange, activeHref, v
     }
   };
 
+  // By using `disableExpandChangeOnHeaderTextClick`, clicking the header text will not toggle the expandable section.
+  const disableExpandChangeOnHeaderTextClick = true;
+
   return (
     <InternalExpandableSection
       className={clsx(
@@ -412,6 +412,7 @@ function ExpandableLinkGroup({ definition, fireFollow, fireChange, activeHref, v
       variant="navigation"
       expanded={userExpanded ?? expanded}
       onChange={onExpandedChange}
+      __disableExpandChangeOnHeaderTextClick={disableExpandChangeOnHeaderTextClick}
       headerText={
         <Link
           definition={{ type: 'link', href: definition.href, text: definition.text }}