-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(ripple): don't launch ripple for fake mouse events #11997
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
Conversation
devversion
left a comment
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.
LGTM.
| private onMousedown = (event: MouseEvent) => { | ||
| // Screen readers will fire fake mouse events for space/enter. Skip launching a | ||
| // ripple in this case for consistency with the non-screen-reader experience. | ||
| const isFakeMousedown = event.buttons === 0; |
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.
We should use isFakeMousedownFromScreenReader from @angular/cdk/a11y?
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.
I wanted to avoid pulling in cdk/a11y since the idea is to pull the ripples out into their own module.
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.
I see. I don't really think though, that this means that we should hesitate using the CDK. Even if the ripples are moved into its own module, they can still have dependencies on the CDK.
The goal is not make ripples independent from anything else. It should be just possible for developers to explicitly use @angular/material/ripple without having all the other stuff.
e.g. see @angular/material/core does have deps on the CDK, and also the ripples already use cdk/platform.
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.
What I meant is that it seemed like overkill to bring in another CDK package for a one-liner method. I've switched it now.
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.
Yeah, totally see what you mean. My reasoning is just that the CDK is being pulled by NPM anyway and with tree shaking (which is quite common used) it doesn't really matter how big the a11y package is because only the one function will be pulled in.
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.
Not sure if I can make that decision though. Let's wait what Jeremy says on that. I'm a huge fan of consistency, and I'm worried that we'll duplicate logic which can be easily forgotten.
d57f85d to
06b7a36
Compare
jelbourn
left a comment
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.
LGTM
|
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
When using a screen reader the ripples can get fake mouse events when pressing space or enter. With the following changes we skip launching the ripples in these cases, in order to align the experience with the non-screen-reader.
06b7a36 to
e0bf841
Compare
|
Hi @crisbeto & @jelbourn, |
When using a screen reader the ripples can get fake mouse events when pressing space or enter. With the following changes we skip launching the ripples in these cases, in order to align the experience with the non-screen-reader.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When using a screen reader the ripples can get fake mouse events when pressing space or enter. With the following changes we skip launching the ripples in these cases, in order to align the experience with the non-screen-reader.
For reference:
