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

[Slider] Fix override of event.target when preparing change events #24782

Merged
merged 9 commits into from Feb 12, 2021
5 changes: 1 addition & 4 deletions docs/src/pages/components/slider/ContinuousSlider.tsx
Expand Up @@ -9,10 +9,7 @@ import VolumeUp from '@material-ui/icons/VolumeUp';
export default function ContinuousSlider() {
const [value, setValue] = React.useState<number>(30);

const handleChange = (
event: React.SyntheticEvent,
newValue: number | number[],
) => {
const handleChange = (event: Event, newValue: number | number[]) => {
setValue(newValue as number);
};

Expand Down
5 changes: 1 addition & 4 deletions docs/src/pages/components/slider/InputSlider.tsx
Expand Up @@ -16,10 +16,7 @@ export default function InputSlider() {
30,
);

const handleSliderChange = (
event: React.SyntheticEvent,
newValue: number | number[],
) => {
const handleSliderChange = (event: Event, newValue: number | number[]) => {
setValue(newValue);
};

Expand Down
5 changes: 1 addition & 4 deletions docs/src/pages/components/slider/NonLinearSlider.tsx
Expand Up @@ -24,10 +24,7 @@ function calculateValue(value: number) {
export default function NonLinearSlider() {
const [value, setValue] = React.useState<number>(10);

const handleChange = (
event: React.SyntheticEvent,
newValue: number | number[],
) => {
const handleChange = (event: Event, newValue: number | number[]) => {
if (typeof newValue === 'number') {
setValue(newValue);
}
Expand Down
5 changes: 1 addition & 4 deletions docs/src/pages/components/slider/RangeSlider.tsx
Expand Up @@ -10,10 +10,7 @@ function valuetext(value: number) {
export default function RangeSlider() {
const [value, setValue] = React.useState<number[]>([20, 37]);

const handleChange = (
event: React.SyntheticEvent,
newValue: number | number[],
) => {
const handleChange = (event: Event, newValue: number | number[]) => {
setValue(newValue as number[]);
};

Expand Down
2 changes: 1 addition & 1 deletion docs/translations/api-docs/select/select.json
Expand Up @@ -16,7 +16,7 @@
"MenuProps": "Props applied to the <a href=\"/api/menu/\"><code>Menu</code></a> element.",
"multiple": "If <code>true</code>, <code>value</code> must be an array and the menu will support multiple selections.",
"native": "If <code>true</code>, the component uses a native <code>select</code> element.",
"onChange": "Callback fired when a menu item is selected.<br><br><strong>Signature:</strong><br><code>function(event: object, child?: object) =&gt; void</code><br><em>event:</em> The event source of the callback. You can pull out the new value by accessing <code>event.target.value</code> (any).<br><em>child:</em> The react element that was selected when <code>native</code> is <code>false</code> (default).",
"onChange": "Callback fired when a menu item is selected.<br><br><strong>Signature:</strong><br><code>function(event: object, child?: object) =&gt; void</code><br><em>event:</em> The event source of the callback. You can pull out the new value by accessing <code>event.target.value</code> (any). <strong>Warning</strong>: This is a generic event not a change event.<br><em>child:</em> The react element that was selected when <code>native</code> is <code>false</code> (default).",
"onClose": "Callback fired when the component requests to be closed. Use in controlled mode (see open).<br><br><strong>Signature:</strong><br><code>function(event: object) =&gt; void</code><br><em>event:</em> The event source of the callback.",
"onOpen": "Callback fired when the component requests to be opened. Use in controlled mode (see open).<br><br><strong>Signature:</strong><br><code>function(event: object) =&gt; void</code><br><em>event:</em> The event source of the callback.",
"open": "If <code>true</code>, the component is shown. You can only use it when the <code>native</code> prop is <code>false</code> (default).",
Expand Down
Expand Up @@ -17,7 +17,7 @@
"max": "The maximum allowed value of the slider. Should not be equal to min.",
"min": "The minimum allowed value of the slider. Should not be equal to max.",
"name": "Name attribute of the hidden <code>input</code> element.",
"onChange": "Callback function that is fired when the slider&#39;s value changed.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"onChange": "Callback function that is fired when the slider&#39;s value changed.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. You can pull out the new value by accessing <code>event.target.value</code> (any). <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"onChangeCommitted": "Callback function that is fired when the <code>mouseup</code> is triggered.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"orientation": "The component orientation.",
"scale": "A transformation function, to change the scale of the slider.",
Expand Down
2 changes: 1 addition & 1 deletion docs/translations/api-docs/slider/slider.json
Expand Up @@ -17,7 +17,7 @@
"max": "The maximum allowed value of the slider. Should not be equal to min.",
"min": "The minimum allowed value of the slider. Should not be equal to max.",
"name": "Name attribute of the hidden <code>input</code> element.",
"onChange": "Callback function that is fired when the slider&#39;s value changed.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"onChange": "Callback function that is fired when the slider&#39;s value changed.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. You can pull out the new value by accessing <code>event.target.value</code> (any). <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"onChangeCommitted": "Callback function that is fired when the <code>mouseup</code> is triggered.<br><br><strong>Signature:</strong><br><code>function(event: object, value: number \\| number[]) =&gt; void</code><br><em>event:</em> The event source of the callback. <strong>Warning</strong>: This is a generic event not a change event.<br><em>value:</em> The new value.",
"orientation": "The component orientation.",
"scale": "A transformation function, to change the scale of the slider.",
Expand Down
Expand Up @@ -173,17 +173,19 @@ export interface SliderUnstyledTypeMap<P = {}, D extends React.ElementType = 'sp
/**
* Callback function that is fired when the slider's value changed.
*
* @param {object} event The event source of the callback. **Warning**: This is a generic event not a change event.
* @param {object} event The event source of the callback.
* You can pull out the new value by accessing `event.target.value` (any).
* **Warning**: This is a generic event not a change event.
* @param {number | number[]} value The new value.
*/
onChange?: (event: React.SyntheticEvent, value: number | number[]) => void;
onChange?: (event: Event, value: number | number[]) => void;
/**
* Callback function that is fired when the `mouseup` is triggered.
*
* @param {object} event The event source of the callback. **Warning**: This is a generic event not a change event.
* @param {number | number[]} value The new value.
*/
onChangeCommitted?: (event: React.SyntheticEvent, value: number | number[]) => void;
onChangeCommitted?: (event: React.SyntheticEvent | Event, value: number | number[]) => void;
/**
* The component orientation.
* @default 'horizontal'
Expand Down
24 changes: 14 additions & 10 deletions packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js
Expand Up @@ -230,17 +230,19 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
const handleChange =
onChange &&
((event, value) => {
if (!(event instanceof Event)) event.persist();

// Redefine target to allow name and value to be read.
// This allows seamless integration with the most popular form libraries.
// https://github.com/mui-org/material-ui/issues/13485#issuecomment-676048492
Object.defineProperty(event, 'target', {
// Clone the event to not override `target` of the original event.
const nativeEvent = event.nativeEvent || event;
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);

Object.defineProperty(clonedEvent, 'target', {
writable: true,
value: { value, name },
});

onChange(event, value);
onChange(clonedEvent, value);
});

const range = Array.isArray(valueDerived);
Expand Down Expand Up @@ -459,25 +461,25 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
stopListening();
});

const handleTouchStart = useEventCallback((event) => {
const handleTouchStart = useEventCallback((nativeEvent) => {
// If touch-action: none; is not supported we need to prevent the scroll manually.
if (!doesSupportTouchActionNone()) {
event.preventDefault();
nativeEvent.preventDefault();
}

const touch = event.changedTouches[0];
const touch = nativeEvent.changedTouches[0];
if (touch != null) {
// A number that uniquely identifies the current finger in the touch session.
touchId.current = touch.identifier;
}
const finger = trackFinger(event, touchId);
const finger = trackFinger(nativeEvent, touchId);
const { newValue, activeIndex } = getFingerNewValue({ finger, values, source: valueDerived });
focusThumb({ sliderRef, activeIndex, setActive });

setValueState(newValue);

if (handleChange) {
handleChange(event, newValue);
handleChange(nativeEvent, newValue);
}

const doc = ownerDocument(sliderRef.current);
Expand Down Expand Up @@ -892,7 +894,9 @@ SliderUnstyled.propTypes = {
/**
* Callback function that is fired when the slider's value changed.
*
* @param {object} event The event source of the callback. **Warning**: This is a generic event not a change event.
* @param {object} event The event source of the callback.
* You can pull out the new value by accessing `event.target.value` (any).
* **Warning**: This is a generic event not a change event.
* @param {number | number[]} value The new value.
*/
onChange: PropTypes.func,
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Select/Select.d.ts
Expand Up @@ -113,6 +113,7 @@ export interface SelectProps<T = unknown>
*
* @param {object} event The event source of the callback.
* You can pull out the new value by accessing `event.target.value` (any).
* **Warning**: This is a generic event not a change event.
* @param {object} [child] The react element that was selected when `native` is `false` (default).
*/
onChange?: SelectInputProps<T>['onChange'];
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Select/Select.js
Expand Up @@ -184,6 +184,7 @@ Select.propTypes = {
*
* @param {object} event The event source of the callback.
* You can pull out the new value by accessing `event.target.value` (any).
* **Warning**: This is a generic event not a change event.
* @param {object} [child] The react element that was selected when `native` is `false` (default).
*/
onChange: PropTypes.func,
Expand Down
19 changes: 19 additions & 0 deletions packages/material-ui/src/Select/Select.test.js
Expand Up @@ -1112,4 +1112,23 @@ describe('<Select />', () => {
);
expect(document.activeElement).to.equal(getByRole('button'));
});

it('should not override the event.target on click events', () => {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
const handleChange = spy();
const handleEvent = spy((event) => event.target);
render(
<div onClick={handleEvent}>
<Select open onChange={handleChange} value="second">
<MenuItem value="first" />
<MenuItem value="second" />
</Select>
</div>,
);

const options = screen.getAllByRole('option');
options[0].click();

expect(handleChange.callCount).to.equal(1);
expect(handleEvent.returnValues).to.have.members([options[0]]);
});
});
2 changes: 1 addition & 1 deletion packages/material-ui/src/Select/SelectInput.d.ts
Expand Up @@ -15,7 +15,7 @@ export interface SelectInputProps<T = unknown> {
native: boolean;
onBlur?: React.FocusEventHandler<any>;
onChange?: (
event: React.ChangeEvent<{ name?: string; value: T }>,
event: React.ChangeEvent<{ name?: string; value: T; event: Event | React.SyntheticEvent }>,
child: React.ReactNode
) => void;
onClose?: (event: React.SyntheticEvent) => void;
Expand Down
13 changes: 9 additions & 4 deletions packages/material-ui/src/Select/SelectInput.js
Expand Up @@ -189,13 +189,18 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
setValueState(newValue);

if (onChange) {
event.persist();
// Preact support, target is read only property on a native event.
Object.defineProperty(event, 'target', {
// Redefine target to allow name and value to be read.
// This allows seamless integration with the most popular form libraries.
// https://github.com/mui-org/material-ui/issues/13485#issuecomment-676048492
// Clone the event to not override `target` of the original event.
const nativeEvent = event.nativeEvent || event;
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);

Object.defineProperty(clonedEvent, 'target', {
writable: true,
value: { value: newValue, name },
});
onChange(event, child);
onChange(clonedEvent, child);
}
}

Expand Down
4 changes: 3 additions & 1 deletion packages/material-ui/src/Slider/Slider.js
Expand Up @@ -548,7 +548,9 @@ Slider.propTypes = {
/**
* Callback function that is fired when the slider's value changed.
*
* @param {object} event The event source of the callback. **Warning**: This is a generic event not a change event.
* @param {object} event The event source of the callback.
* You can pull out the new value by accessing `event.target.value` (any).
* **Warning**: This is a generic event not a change event.
* @param {number | number[]} value The new value.
*/
onChange: PropTypes.func,
Expand Down
5 changes: 3 additions & 2 deletions packages/material-ui/src/Slider/Slider.spec.tsx
Expand Up @@ -2,8 +2,9 @@ import * as React from 'react';
import Slider from '@material-ui/core/Slider';

function testOnChange() {
function handleSliderChange(event: React.SyntheticEvent, tabsValue: unknown) {}
<Slider onChange={handleSliderChange} onChangeCommitted={handleSliderChange} />;
function handleSliderChange(event: Event, value: unknown) {}
function handleSliderChangeCommitted(event: React.SyntheticEvent | Event, value: unknown) {}
<Slider onChange={handleSliderChange} onChangeCommitted={handleSliderChangeCommitted} />;

function handleElementChange(event: React.ChangeEvent) {}
// @ts-expect-error internally it's whatever even lead to a change in value
Expand Down
79 changes: 71 additions & 8 deletions packages/material-ui/src/Slider/Slider.test.js
Expand Up @@ -2,11 +2,17 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import { spy, stub } from 'sinon';
import { expect } from 'chai';
import { createMount, describeConformanceV5, act, createClientRender, fireEvent } from 'test/utils';
import {
createMount,
describeConformanceV5,
act,
createClientRender,
fireEvent,
screen,
} from 'test/utils';
import { ThemeProvider, createMuiTheme } from '@material-ui/core/styles';
import { SliderUnstyled } from '@material-ui/unstyled';
import clsx from 'clsx';
import Slider, { sliderClasses as classes } from './Slider';
import Slider, { sliderClasses as classes } from '@material-ui/core/Slider';

function createTouches(touches) {
return {
Expand Down Expand Up @@ -103,7 +109,7 @@ describe('<Slider />', () => {
expect(handleChangeCommitted.callCount).to.equal(1);
});

it('should edge against a dropped mouseup event', () => {
it('should hedge against a dropped mouseup event', () => {
const handleChange = spy();
const { container } = render(<Slider onChange={handleChange} value={0} />);
stub(container.firstChild, 'getBoundingClientRect').callsFake(() => ({
Expand Down Expand Up @@ -702,14 +708,14 @@ describe('<Slider />', () => {
function ValueLabelComponent(props) {
const { value, open } = props;
return (
<span data-testid="value-label" className={clsx({ open })}>
<span data-testid="value-label" className={open ? 'open' : ''}>
{value}
</span>
);
}
ValueLabelComponent.propTypes = { value: PropTypes.number };

const screen = render(
const { setProps } = render(
<Slider
components={{ ValueLabel: ValueLabelComponent }}
valueLabelDisplay="on"
Expand All @@ -719,7 +725,7 @@ describe('<Slider />', () => {

expect(screen.queryByTestId('value-label')).to.have.class('open');

screen.setProps({
setProps({
valueLabelDisplay: 'off',
});

Expand Down Expand Up @@ -944,7 +950,8 @@ describe('<Slider />', () => {
});

expect(handleChange.callCount).to.equal(1);
expect(handleChange.firstCall.returnValue).to.deep.equal({
const target = handleChange.firstCall.returnValue;
expect(target).to.deep.equal({
name: 'change-testing',
value: 4,
});
Expand All @@ -970,4 +977,60 @@ describe('<Slider />', () => {
expect(getByTestId('value-label')).to.have.text('1010');
});
});

it('should not override the event.target on touch events', () => {
const handleChange = spy();
const handleNativeEvent = spy((event) => event.target);
const handleEvent = spy((event) => event.target);
function Test() {
React.useEffect(() => {
document.addEventListener('touchstart', handleNativeEvent);
return () => {
document.removeEventListener('touchstart', handleNativeEvent);
};
});

return (
<div onTouchStart={handleEvent}>
<Slider data-testid="slider" value={0} onChange={handleChange} />
</div>
);
}
render(<Test />);
const slider = screen.getByTestId('slider');

fireEvent.touchStart(slider, createTouches([{ identifier: 1 }]));

expect(handleChange.callCount).to.equal(1);
expect(handleNativeEvent.returnValues).to.have.members([slider]);
expect(handleEvent.returnValues).to.have.members([slider]);
});

it('should not override the event.target on mouse events', () => {
const handleChange = spy();
const handleNativeEvent = spy((event) => event.target);
const handleEvent = spy((event) => event.target);
function Test() {
React.useEffect(() => {
document.addEventListener('mousedown', handleNativeEvent);
return () => {
document.removeEventListener('mousedown', handleNativeEvent);
};
});

return (
<div onMouseDown={handleEvent}>
<Slider data-testid="slider" value={0} onChange={handleChange} />
</div>
);
}
render(<Test />);
const slider = screen.getByTestId('slider');

fireEvent.mouseDown(slider);

expect(handleChange.callCount).to.equal(1);
expect(handleNativeEvent.returnValues).to.have.members([slider]);
expect(handleEvent.returnValues).to.have.members([slider]);
});
});