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
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Up @@ -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;

Expand Down Expand Up @@ -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>
Expand Down
7 changes: 3 additions & 4 deletions packages/mui-material/src/Collapse/Collapse.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 }}
Expand Down
18 changes: 18 additions & 0 deletions packages/mui-material/src/Collapse/Collapse.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,4 +268,22 @@ 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', function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}

const { container } = render(
<Collapse in ownerState={{}}>
<div style={{ height: '100px' }} />
</Collapse>,
);
const collapse = container.firstChild;

expect(collapse).toHaveComputedStyle({
height: '100px',
});
});
});
5 changes: 3 additions & 2 deletions packages/mui-material/src/Fade/Fade.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -138,7 +139,7 @@ const Fade = React.forwardRef(function Fade(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Grow/Grow.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -200,7 +201,7 @@ const Grow = React.forwardRef(function Grow(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Slide/Slide.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Zoom/Zoom.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)',
Expand All @@ -138,7 +139,7 @@ const Zoom = React.forwardRef(function Zoom(props, ref) {
...children.props.style,
},
ref: handleRef,
...childProps,
...restChildProps,
});
}}
</TransitionComponent>
Expand Down
Loading