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

[TextField] Fix onClick regressions handling changes #38474

Merged
merged 7 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/mui-material/src/InputBase/InputBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,8 @@ const InputBase = React.forwardRef(function InputBase(inProps, ref) {
if (inputRef.current && event.currentTarget === event.target) {
inputRef.current.focus();
}
if (onClick && !fcs.disabled) {

if (onClick) {
onClick(event);
}
};
Expand Down
8 changes: 8 additions & 0 deletions packages/mui-material/src/InputBase/InputBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,14 @@ describe('<InputBase />', () => {
});
expect(handleFocus.called).to.equal(false);
});

it('fires the click event when the <input /> is disabled', () => {
const handleClick = spy();
const { getByRole } = render(<InputBase disabled onClick={handleClick} />);
const input = getByRole('textbox');
fireEvent.click(input);
expect(handleClick.callCount).to.equal(1);
});
});

describe('prop: readonly', () => {
Expand Down
4 changes: 0 additions & 4 deletions packages/mui-material/src/TextField/TextField.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,6 @@ export interface BaseTextFieldProps
name?: string;
onBlur?: InputBaseProps['onBlur'];
onFocus?: StandardInputProps['onFocus'];
/**
* @ignore
*/
onClick?: InputBaseProps['onClick'];
/**
* The short hint displayed in the `input` before the user enters a value.
*/
Expand Down
16 changes: 0 additions & 16 deletions packages/mui-material/src/TextField/TextField.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ const TextField = React.forwardRef(function TextField(inProps, ref) {
name,
onBlur,
onChange,
onClick,
onFocus,
placeholder,
required = false,
Expand Down Expand Up @@ -147,15 +146,6 @@ const TextField = React.forwardRef(function TextField(inProps, ref) {
InputMore['aria-describedby'] = undefined;
}

const handleClick = (event) => {
if (!disabled && onClick) {
// The `onClick` is registered both on the root and the input elements.
// Without stopping the propagation, the event could be triggered twice.
event.stopPropagation();
onClick(event);
}
};

const id = useId(idOverride);
const helperTextId = helperText && id ? `${id}-helper-text` : undefined;
const inputLabelId = label && id ? `${id}-label` : undefined;
Expand All @@ -179,7 +169,6 @@ const TextField = React.forwardRef(function TextField(inProps, ref) {
onBlur={onBlur}
onChange={onChange}
onFocus={onFocus}
onClick={handleClick}
placeholder={placeholder}
inputProps={inputProps}
{...InputMore}
Expand All @@ -198,7 +187,6 @@ const TextField = React.forwardRef(function TextField(inProps, ref) {
color={color}
variant={variant}
ownerState={ownerState}
onClick={handleClick}
{...other}
>
{label != null && label !== '' && (
Expand Down Expand Up @@ -358,10 +346,6 @@ TextField.propTypes /* remove-proptypes */ = {
* You can pull out the new value by accessing `event.target.value` (string).
*/
onChange: PropTypes.func,
/**
* @ignore
*/
onClick: PropTypes.func,
/**
* @ignore
*/
Expand Down
40 changes: 17 additions & 23 deletions packages/mui-material/src/TextField/TextField.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,22 +155,6 @@ describe('<TextField />', () => {
});
});

describe('prop: disabled', () => {
it('should not run click event when disabled', () => {
const handleClick = spy();
const { getByRole } = render(<TextField disabled onClick={handleClick} />);
fireEvent.click(getByRole('textbox'));
expect(handleClick.callCount).to.equal(0);
});

it('should not run click event when disabled and when onClick prop is set through InputProps', () => {
const handleClick = spy();
const { getByRole } = render(<TextField disabled InputProps={{ onClick: handleClick }} />);
fireEvent.click(getByRole('textbox'));
expect(handleClick.callCount).to.equal(0);
});
});

describe('prop: select', () => {
it('can render a <select /> when `native`', () => {
const currencies = [
Expand Down Expand Up @@ -244,12 +228,22 @@ describe('<TextField />', () => {
});
});

it('should trigger `onClick` only once', () => {
const handleClick = spy();
const { getByRole } = render(
<TextField variant="outlined" label="Test" onClick={handleClick} />,
);
fireEvent.click(getByRole('textbox'));
expect(handleClick.callCount).to.equal(1);
describe('event: click', () => {
it('registers `onClick` on the root slot', () => {
const handleClick = spy((event) => event.currentTarget);
const { getByTestId, getByRole } = render(
<TextField data-testid="root" onClick={handleClick} />,
);

const input = getByRole('textbox');

const root = getByTestId('root');

fireEvent.click(input);

expect(handleClick.callCount).to.equal(1);
// return value is event.currentTarget
expect(handleClick.returned(root));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't work, it doesn't fail without the fix.

This would be correct:

Suggested change
expect(handleClick.returned(root));
expect(handleClick.returned(root)).to.equal(true);

Can we fix it? Thanks

Actually, maybe the assertion itself would be better like this:

Suggested change
expect(handleClick.returned(root));
expect(handleClick.returnValues[0]).to.equal(root);

to give a clearer error message, but so far, we have an issue with our mocha DOM node comparator, it fails to log the diff correctly (another bug)

});
});
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import TextField from '@mui/material/TextField';

export default function TextFieldWithOnClick() {
export default function OutlinedTextFieldOnClick() {
const [isClicked, setIsClicked] = React.useState(false);
return (
<TextField
Expand Down
7 changes: 4 additions & 3 deletions test/e2e/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,13 @@ describe('e2e', () => {
});

describe('<TextField />', () => {
it('should handle `onClick` when clicking on the focused label position', async () => {
await renderFixture('TextField/TextFieldWithOnClick');
it('should fire `onClick` when clicking on the focused label position', async () => {
await renderFixture('TextField/OutlinedTextFieldOnClick');

// execute the click on the focused label position
await page.getByRole('textbox').click({ position: { x: 10, y: 10 } });
await page.waitForSelector('.MuiInputBase-root.Mui-error');
const errorSelector = page.locator('.MuiInputBase-root.Mui-error');
await errorSelector.waitFor();
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
});
});
});