-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(material/core): ripple coords when trigger is not the container #29020
base: main
Are you sure you want to change the base?
fix(material/core): ripple coords when trigger is not the container #29020
Conversation
783d885
to
cd59496
Compare
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.
Thx for the PR (sorry for the delayed review— I'm not working on Material but got assigned)
const triggerRect = (this._triggerRect = | ||
this._triggerRect || this._triggerElement.getBoundingClientRect()); | ||
x = ((x - triggerRect.left) / triggerRect.width) * containerRect.width + containerRect.left; | ||
y = ((y - triggerRect.top) / triggerRect.height) * containerRect.height + containerRect.top; |
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 assumes that the dimensions of the trigger are translatable to the container right? I wonder if we would rather want to prevent the case where the trigger is outside of the container? What do you think?
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 assumes that the dimensions of the trigger are translatable to the container right?
That's right. And I don't see any case where the dimensions of the trigger aren't translatable to the container because here we do simple proportion calculations (except when the container height or width equal to 0 but this case is already covered).
I wonder if we would rather want to prevent the case where the trigger is outside of the container?
No, I don't want to prevent the case where the trigger is outside of the container. I want to translate a position clicked in trigger element into the ripple container.
In this stackblitz to reproduce the current behavior, I apply [matRippleRadius]="10"
on the ripple container. But if you don't apply a radius, you can see that the ripple element coords is outside the ripple container (I clicked where there is the letter "X" in red) and create a huge radius:
In this example, the experience is not too shocking because the trigger is near to the ripple container. But in case where the trigger is far from the ripple container: the ripple element rendering is strange (when we think about user experience). It's a better user experience when the ripple element coords is inside the ripple container in coords translated from the original coords (where the user clicked on the trigger).
Note: indeed, this translation has no effect when the ripple element is centered.
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.
That's right. And I don't see any case where the dimensions of the trigger aren't translatable to the container because here we do simple proportion calculation
Right, but does it make sense to do a simple proportion calculation? what's the use-case? I.e. in most cases the ripple should originate where the click happens, and different proportions wouldn't necessarily make sense then, right?
No, I don't want to prevent the case where the trigger is outside of the container. I want to translate a position clicked in trigger element into the ripple container.
What's the use-case though? Why wouldn't the trigger not be inside the container? or a sibling that is "positioned over the ripple container"?
In this stackblitz to reproduce the current behavior, I apply [matRippleRadius]="10" on the ripple container. But if you don't apply a radius, you can see that the ripple element coords is outside the ripple container (I clicked where there is the letter "X" in red) and create a huge radius:
Yeah, this is a bug. Good catch, and one solution would be to disallow the trigger to be outside of the boundaries of the container I think.
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, this is a bug. Good catch, and one solution would be to disallow the trigger to be outside of the boundaries of the container I think.
I don't think that is a bug. As we can read in ripple.md: it's a feature "to show ripples on interaction with some other element" that the container.
Right, but does it make sense to do a simple proportion calculation? what's the use-case?
Check yourself: I made another stackblitz and compare the user experience with the both triggers (I think that a demonstration can be more explicit than words).
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 don't think that is a bug. As we can read in ripple.md: it's a feature "to show ripples on interaction with some other element" that the container.
yeah, I actually created the implementation of ripple a couple of years ago. The intent was to support triggers of different elements, but not necessarily disconnected from the container visually (but to support the trigger overlaying the ripple elements, to e.g. not prevent further clicks).
We can limit this feature, or clarify the intent to address this problem. It's just an option we can consider. We can certainly also fix this if it makes sense to do so.
Check yourself: I made another stackblitz and compare the user experience with the both triggers (I think that a demonstration can be more explicit than words).
I follow what you are trying to achieve, but what is the real-world use-case here? Why would you want to display the ripple in a totally different element? Ripples from the Material specification generally don't seem to mention such a scenario: https://m2.material.io/develop/ios/supporting/ripple
Also cc. @crisbeto / @andrewseguin
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 follow what you are trying to achieve, but what is the real-world use-case here? Why would you want to display the ripple in a totally different element?
It's subjective. The first time I use a trigger element different than the ripple container element (where the trigger element is far from the ripple container element): I don't understand why the ripple effect seems to be strange (the ripple effect was too different than the case when the ripple container element is also the trigger element). I needed to investigate to understand what causes this difference.
After that, I wrote a code to adapt the ripple position like in this stackblitz. And later I made this pull request.
I would have been curious to have other opinions than ours. But if you want we can close this topic.
This pull request is a proposal to fix coordinates emitted from the trigger element when it's different than the container element.
Actual behavior
When a pointer down event (
mousedown: MouseEvent
ortouchstart: TouchEvent
) is emitted from the trigger element, itsclientX
andclientY
properties are used to callRippleRender::fadeInRipple
.In case where the trigger element is the ripple container element,
clientX
andclientY
are inside the container dom rect.But when the trigger element is different than the ripple container element (a trigger element can be set by using
matRippleTrigger
input),clientX
andclientY
are outside the container dom rect.Expeded behavior
In case where the trigger element is different than the ripple container element, using
clientX
andclientY
properties and calculating an equivalent point in the container element before callingRippleRender::fadeInRipple
.Reproduction
StackBlitz link: https://stackblitz.com/edit/components-issue-gtvxae
<div matRipple matRippleRadius="10" [matRippleTrigger]="trigger"></div>
<button #trigger>Trigger Button</button>
clientX
andclientY
properties are fixed and the ripple element (green dot in screenshot below) is positionned correctly inside the ripple container dom rect