[material-ui][Transitions] External ownerState is incorrectly forwarded to inner components

[DiegoAndai]

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/cold-firefly-dpmqj7?file=%2Fsrc%2FDemo.tsx%3A29%2C5

Steps:

1. Provide an ownerState prop to Collapse transition
2. Set transition as in={true} initially

Current behavior

The transition is collapsed initially, even though it shouldn't.

Expected behavior

The transition should be expanded initially.

Context

This is a very mild symptom of a bigger underlying problem: If an external ownerState is provided to a transition component, then that ownerState is forwarded to the inner components and treated as the component's own ownerState. Here's a trace of this using Collapse as an example:

• The Collapse component receives an ownerState prop
• The ownerState property goes into ...other
• The other is provided to Transition
• Transition does not recognize the ownerState prop, so it forwards it as childProps
• childProps.ownerState overrides Collapse's own ownerState, and the animation breaks.
  

The symptom is only perceivable on the initial render as for subsequent enter calls, the size in handled on the onEntered event.

The impact of this is that the transition components are unsuitable for usage with the slots pattern: We would like to be able to provide the ownerState for custom transitions used in components' slots, but this could have unintended consequences because of the behavior explained above.

Another symptom this has is that the Collapse transition does not work for the Tooltip component, as it provides ownerState to the transition component.

It is not very noticeable at the moment as the Fade, Grow, Slide, and Zoom transitions do not rely on an internal ownerState.

Discovered while working on #40418. When fixing this issue, we should remove this workaround.

Your environment

npx @mui/envinfo

  System:
    OS: macOS 13.4.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 28.03 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.19.0 - ~/.nvm/versions/node/v18.19.0/bin/node
    npm: 10.2.3 - ~/.nvm/versions/node/v18.19.0/bin/npm
    pnpm: 8.13.1 - ~/.nvm/versions/node/v18.19.0/bin/pnpm
  Managers:
    Homebrew: 4.2.0 - /usr/local/bin/brew
    pip3: 21.2.4 - /usr/bin/pip3
    RubyGems: 3.0.3.1 - /usr/bin/gem
  Utilities:
    Make: 3.81 - /usr/bin/make
    GCC: 14.0.3 - /usr/bin/gcc
    Git: 2.39.2 - /usr/bin/git
    Clang: 14.0.3 - /usr/bin/clang
    Curl: 7.88.1 - /usr/bin/curl
  Servers:
    Apache: 2.4.56 - /usr/sbin/apachectl
  IDEs:
    VSCode: 1.85.1 - /usr/local/bin/code
    Vim: 9.0 - /usr/bin/vim
    Xcode: /undefined - /usr/bin/xcodebuild
  Languages:
    Bash: 3.2.57 - /bin/bash
    Perl: 5.30.3 - /usr/bin/perl
    Python3: 3.9.6 - /usr/bin/python3
    Ruby: 2.6.10 - /usr/bin/ruby
  Databases:
    SQLite: 3.39.5 - /usr/bin/sqlite3
  Browsers:
    Chrome: 120.0.6099.216
    Safari: 16.5.1
  Monorepos:
    Lerna: 8.0.2

Search keywords: transitions collapse ownerstate

[gitstart]

@DiegoAndai we would like to pick this up

[DiegoAndai]

@gitstart, sure; let me know if you need help. Thanks.

[gitstart]

@DiegoAndai

What are your thoughts about us setting this prop after the spread operation of childProp? This way, the forwarded ownerState from the Transition component won't be able to override the internal one. By doing so, we can revert the delete operation here, and ownerState can then be forwarded to the custom transition component if needed.

[DiegoAndai]

@gitstart

What are your thoughts about us setting this prop after the spread operation of childProp?

I think it would be better to avoid destructuring ownerState to ..other here, so it never goes through childProps. That's more explicit for future maintainers, so it would be harder to overlook. What do you think?

[gitstart]

@DiegoAndai, for the benefit of future maintainers, we can add comments that explicitly explain the rationale behind relocating the ownerState prop after the childProp destructuring.

Even though the ownerState is abstracted from ...other , it will eventually be passed to the transition component because we want to allow users access to the ownerState in custom transition components. While we could attempt to check if users provide a custom transition component, the issue may still persist if the custom transition component provided by the user is the same as the one used by MUI that doesn't handle ownerState.

[DiegoAndai]

Even though the ownerState is abstracted from ...other , it will eventually be passed to the transition component because we want to allow users access to the ownerState in custom transition components.

Good point. Let's try moving the ownerState prop after the childProps spread as you suggested.