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

… machine (#7364) ### WHY are these changes introduced? Fixes #7318 Today the Collapsible component can get stuck in the "animating" state for 2 reasons, both of which cause the `onTransitionEnd` event not to fire. 1. The prop `transition={{ duration: "0ms" }}` is used as [Navigation Items](https://github.com/Shopify/polaris/blob/b0445cf9b036752062af215da57347ce4b8f6f17/polaris-react/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx#L19) does. 2. The children of the Collapsible have no margin and it is rendered for the first time with `open=true` (as raised in the linked issue). This causes the `div` to have `max-height` not change, which means the transition does not occur. Fundamentally the issue with this component is that it relies on the `onTransitionEnd` event to move out of the `animating` state. There are 2 ways that this event won't fire, similarly related: 1. If the animation is started but the height doesn't change (e.g. from 0px to 0px) 2. If the animation is started but the duration is set to 0 (`0ms`, `0s` or `-some-var-that-evaluates-to-0`) This PR does not solve the dependency on the `onTransitionEnd` event. Instead it: 1. Adds formal support for disabling the transition by setting `transition={false}`. This prevents the need to pass `0ms` duration. (Note this does not stop the bug occurring) 2. Handles if the user passes `0ms` or `0s` to also disable the transition. 3. Removes the "re-measure on child update" logic, as this was a workaround for the fact that the component was getting stuck in the animating state. As such there will still be a bug if the computed height of `children` is set to 0px. However I'm choosing not to address this in this PR as the chances are rare. To trigger this bug you must: 1. Render the Collapsible in closed state (`open={false}`) 2. Toggle the open state to true, while the children is `0px` high as computed by `element.scrollHeight` 4. Change the children to have content ### WHAT is this pull request doing? 1. It reverts the changes from #6283 and #6100 which caused the linked bug. 5. Adds support for passing `false` to the `transition` prop to disable the transition. 6. Changes the `Navigation` `Secondary` component to use `transition={false}` ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import {HomeMinor, OrdersMinor, ProductsMinor} from '@shopify/polaris-icons'; import React, {useState} from 'react'; import {Button, Card, Collapsible, Frame, Navigation} from '../src'; const list1 = [ {label: 'a', url: 'path/to/one'}, {label: 'b', url: 'path/to/two'}, ]; const list2 = [ {label: 'one', url: 'path/to/one'}, {label: 'two', url: 'path/to/two'}, {label: 'three', url: 'path/to/three'}, ]; const list3 = [ {label: '1', url: 'path/to/one'}, {label: '2', url: 'path/to/two'}, {label: '3', url: 'path/to/three'}, {label: '4', url: 'path/to/four'}, {label: '5', url: 'path/to/five'}, ]; export function Playground() { const [open, setOpen] = useState(true); const toggleOpen = () => { setOpen(!open); }; const scenario1 = ( <div> <h1>Scenario 1</h1> <Collapsible open={open} id="1"> <ul> <li>List item</li> </ul> </Collapsible> </div> ); const scenario2 = ( <div> <h1>Scenario 2</h1> <button onClick={toggleOpen}>Toggle open</button> <Collapsible open={open} id="2"> <div>Hello</div> </Collapsible> </div> ); const [selected, setSelected] = useState('home'); const [subitems, setSubitems] = useState(list2); const scenario3 = ( <Frame> <button onClick={() => setSubitems(list1)}>list 1</button> <button onClick={() => setSubitems(list2)}>list 2</button> <button onClick={() => setSubitems(list3)}>list 3</button> <Navigation location="/"> <Navigation.Section items={[ { url: '/', label: 'Home', icon: HomeMinor, selected: selected === 'home', onClick: () => setSelected('home'), }, { url: '/', label: 'Orders', icon: OrdersMinor, subNavigationItems: subitems, selected: selected === 'orders', onClick: () => setSelected('orders'), }, { url: '/', label: 'Products', icon: ProductsMinor, selected: selected === 'products', onClick: () => setSelected('products'), }, ]} /> <Navigation.Item url="/" label="Expected List" subNavigationItems={subitems} /> </Navigation> </Frame> ); const scenario4 = ( <Card> Scenario 4 <Collapsible id="4" open={open} transition={{duration: '100ms'}}> Content goes here </Collapsible> </Card> ); const [firstOpen, setFirstOpen] = useState(false); const [secondOpen, setSecondOpen] = useState(false); const scenario5 = ( <Card> <h1>Scenario 5</h1> <button onClick={() => setFirstOpen(!firstOpen)}>First</button> <button onClick={() => setSecondOpen(!secondOpen)}>Second</button> <Collapsible id="5" open={firstOpen} transition={{duration: '100ms'}}> <Collapsible id="5.1" open={secondOpen}> <div style={{width: '100px', height: '100px', backgroundColor: 'purple'}} /> </Collapsible> </Collapsible> </Card> ); const scenario6 = ( <Card title="Scenario 6"> <Collapsible id="6" open={open}> <div style={{height: 0}} /> </Collapsible> </Card> ); return ( <div> <button onClick={toggleOpen}>Toggle open</button> {scenario1} {scenario2} {scenario3} {scenario4} {scenario5} {scenario6} </div> ); } ``` </details> The code has 6 scenarios: - A `Collapsible` with `ul` as its child (this has margin-top and margin-bottom) - A `Collapsible` with `div` as its child (this has no margins) - The original scenario from #6100 to show that this does not cause a regression. - Duration 100ms explicitly set - Nested collapsibles - A `Collapsible` that has children that is 0px high (not fixed by this PR) ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
Shopify · Oct 13, 2022 · e4b2c36 · e4b2c36
1 parent 8a6c323
commit e4b2c36
Show file tree

Hide file tree

Showing 5 changed files with 213 additions and 81 deletions.
diff --git a/.changeset/light-houses-repeat.md b/.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 when duration is 0.
+Add support for intentionally disabling the transition in Collapsible.
diff --git a/polaris-react/src/components/Collapsible/Collapsible.tsx b/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.
+   * @default transition={{duration: 'var(--p-duration-150)', timingFunction: 'var(--p-ease-in-out)'}}
+   */
+  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,15 @@ export function Collapsible({
     expandOnPrint && styles.expandOnPrint,
   );
 
+  const transitionDisabled = isTransitionDisabled(transition);
+
+  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 +78,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(() => {
@@ -118,3 +136,17 @@ export function Collapsible({
     </div>
   );
 }
+
+const zeroDurationRegex = /^0(ms|s)$/;
+
+function isTransitionDisabled(transitionProp: Transition | boolean) {
+  if (typeof transitionProp === 'boolean') {
+    return !transitionProp;
+  }
+
+  const {duration} = transitionProp;
+  if (duration && zeroDurationRegex.test(duration.trim())) {
+    return true;
+  }
+  return false;
+}
diff --git a/polaris-react/src/components/Collapsible/tests/Collapsible.test.tsx b/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: expect.objectContaining({
+      maxHeight: 'none',
+      overflow: 'visible',
+    }),
+  };
+  const fullyClosedProps = {
+    'aria-hidden': true,
+    className: expect.stringContaining('isFullyClosed'),
+    style: expect.objectContaining({
+      maxHeight: '0px',
+      overflow: 'hidden',
+    }),
+  };
+  const animatingOpenProps = {
+    'aria-hidden': false,
+    style: expect.objectContaining({
+      maxHeight: '2px',
+      overflow: 'hidden',
+    }),
+  };
+  const animatingClosedProps = {
+    'aria-hidden': true,
+    className: expect.not.stringContaining('isFullyClosed'),
+    style: expect.objectContaining({
+      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>
+        content
+      </Collapsible>,
+    );
+
+    expect(collapsible).toContainReactComponent('div', fullyOpenProps);
+  });
+
+  it('begins animation when toggled open', () => {
+    const collapsible = mountWithApp(
+      <Collapsible id="test-collapsible" open={false}>
+        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>
+        content
+      </Collapsible>,
+    );
+
+    collapsible.setProps({open: false});
+
+    expect(collapsible).toContainReactComponent('div', animatingClosedProps);
+  });
+
   describe('Transition', () => {
     it('passes a duration property', () => {
       const duration = '150ms';
@@ -88,96 +165,119 @@ describe('<Collapsible />', () => {
 
       expect(collapsible).toHaveReactProps({transition: {timingFunction}});
     });
+
+    const transitionDisabledOptions = [
+      false,
+      {duration: '0s'},
+      {duration: '0ms'},
+    ];
+
+    it.each(transitionDisabledOptions)(
+      'does not animate when transition is disabled with %p',
+      (transition) => {
+        const collapsible = mountWithApp(
+          <Collapsible
+            id="test-collapsible"
+            open={false}
+            transition={transition}
+          >
+            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,
+  });
 }
diff --git a/polaris-react/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx b/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>
   );