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.

Sign up for GitHub

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

Jump to bottom

fix(material/core): ripple coords when trigger is not the container #29020

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −3
Open

fix(material/core): ripple coords when trigger is not the container #29020

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/material/core/ripple/ripple-renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ export class RippleRenderer implements EventListenerObject {
*/
private _containerRect: DOMRect | null;

/**
* Cached dimensions of the trigger element. Set only if the trigger
* element is different than the container element when the trigger
* is being first pressed and cleared once no more ripples are visible.
*/
private _triggerRect!: DOMRect | null;

private static _eventManager = new RippleEventManager();

constructor(
Expand Down Expand Up @@ -337,9 +344,10 @@ export class RippleRenderer implements EventListenerObject {
const eventListeners = this._activeRipples.get(rippleRef) ?? null;
this._activeRipples.delete(rippleRef);

// Clear out the cached bounding rect if we have no more ripples.
// Clear out the cached bounding rects if we have no more ripples.
if (!this._activeRipples.size) {
this._containerRect = null;
this._triggerRect = null;
}

// If the current ref is the most recent transient ripple, unset it
Expand Down Expand Up @@ -367,7 +375,8 @@ export class RippleRenderer implements EventListenerObject {

if (!this._target.rippleDisabled && !isFakeMousedown && !isSyntheticEvent) {
this._isPointerDown = true;
this.fadeInRipple(event.clientX, event.clientY, this._target.rippleConfig);
const pointerCoords = this._fixCoordinatesEmittedFromTrigger(event.clientX, event.clientY);
this.fadeInRipple(pointerCoords[0], pointerCoords[1], this._target.rippleConfig);
}
}

Expand All @@ -388,7 +397,11 @@ export class RippleRenderer implements EventListenerObject {
// the browser appears to not assign them in tests which leads to flakes.
if (touches) {
for (let i = 0; i < touches.length; i++) {
this.fadeInRipple(touches[i].clientX, touches[i].clientY, this._target.rippleConfig);
const pointerCoords = this._fixCoordinatesEmittedFromTrigger(
touches[i].clientX,
touches[i].clientY,
);
this.fadeInRipple(pointerCoords[0], pointerCoords[1], this._target.rippleConfig);
}
}
}
Expand Down Expand Up @@ -438,6 +451,19 @@ export class RippleRenderer implements EventListenerObject {
}
}
}

/** Fix coordinates emitted from the trigger element when is not the container element. */
private _fixCoordinatesEmittedFromTrigger(x: number, y: number): [number, number] {
if (this._triggerElement && this._triggerElement !== this._containerElement) {
const containerRect = (this._containerRect =
this._containerRect || this._containerElement.getBoundingClientRect());
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;
Copy link
Member

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?

Copy link
Contributor Author

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:

image

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.

Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

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

Copy link
Contributor Author

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 why the 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.

}
return [x, y];
}
}

/**
Expand Down