-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
[ButtonBase] Fix native button detection #47985
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
Changes from 5 commits
55a7d08
1ddea13
9bfedcd
7cac119
40f02b8
02ef8d6
9fb1d35
2bc0a4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -824,6 +824,30 @@ describe('<ButtonBase />', () => { | |
| setProps({ disabled: false }); | ||
| expect(button).not.to.have.attribute('aria-disabled'); | ||
| }); | ||
|
|
||
| it('should not propagate click events when Space is released on a disabled non-native button', async () => { | ||
| const parentClickSpy = spy(); | ||
| const buttonClickSpy = spy(); | ||
|
|
||
| render( | ||
| <div onClick={parentClickSpy}> | ||
| <ButtonBase component="span" disabled onClick={buttonClickSpy}> | ||
| Hello | ||
| </ButtonBase> | ||
| </div>, | ||
| ); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
|
|
||
| await act(async () => { | ||
| button.focus(); | ||
| }); | ||
|
|
||
| fireEvent.keyUp(button, { key: ' ' }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 changed to |
||
|
|
||
| expect(buttonClickSpy.callCount).to.equal(0); | ||
| expect(parentClickSpy.callCount).to.equal(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe('prop: component', () => { | ||
|
|
@@ -1182,6 +1206,37 @@ describe('<ButtonBase />', () => { | |
| expect(onClickSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
| it('should preserve native button keyboard behavior when a custom component renders a native button', async () => { | ||
| const onClickSpy = spy(); | ||
| const onKeyDownSpy = spy(); | ||
|
|
||
| /** @type {React.ForwardRefExoticComponent<React.ButtonHTMLAttributes<HTMLButtonElement>>} */ | ||
| const MyButton = React.forwardRef((props, ref) => <button ref={ref} {...props} />); | ||
|
|
||
| const { user } = render( | ||
| <ButtonBase component={MyButton} onClick={onClickSpy} onKeyDown={onKeyDownSpy}> | ||
| Hello | ||
| </ButtonBase>, | ||
| ); | ||
|
|
||
| await user.tab(); | ||
|
|
||
| await user.keyboard('{Enter}'); | ||
|
|
||
| expect(onKeyDownSpy.callCount).to.equal(1); | ||
| expect(onClickSpy.callCount).to.equal(1); | ||
| expect(onKeyDownSpy.firstCall.args[0]).to.have.property('defaultPrevented', false); | ||
|
|
||
| onClickSpy.resetHistory(); | ||
| onKeyDownSpy.resetHistory(); | ||
|
|
||
| await user.keyboard(' '); | ||
|
|
||
| expect(onKeyDownSpy.callCount).to.equal(1); | ||
| expect(onClickSpy.callCount).to.equal(1); | ||
| expect(onKeyDownSpy.firstCall.args[0]).to.have.property('defaultPrevented', false); | ||
| }); | ||
|
|
||
| it('prevents default on Enter with an anchor and empty href', async () => { | ||
| const onClickSpy = spy(); | ||
| const onKeyDownSpy = spy(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would user.tab() work here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)