Collapsible does not always handle growing children

[Bringer128]

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]

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]

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]

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.