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.

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

[material-ui] Prevent ownerState propagation for transition slots #44401

Merged
Changes from 24 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7ba4b13
remove ownerState if custom transition slot is provided
ZeeshanTamboli Nov 13, 2024
ac5ca15
prettier
ZeeshanTamboli Nov 13, 2024
3018428
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Nov 13, 2024
abc5ada
add description and test
ZeeshanTamboli Nov 13, 2024
131adc5
fix test
ZeeshanTamboli Nov 13, 2024
11dc5f5
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Nov 13, 2024
17478d6
Update packages/mui-material/src/utils/useSlot.ts
ZeeshanTamboli Nov 16, 2024
f4d4d07
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Nov 20, 2024
c435a13
prevent ownerState propagation in Zoom and Fade components
ZeeshanTamboli Nov 20, 2024
c3ac857
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Nov 20, 2024
fa69642
prevent ownerState in Slide and Grow
ZeeshanTamboli Nov 20, 2024
79cee1b
Add code comments
ZeeshanTamboli Nov 20, 2024
42cc23e
pnpm proptypes
ZeeshanTamboli Nov 20, 2024
4f9606f
pnpm proptypes
ZeeshanTamboli Nov 20, 2024
7ea5845
adjust comment formatting
ZeeshanTamboli Nov 20, 2024
988ac79
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Dec 13, 2024
7c0e6d1
add tests
ZeeshanTamboli Dec 14, 2024
a2287e6
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Dec 14, 2024
158ddde
add defaultExpanded
ZeeshanTamboli Dec 14, 2024
7eec13c
do not forward incoming ownerState in Collapse component
ZeeshanTamboli Dec 17, 2024
03e4eb8
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Dec 17, 2024
f52ac6e
add test
ZeeshanTamboli Dec 18, 2024
99b7f2d
improve
ZeeshanTamboli Dec 18, 2024
b875da5
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
ZeeshanTamboli Dec 18, 2024
6524aa5
Update packages/mui-material/src/Collapse/Collapse.test.js
DiegoAndai Dec 18, 2024
2da1a43
pnpm dedupe
DiegoAndai Dec 18, 2024
82c4ed8
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
DiegoAndai Dec 18, 2024
0fc6f3d
Merge branch 'master' into remove-ownerState-propogation-accordion-tr…
DiegoAndai Dec 18, 2024
8af558e
Merge branch 'remove-ownerState-propogation-accordion-transition-slot…
ZeeshanTamboli Dec 18, 2024
bffe3d7
Fix test
DiegoAndai Dec 18, 2024
52912dd
Merge branch 'remove-ownerState-propogation-accordion-transition-slot…
ZeeshanTamboli Dec 18, 2024
8511dc8
fix lint
ZeeshanTamboli Dec 18, 2024
ea60c29
pnpm dedupe
ZeeshanTamboli Dec 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion packages/mui-material/src/Accordion/Accordion.test.js
Original file line number Diff line number Diff line change
@@ -2,9 +2,14 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import { expect } from 'chai';
import { spy } from 'sinon';
import { createRenderer, fireEvent, reactMajor } from '@mui/internal-test-utils';
import { createRenderer, fireEvent, reactMajor, screen } from '@mui/internal-test-utils';
import Accordion, { accordionClasses as classes } from '@mui/material/Accordion';
import Paper from '@mui/material/Paper';
import Collapse from '@mui/material/Collapse';
import Fade from '@mui/material/Fade';
import Slide from '@mui/material/Slide';
import Grow from '@mui/material/Grow';
import Zoom from '@mui/material/Zoom';
import AccordionSummary from '@mui/material/AccordionSummary';
import describeConformance from '../../test/describeConformance';

@@ -261,4 +266,48 @@ describe('<Accordion />', () => {
expect(queryByTestId('details')).to.equal(null);
});
});

describe('should not forward ownerState prop to the underlying DOM element when using transition slot', () => {
const transitions = [
{
component: Collapse,
name: 'Collapse',
},
{
component: Fade,
name: 'Fade',
},
{
component: Grow,
name: 'Grow',
},
{
component: Slide,
name: 'Slide',
},
{
component: Zoom,
name: 'Zoom',
},
];

transitions.forEach((transition) => {
it(transition.name, () => {
render(
<Accordion
defaultExpanded
slots={{
transition: transition.component,
}}
slotProps={{ transition: { timeout: 400 } }}
>
<AccordionSummary>Summary</AccordionSummary>
Details
</Accordion>,
);

expect(screen.getByRole('region')).not.to.have.attribute('ownerstate');
});
});
});
});
8 changes: 1 addition & 7 deletions packages/mui-material/src/Backdrop/Backdrop.js
Original file line number Diff line number Diff line change
@@ -9,11 +9,6 @@ import useSlot from '../utils/useSlot';
import Fade from '../Fade';
import { getBackdropUtilityClass } from './backdropClasses';

const removeOwnerState = (props) => {
const { ownerState, ...rest } = props;
return rest;
};

