Revert "fix: Refactor and fix AppLayout and side navigation panel sta…

…te and behaviour (#1434)"

This reverts commit f0d128b.

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

@@ -4,7 +4,6 @@ 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';
 
@@ -102,16 +101,12 @@ const items: SideNavigationProps.Item[] = [
 ];
 
 export default function SideNavigationPage() {
-  const [open, setOpen] = React.useState(true);
-
   return (
     <AppLayout
-      navigationOpen={open}
-      onNavigationChange={({ detail }) => {
-        setOpen(detail.open);
-      }}
+      toolsHide={true}
+      navigationOpen={true}
       contentType="form"
-      ariaLabels={labels}
+      ariaLabels={{ navigationClose: 'Close' }}
       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, { useState } from 'react';
+import React from 'react';
 import { act } from 'react-dom/test-utils';
 import {
   describeEachThemeAppLayout,
@@ -22,33 +22,12 @@ 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'];
@@ -67,37 +46,6 @@ 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();
@@ -138,58 +86,30 @@ describeEachThemeAppLayout(true, theme => {
   });
 
   test('closes navigation when clicking on links', () => {
+    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayoutWithControlledNavigation
-        initialNavigationOpen={true}
+      <AppLayout
+        navigationOpen={true}
+        onNavigationChange={onNavigationChange}
         navigation={
           <>
             <h1>Navigation</h1>
-            <a href="test">Link</a>
+            <a href="#">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);
 
-    wrapper.findNavigation().find('a')!.click();
-    expect(isDrawerClosed(wrapper.findNavigation())).toBe(true);
+    expect(onNavigationChange).toHaveBeenCalledWith(expect.objectContaining({ detail: { open: false } }));
   });
 
   test('does not close navigation when anchor without href was clicked', () => {
+    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayoutWithControlledNavigation
-        initialNavigationOpen={true}
+      <AppLayout
+        navigationOpen={true}
+        onNavigationChange={onNavigationChange}
         navigation={
           <>
             <h1>Navigation</h1>
@@ -198,36 +118,28 @@ describeEachThemeAppLayout(true, theme => {
         }
       />
     );
-    // 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(false);
+
+    expect(onNavigationChange).not.toHaveBeenCalled();
   });
 
   test('does not close navigation when other elements were clicked', () => {
+    const onNavigationChange = jest.fn();
     const { wrapper } = renderComponent(
-      <AppLayoutWithControlledNavigation
-        initialNavigationOpen={true}
+      <AppLayout
+        navigationOpen={true}
+        onNavigationChange={onNavigationChange}
         navigation={
           <>
             <h1>Navigation</h1>
-            <a>Link</a>
+            <a href="#">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('h1')!.click();
-    expect(isDrawerClosed(wrapper.findNavigation())).toBe(false);
+
+    expect(onNavigationChange).not.toHaveBeenCalled();
   });
 
   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: (size: 'desktop' | 'mobile') => void) {
+export function describeEachAppLayout(callback: () => void) {
   for (const theme of ['refresh', 'classic']) {
-    for (const size of ['desktop', 'mobile'] as const) {
+    for (const size of ['desktop', 'mobile']) {
       describe(`Theme=${theme}, Size=${size}`, () => {
         beforeEach(() => {
           (useMobile as jest.Mock).mockReturnValue(size === 'mobile');
@@ -88,7 +88,7 @@ export function describeEachAppLayout(callback: (size: 'desktop' | 'mobile') =>
           (useMobile as jest.Mock).mockReset();
           (useVisualRefresh as jest.Mock).mockReset();
         });
-        callback(size);
+        callback();
       });
     }
   }

src/app-layout/index.tsx

@@ -170,11 +170,14 @@ const OldAppLayout = React.forwardRef(
       setLastInteraction: setDrawerLastInteraction,
     } = useDrawerFocusControl([activeDrawer?.resizable], toolsOpen || activeDrawer !== undefined, true);
 
-    const onNavigationToggle = useStableCallback((open: boolean) => {
-      setNavigationOpen(open);
-      focusNavButtons();
-      fireNonCancelableEvent(onNavigationChange, { open });
-    });
+    const onNavigationToggle = useCallback(
+      (open: boolean) => {
+        setNavigationOpen(open);
+        focusNavButtons();
+        fireNonCancelableEvent(onNavigationChange, { open });
+      },
+      [setNavigationOpen, onNavigationChange, focusNavButtons]
+    );
     const onToolsToggle = useCallback(
       (open: boolean) => {
         setToolsOpen(open);
@@ -194,13 +197,6 @@ 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 { useStableCallback, warnOnce } from '@cloudscape-design/component-toolkit/internal';
+import { 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';
@@ -179,18 +179,14 @@ export const AppLayoutInternalsProvider = React.forwardRef(
 
     const { refs: navigationRefs, setFocus: focusNavButtons } = useFocusControl(isNavigationOpen);
 
-    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]);
+    const handleNavigationClick = useCallback(
+      function handleNavigationChange(isOpen: boolean) {
+        setIsNavigationOpen(isOpen);
+        focusNavButtons();
+        fireNonCancelableEvent(props.onNavigationChange, { open: isOpen });
+      },
+      [props.onNavigationChange, setIsNavigationOpen, focusNavButtons]
+    );
 
     /**
      * The useControllable hook will set the default value and manage either

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

@@ -95,15 +95,14 @@ const ExpandableNavigationHeader = ({
   icon,
 }: ExpandableNavigationHeaderProps) => {
   return (
-    <div id={id} className={clsx(className, styles['click-target'])}>
+    <div id={id} className={clsx(className, styles['click-target'])} onClick={onClick}>
       <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,6 +227,9 @@ 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);
       }