Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collapsible does not always handle growing children #7318

Closed
Bringer128 opened this issue Sep 30, 2022 · 3 comments · Fixed by #7364
Closed

Collapsible does not always handle growing children #7318

Bringer128 opened this issue Sep 30, 2022 · 3 comments · Fixed by #7364

Comments

@Bringer128
Copy link
Contributor

Bringer128 commented Sep 30, 2022 •
edited
Loading

Issue summary

Collapsible occasionally does not recompute the size of its children even if they change their size appropriately within the React event cycle.

Expected behavior

As the children grow, all expanded Collapsible containers show their children

Actual behavior

Gif recording of last Collapsible element not resizing

Steps to reproduce the problem

A reproduction scenario is here

If you want to recreate the repro:

  1. Build a component that grows every second
  2. In another component, have a button to add a Collapsible with the growing element inside.
  3. Then click this button any number of times; the last added Collapsible will not grow but the others will

Reduced test case

See the problem here: https://codesandbox.io/s/quiet-leaf-bsrk37?file=/App.js

Specifications

  • Are you using the React components? (Y/N): Yes
  • Polaris version number: 10.4.1
  • Browser: Google Chrome
  • Device: MacBook Pro (14-inch, 2021); Apple M1 Pro
  • Operating System: MacOS Monterey 12.6 (21G115)
@Bringer128
Copy link
Contributor Author

Bringer128 commented Oct 3, 2022 •
edited
Loading

As I investigate this, it turns out that the component relies on onTransitionEnd firing to consider the "animation complete" and let the size of the component set to max-height: none. However if it is rendered as "open" to start with, then the event does not fire.

@Bringer128
Copy link
Contributor Author

Bringer128 commented Oct 6, 2022 •
edited
Loading

From investigation it looks like the problem is this change: #6100

Specifically, this change incorrectly fixed a bug in Secondary Navigation Items where the Collapsible would not change size if the children changed. However the root cause of that bug is the use of duration: 0ms as seen here to prevent the transition from occurring. This would cause the Collapsible to get stuck in the animating state, as the browser would not do the transition, and the onTransitionEnd callback would not fire.

When in the animating state, the max-height is set to a fixed value and overflow is hidden. This hides the contents of an extended list, which caused the original bug.

Unfortunately, this caused a couple of bugs. One is that the re-measure process when the component is already open is a bit janky, which is why this PR was made (see the janky animation here.

The other is that the PR made a measure -> animate cycle happen on first render. However it neglects this useEffect call which sets the height state variable. Entering the measure -> animate cycle in this state means that the height does not change on the div which means the transition does not occur.

Why did the original PR author not notice this? Because in Polaris by default, a <ul> has 14px margin on the top and bottom (which is how the Collapsible is used in Navigation). And due to a quirk in how the component measures scrollHeight the height changes twice during the measure -> animate cycle. During the measure state and the "first render" useEffect the max-height is first measured when overflow:visible is set. Then in the next render overflow:hidden is set, which then includes the extra margin in the scrollHeight. Because the height changes between the first render and the animating state, a transition does occur and onTransitionEnd does fire. By contrast, the example in this issue does not have a top and bottom margin, so gets stuck in the animating state.

TL;DR: The linked PR introduced this bug by trying to re-measure on children change. The correct fix is instead to avoid animating if duration: 0ms is used.

@Bringer128
Copy link
Contributor Author

Note: If we choose to add a feature for "animate while open" it will need more thought. The way this component is designed today only handles animating between "open" and "closed" states.

@Bringer128 Bringer128 mentioned this issue Oct 6, 2022
Merged
5 tasks
Bringer128 added a commit that referenced this issue Oct 13, 2022
@Bringer128
[Collapsible] Add support for disabling the transition, and fix state…
e4b2c36 
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Collapsible] Add support for disabling the transition, and fix state machine Shopify/polaris
1 participant
@Bringer128