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

[ButtonBase] Ensure that onClick propagates when non-native button is clicked #30145

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kmurgic
Copy link
Contributor

@kmurgic kmurgic commented Dec 10, 2021

This should solve #30144. I don't believe there are any unintended negative consequences from programmatically clicking the button, but someone please double check me on that. Although, one concern could be this being a breaking change for someone who is checking to see if the button onClick function was triggered by a keyboard event? I'm open to suggestions for different fixes to this bug if we need to make sure we are still passing the keyboard event to the onClick handler.

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 10, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 6268ece

@kmurgic
Copy link
Contributor Author

kmurgic commented Dec 10, 2021

Just noticed that I have some failing tests, fixing now.

@kmurgic
Copy link
Contributor Author

kmurgic commented Dec 10, 2021

Okay, fixed the tests. If we need to pass the original keyboard event to the onClick handler then there's still a bit more work to do, but waiting to look into that until I get some feedback!

@michaldudak
Copy link
Member

Hi @kmurgic! Thanks for your PR. The change you introduced makes a lot of sense as it improves consistency between using the native button and a custom element.
However, as you noticed, this is a breaking change. Not only we're changing the type of the fired event, but also the defaultPrevented is different. We could include this change in the next major version, but I don't think it would be safe to do so now.

@kmurgic
Copy link
Contributor Author

kmurgic commented Dec 13, 2021

I spent a little time digging and I couldn't find another good way to trigger the event propagation. The best I could do was to fire the event on the parent element, but then the event has the wrong target element. I'm throwing in the towel on doing this without breaking changes, but someone else may want to try. If we can't fix this bug then maybe MUI should document somewhere the known issue with keyboard accessibility when using an onClick on the Menu component.

@kmurgic kmurgic closed this Dec 13, 2021
@kmurgic kmurgic reopened this Dec 13, 2021
@michaldudak michaldudak added on hold There is a blocker, we need to wait component: ButtonBase The React component. labels Dec 14, 2021
@michaldudak michaldudak added this to the v6 milestone Dec 14, 2021
@michaldudak michaldudak changed the title Ensure that onClick propagates when non-native button is clicked [ButtonBase] Ensure that onClick propagates when non-native button is clicked Dec 14, 2021
@michaldudak
Copy link
Member

Let's keep this PR open. We'll get back to it once we're closer to v6.

@kmurgic
Copy link
Contributor Author

kmurgic commented Jan 6, 2025

@michaldudak should we close this now that v6 is released?

@michaldudak
Copy link
Member

@DiegoAndai - do you know what's the status of this?

@mnajdova
Copy link
Member

I think we can include this fix in v7. I am changing the branch where we want to fix this (to master). cc @DiegoAndai

@mnajdova mnajdova changed the base branch from v5.x to master February 13, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: ButtonBase The React component. on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants