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

Stepper: replace onMousedown by onClick for consistency #1448

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nicooprat
Copy link
Contributor

@nicooprat nicooprat commented Nov 22, 2024

Using mousedown event can lead to some edge cases.

In our app, changing step changes the modal content, thus its height, and a mouseup event would then be fired outside the modal: so changing step was actually closing the modal... it was hard to find why!

Also, this would be better for accessibility because I think using enter actually trigger the click event natively.

I can't think of any reason why using mousedown would be better, but I might be wrong!

Related: #1330

@sadeghbarati sadeghbarati requested a review from epr3 November 23, 2024 05:27
Copy link
Collaborator

@epr3 epr3 left a comment

Choose a reason for hiding this comment

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

Hello @nicooprat,

Thank you for the PR. I think we're using the mousedown handler to have faster responses from the UI and the ability to handle events earlier in the interaction chain.

Now that I think of it, I guess pointerdown would have been better here for the same reason outlined above plus compatibility with various other pointing devices (touchscreen, stylus, etc.).

Could you try pointerdown and see if it works for your use case?

Otherwise, from my side click could work too.
@zernonia would click work here instead of mousedown/pointerdown?

@epr3 epr3 requested a review from zernonia November 28, 2024 16:11
@nicooprat
Copy link
Contributor Author

Hi, thanks for your answer.

I guess pointerdown would be a more modern way of doing it indeed, but it still causes the issue described (just tested).

@nicooprat
Copy link
Contributor Author

PS. I'm currently attending an accessibility training and it's explicitely explained that no action must be registered on "down" events (mousedown, pointerdown, keydown, touchstart) unless specifically needed, because some people might misclick and could need the escape way of dragging out before releasing the click.

@zernonia
Copy link
Member

zernonia commented Dec 5, 2024

Yoo @nicooprat ! The Stepper implementation was heavily ported from the Tab component, which has a similar action on Trigger component (specifically mousedown).

I'm not 100% sure regarding the reason but this is what the other major a11y focus library were doing (namely Radix, React Spectrum)

@nicooprat
Copy link
Contributor Author

Thanks, I answered in a related discussion in Radix repo: radix-ui/themes#301 (reply in thread)

@zernonia
Copy link
Member

zernonia commented Dec 5, 2024

Awesome @nicooprat ! I will set this to on-hold till we have further insight ya 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants