[ButtonBase] Fix native button detection#47985
Conversation
Netlify deploy previewhttps://deploy-preview-47985--material-ui.netlify.app/ Bundle size report
|
mj12albert
force-pushed
the
fix/button-base
branch
from
bf76c78 to
c73eb83
Compare
| const handleClick = useEventCallback((event) => { | ||
| if (disabled) { | ||
| event.preventDefault(); | ||
| return; | ||
| } | ||
|
|
||
| if (onClick) { | ||
| onClick(event); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
I think there is a risk to intercept the click event here given that the ButtonBase is used by a lot of components. What's the issue it fixes? or is this change align with Base UI?
There was a problem hiding this comment.
This possibly relates more to #47989 than this PR
Currently when disabled we are relying on CSS pointer-events none to "disable" the click. However I want to extract these event handlers to a hook so that new ChipButton/ChipDelete can reuse the logic, but may not necessarily want to set pointer-events: none on them @siriwatknp
mj12albert
force-pushed
the
fix/button-base
branch
from
34d1724 to
40f02b8
Compare
mj12albert
changed the title
|
|
||
| const button = screen.getByRole('button'); | ||
|
|
||
| await act(async () => { |
There was a problem hiding this comment.
Would user.tab() work here instead?
There was a problem hiding this comment.
This is kind of an edge case to begin with since it's a disabled non-native button, because it's disabled a Tab wouldn't land focus there to begin with, only programmatic focus is possible (added a comment in the test)
| button.focus(); | ||
| }); | ||
|
|
||
| fireEvent.keyUp(button, { key: ' ' }); |
There was a problem hiding this comment.
having just a keyUp is definitely not what is going to happen in production, even though the test logic is sound. would it work if we use user.keyboard? Again, the test makes sense, it's fine, I'm just asking if we can mimic the user behavior better here.
There was a problem hiding this comment.
👍 changed to user.keyboard(' ')
silviuaavram
added
needs cherry-pick
|
Cherry-pick PRs will be created targeting branches: v7.x |
5 participants
The "is native button" check only checked
component !== 'button'. Whencomponent={StyledButton}where<StyledButton>renders a<button>element, event handlers incorrectly apply the logic for non-native buttons