const useUtilityClasses = (ownerState) => {
const { classes, invisible } = ownerState;

@@ -101,10 +96,9 @@ const Backdrop = React.forwardRef(function Backdrop(inProps, ref) {
externalForwardedProps,
ownerState,
});
const transitionPropsRemoved = removeOwnerState(transitionProps);

return (
<TransitionSlot in={open} timeout={transitionDuration} {...other} {...transitionPropsRemoved}>
<TransitionSlot in={open} timeout={transitionDuration} {...other} {...transitionProps}>
<RootSlot aria-hidden {...rootProps} classes={classes} ref={ref}>
{children}
</RootSlot>
7 changes: 3 additions & 4 deletions packages/mui-material/src/Collapse/Collapse.js
Original file line number Diff line number Diff line change
@@ -308,7 +308,8 @@ const Collapse = React.forwardRef(function Collapse(inProps, ref) {
timeout={timeout === 'auto' ? null : timeout}
{...other}
>
{(state, childProps) => (
{/* Destructure child props to prevent the component's "ownerState" from being overridden by incomingOwnerState. */}
{(state, { ownerState: incomingOwnerState, ...restChildProps }) => (
<CollapseRoot
as={component}
className={clsx(
@@ -324,10 +325,8 @@ const Collapse = React.forwardRef(function Collapse(inProps, ref) {
...style,
}}
ref={handleRef}
{...childProps}
// `ownerState` is set after `childProps` to override any existing `ownerState` property in `childProps`
// that might have been forwarded from the Transition component.
ownerState={{ ...ownerState, state }}
{...restChildProps}
>
<CollapseWrapper
ownerState={{ ...ownerState, state }}
15 changes: 15 additions & 0 deletions packages/mui-material/src/Collapse/Collapse.test.js
Original file line number Diff line number Diff line change
@@ -268,4 +268,19 @@ describe('<Collapse />', () => {
expect(handleExiting.args[0][0].style.height).to.equal(collapsedSize);
});
});

// Test for https://github.com/mui/material-ui/issues/40653
it('should render correctly when external ownerState prop is passed', () => {
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}

const { container } = render(<Collapse {...defaultProps} ownerState={{}} />);
const collapse = container.firstChild;

// height: auto is applied when the component has completed transition and is in entered state
expect(collapse).toHaveComputedStyle({
height: 'auto',
});
});
});
5 changes: 3 additions & 2 deletions packages/mui-material/src/Fade/Fade.js
Original file line number Diff line number Diff line change
@@ -128,7 +128,8 @@ const Fade = React.forwardRef(function Fade(props, ref) {
timeout={timeout}
{...other}
>
{(state, childProps) => {
{/* Ensure "ownerState" is not forwarded to the child DOM element when a direct HTML element is used. This avoids unexpected behavior since "ownerState" is intended for internal styling, component props and not as a DOM attribute. */}
{(state, { ownerState, ...restChildProps }) => {
return React.cloneElement(children, {
style: {
opacity: 0,
@@ -138,7 +139,7 @@ const Fade = React.forwardRef(function Fade(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
5 changes: 3 additions & 2 deletions packages/mui-material/src/Grow/Grow.js
Original file line number Diff line number Diff line change
@@ -189,7 +189,8 @@ const Grow = React.forwardRef(function Grow(props, ref) {
timeout={timeout === 'auto' ? null : timeout}
{...other}
>
{(state, childProps) => {
{/* Ensure "ownerState" is not forwarded to the child DOM element when a direct HTML element is used. This avoids unexpected behavior since "ownerState" is intended for internal styling, component props and not as a DOM attribute. */}
{(state, { ownerState, ...restChildProps }) => {
return React.cloneElement(children, {
style: {
opacity: 0,
@@ -200,7 +201,7 @@ const Grow = React.forwardRef(function Grow(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
5 changes: 3 additions & 2 deletions packages/mui-material/src/Slide/Slide.js
Original file line number Diff line number Diff line change
@@ -252,15 +252,16 @@ const Slide = React.forwardRef(function Slide(props, ref) {
timeout={timeout}
{...other}
>
{(state, childProps) => {
{/* Ensure "ownerState" is not forwarded to the child DOM element when a direct HTML element is used. This avoids unexpected behavior since "ownerState" is intended for internal styling, component props and not as a DOM attribute. */}
{(state, { ownerState, ...restChildProps }) => {
return React.cloneElement(children, {
ref: handleRef,
style: {
visibility: state === 'exited' && !inProp ? 'hidden' : undefined,
...style,
...children.props.style,
},
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
5 changes: 3 additions & 2 deletions packages/mui-material/src/Zoom/Zoom.js
Original file line number Diff line number Diff line change
@@ -128,7 +128,8 @@ const Zoom = React.forwardRef(function Zoom(props, ref) {
timeout={timeout}
{...other}
>
{(state, childProps) => {
{/* Ensure "ownerState" is not forwarded to the child DOM element when a direct HTML element is used. This avoids unexpected behavior since "ownerState" is intended for internal styling, component props and not as a DOM attribute. */}
{(state, { ownerState, ...restChildProps }) => {
return React.cloneElement(children, {
style: {
transform: 'scale(0)',
@@ -138,7 +139,7 @@ const Zoom = React.forwardRef(function Zoom(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>