[TextField] Fix onClick regressions handling changes #38474
Conversation
mj12albert
added
the
component: text field
mj12albert
changed the title
Netlify deploy previewhttps://deploy-preview-38474--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
|
We could maybe add test cases, to cover the 2-3 regressions we saw (maybe 1. the one Lukas had, 2. the click bubble issue on TextField 3. the click bubble issue on disabled InputBase). |
oliviertassinari
changed the title
mj12albert
force-pushed
the
revert-textfield-disabled
branch
3 times, most recently
from
68fd1fd
to
009debe
Compare
There was a problem hiding this comment.
For what its worth, LGTM. The revert fixes the relevant issue: mui/mui-x#9386 on X side. 🙌
mj12albert
force-pushed
the
revert-textfield-disabled
branch
from
2faaf69
to
3ceae68
Compare
| it('registers `onClick` on the root slot', () => { | ||
| const handleClick = spy(); | ||
| const { getByTestId } = render( | ||
| <TextField | ||
| data-testid="root" | ||
| onClick={handleClick} | ||
| inputProps={{ | ||
| onClick: handleClick, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| const root = getByTestId('root'); | ||
|
|
||
| fireEvent.click(root); | ||
| expect(handleClick.callCount).to.equal(1); | ||
| }); |
There was a problem hiding this comment.
This strange looks strange to me. I think the way we test registers onClick on the root slot is by firing the click on the textbox (<input>), adding onClick on the TextField, and checking that event.currentTarget is the root element.
There was a problem hiding this comment.
@oliviertassinari Thanks for the suggestion ~ I've changed the test to check that instead
(Also removed the "fires only once" test, it's redundant)
mj12albert
force-pushed
the
revert-textfield-disabled
branch
from
3ceae68
to
2b2c35b
Compare
|
|
||
| expect(handleClick.callCount).to.equal(1); | ||
| // return value is event.currentTarget | ||
| expect(handleClick.returned(root)); |
There was a problem hiding this comment.
This test doesn't work, it doesn't fail without the fix.
This would be correct:
| 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:
| 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)
Follows up #36892 (comment)
Reverts #38072 and #36892