[Collapsible] Add support for disabling the transition, and fix state machine

.changeset/light-houses-repeat.md

@@ -0,0 +1,7 @@
+---
+'@shopify/polaris': minor
+---
+
+Deprecated Collapsible preventMeasuringOnChildrenUpdate.
+Fixed bug where Collapsible would get stuck in animating state.
+Add support for intentionally disabling the transition in Collapsible.

polaris-react/src/components/Collapsible/Collapsible.tsx

@@ -18,9 +18,11 @@ export interface CollapsibleProps {
   expandOnPrint?: boolean;
   /** Toggle whether the collapsible is expanded or not. */
   open: boolean;
-  /** Assign transition properties to the collapsible */
-  transition?: Transition;
-  /** Prevents component from re-measuring when child is updated **/
+  /** Override transition properties. When set to false, disables transition completely. Defaults to true, which uses
+   * default transition properties.
+   */
+  transition?: boolean | Transition;
+  /** @deprecated Re-measuring is no longer necessary on children update **/
   preventMeasuringOnChildrenUpdate?: boolean;
   /** The content to display inside the collapsible. */
   children?: React.ReactNode;
@@ -32,8 +34,8 @@ export function Collapsible({
   id,
   expandOnPrint,
   open,
-  transition,
-  preventMeasuringOnChildrenUpdate,
+  transition = true,
+  preventMeasuringOnChildrenUpdate: _preventMeasuringOnChildrenUpdate,
   children,
 }: CollapsibleProps) {
   const [height, setHeight] = useState(0);
@@ -51,11 +53,14 @@ export function Collapsible({
     expandOnPrint && styles.expandOnPrint,
   );
 
+  const transitionDisabled = transition === false;
+
+  const transitionStyles = typeof transition === 'object' && {
+    transitionDuration: transition.duration,
+    transitionTimingFunction: transition.timingFunction,
+  };
   const collapsibleStyles = {
-    ...(transition && {
-      transitionDuration: `${transition.duration}`,
-      transitionTimingFunction: `${transition.timingFunction}`,
-    }),
+    ...transitionStyles,
     ...{
       maxHeight: isFullyOpen ? 'none' : `${height}px`,
       overflow: isFullyOpen ? 'visible' : 'hidden',
@@ -72,15 +77,27 @@ export function Collapsible({
     [open],
   );
 
-  useEffect(() => {
-    if (isFullyClosed || preventMeasuringOnChildrenUpdate) return;
-    setAnimationState('measuring');
-  }, [children, isFullyClosed, preventMeasuringOnChildrenUpdate]);
+  const startAnimation = useCallback(() => {
+    if (transitionDisabled) {
+      setIsOpen(open);
+      setAnimationState('idle');
+
+      if (open && collapsibleContainer.current) {
+        setHeight(collapsibleContainer.current.scrollHeight);
+      } else {
+        setHeight(0);
+      }
+    } else {
+      setAnimationState('measuring');
+    }
+  }, [open, transitionDisabled]);
 
   useEffect(() => {
     if (open !== isOpen) {
-      setAnimationState('measuring');
+      startAnimation();
     }
+    // startAnimation should only be fired if the open state changes.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [open, isOpen]);
 
   useEffect(() => {

polaris-react/src/components/Collapsible/tests/Collapsible.test.tsx

@@ -1,9 +1,52 @@
-import React, {useState, useCallback} from 'react';
-import {mountWithApp} from 'tests/utilities';
+import React from 'react';
+import {CustomRoot, mountWithApp} from 'tests/utilities';
 
+import type {WithPolarisTestProviderOptions} from '../../PolarisTestProvider';
 import {Collapsible, CollapsibleProps} from '../Collapsible';
 
+const mockScrollHeight = 2;
+
 describe('<Collapsible />', () => {
+  const fullyOpenProps = {
+    'aria-hidden': false,
+    style: {
+      maxHeight: 'none',
+      overflow: 'visible',
+    },
+  };
+  const fullyClosedProps = {
+    'aria-hidden': true,
+    className: expect.stringContaining('isFullyClosed'),
+    style: {
+      maxHeight: '0px',
+      overflow: 'hidden',
+    },
+  };
+  const animatingOpenProps = {
+    'aria-hidden': false,
+    style: {
+      maxHeight: '2px',
+      overflow: 'hidden',
+    },
+  };
+  const animatingClosedProps = {
+    'aria-hidden': true,
+    className: expect.not.stringContaining('isFullyClosed'),
+    style: {
+      maxHeight: '0px',
+      overflow: 'hidden',
+    },
+  };
+
+  beforeEach(() => {
+    const mockScrollHeightFn = jest.spyOn(
+      Element.prototype,
+      'scrollHeight',
+      'get',
+    );
+    mockScrollHeightFn.mockReturnValue(mockScrollHeight);
+  });
+
   it('does not render its children and indicates hidden with aria-hidden', () => {
     const collapsible = mountWithApp(
       <Collapsible id="test-collapsible" open={false}>
@@ -66,6 +109,40 @@ describe('<Collapsible />', () => {
     });
   });
 
+  it('does not animate when rendered open', () => {
+    const collapsible = mountWithApp(
+      <Collapsible id="test-collapsible" open expandOnPrint>
+        content
+      </Collapsible>,
+    );
+
+    expect(collapsible).toContainReactComponent('div', fullyOpenProps);
+  });
+
+  it('begins animation when toggled open', () => {
+    const collapsible = mountWithApp(
+      <Collapsible id="test-collapsible" open={false} expandOnPrint>
+        content
+      </Collapsible>,
+    );
+
+    collapsible.setProps({open: true});
+
+    expect(collapsible).toContainReactComponent('div', animatingOpenProps);
+  });
+
+  it('begins animation when toggled closed', () => {
+    const collapsible = mountWithApp(
+      <Collapsible id="test-collapsible" open expandOnPrint>
+        content
+      </Collapsible>,
+    );
+
+    collapsible.setProps({open: false});
+
+    expect(collapsible).toContainReactComponent('div', animatingClosedProps);
+  });
+
   describe('Transition', () => {
     it('passes a duration property', () => {
       const duration = '150ms';
@@ -88,96 +165,106 @@ describe('<Collapsible />', () => {
 
       expect(collapsible).toHaveReactProps({transition: {timingFunction}});
     });
+
+    it('does not animate when transition is set to false', () => {
+      const collapsible = mountWithApp(
+        <Collapsible id="test-collapsible" open={false} transition={false}>
+          content
+        </Collapsible>,
+      );
+
+      collapsible.setProps({open: true});
+
+      expect(collapsible).toContainReactComponent('div', fullyOpenProps);
+
+      collapsible.setProps({open: false});
+
+      expect(collapsible).toContainReactComponent('div', fullyClosedProps);
+    });
   });
 
   describe('onTransitionEnd', () => {
-    it('adds an isFullyClosed class to the collapsible onTransitionEnd if the event target is the collapsible div', () => {
+    it('completes opening transition', () => {
       const id = 'test-collapsible';
-      const collapsibleWithToggle = mountWithApp(
-        <CollapsibleWithToggle id={id} />,
+      const collapsible = mountWithApp<CollapsibleProps>(
+        <Collapsible id={id} open={false}>
+          content
+        </Collapsible>,
       );
-      collapsibleWithToggle.find('button')!.trigger('onClick');
+      collapsible.setProps({open: true});
 
-      const wrapper = collapsibleWithToggle.find('div', {id})!;
-      wrapper.trigger('onTransitionEnd', {
-        target: wrapper.domNode as HTMLDivElement,
-      });
+      fireTransitionEnd(collapsible);
 
-      expect(
-        collapsibleWithToggle.find('div', {
-          id,
-          'aria-hidden': true,
-          className: 'Collapsible isFullyClosed',
-        }),
-      ).not.toBeNull();
+      expect(collapsible).toContainReactComponent('div', {
+        ...fullyOpenProps,
+        id,
+      });
     });
 
-    it('does not add an isFullyClosed class to the collapsible onTransitionEnd if the event target is not the collapsible div', () => {
+    it('completes closing transition', () => {
       const id = 'test-collapsible';
-      const collapsibleWithToggle = mountWithApp(
-        <CollapsibleWithToggle id={id} />,
+      const collapsible = mountWithApp<CollapsibleProps>(
+        <Collapsible id={id} open>
+          content
+        </Collapsible>,
       );
-      collapsibleWithToggle.find('button')!.trigger('onClick');
 
-      collapsibleWithToggle.find('div', {id})!.trigger('onTransitionEnd', {
-        target: document.createElement('div'),
-      });
+      collapsible.setProps({open: false});
 
-      expect(
-        collapsibleWithToggle.find('div', {
-          id,
-          'aria-hidden': true,
-          className: 'Collapsible',
-        }),
-      ).not.toBeNull();
+      fireTransitionEnd(collapsible);
+
+      expect(collapsible).toContainReactComponent('div', {
+        ...fullyClosedProps,
+        id,
+      });
     });
-  });
 
-  describe('preventMeasuringOnChildrenUpdate', () => {
-    it('does not re-measure on child update when preventMeasuringOnChildUpdate is true', () => {
+    it('does not complete opening transition if onTransitionEnd fires on a different target', () => {
       const id = 'test-collapsible';
-
-      const collapsible = mountWithApp(
-        <Collapsible id={id} open preventMeasuringOnChildrenUpdate>
-          <div>Foo</div>
+      const collapsible = mountWithApp<CollapsibleProps>(
+        <Collapsible id={id} open={false}>
+          content
         </Collapsible>,
       );
 
-      collapsible.setProps({children: <div>Bar</div>});
+      collapsible.setProps({open: true});
+      fireTransitionEnd(collapsible, true);
 
       expect(collapsible).toContainReactComponent('div', {
+        ...animatingOpenProps,
         id,
-        style: {maxHeight: 'none', overflow: 'visible'},
       });
     });
 
-    it('re-measures on child update when preventMeasuringOnChildUpdate is false', () => {
+    it('does not complete closing transition if onTransitionEnd fires on a different target', () => {
       const id = 'test-collapsible';
-
-      const collapsible = mountWithApp(
-        <Collapsible id={id} open preventMeasuringOnChildrenUpdate={false}>
-          <div>Foo</div>
+      const collapsible = mountWithApp<CollapsibleProps>(
+        <Collapsible id={id} open>
+          content
         </Collapsible>,
       );
 
-      collapsible.setProps({children: <div>Bar</div>});
+      collapsible.setProps({open: false});
+
+      fireTransitionEnd(collapsible, true);
 
       expect(collapsible).toContainReactComponent('div', {
+        ...animatingClosedProps,
         id,
-        style: {maxHeight: '0px', overflow: 'hidden'},
       });
     });
   });
 });
 
-function CollapsibleWithToggle(props: Omit<CollapsibleProps, 'open'>) {
-  const [open, setOpen] = useState(true);
-  const handleToggle = useCallback(() => setOpen((open) => !open), []);
-
-  return (
-    <>
-      <button onClick={handleToggle}>Activator</button>
-      <Collapsible {...props} open={open} />
-    </>
-  );
+function fireTransitionEnd(
+  collapsible: CustomRoot<CollapsibleProps, WithPolarisTestProviderOptions>,
+  fireOnWrongTarget = false,
+) {
+  const id = collapsible.props.id;
+  const div = collapsible.find('div', {id});
+  const correctTarget = div!.domNode as HTMLDivElement;
+  const wrongTarget = document.createElement('div');
+  div!.trigger('onTransitionEnd', {
+    target: fireOnWrongTarget ? wrongTarget : correctTarget,
+  });
 }

polaris-react/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx

@@ -13,11 +13,7 @@ interface SecondaryProps {
 export function Secondary({id, children, expanded}: SecondaryProps) {
   const uid = useUniqueId('SecondaryNavigation');
   return (
-    <Collapsible
-      id={id || uid}
-      open={expanded}
-      transition={{duration: '0ms', timingFunction: 'linear'}}
-    >
+    <Collapsible id={id || uid} open={expanded} transition={false}>
       <ul className={styles.List}>{children}</ul>
     </Collapsible>
   );

polaris-react/src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx

@@ -21,13 +21,10 @@ describe('Secondary()', () => {
     });
   });
 
-  it('adds custom transition props to Collapsible', () => {
+  it('disables Collapsible transition', () => {
     const component = mountWithApp(<Secondary expanded />);
     expect(component).toContainReactComponent(Collapsible, {
-      transition: {
-        duration: expect.any(String),
-        timingFunction: expect.any(String),
-      },
+      transition: false,
     });
   });