feat(material/button): make button ripples lazy #26568

wagnermaciel · 2023-02-03T19:37:18Z

No description provided.

src/material/button/button-lazy-ripple-renderer.ts

wagnermaciel · 2023-03-28T19:09:49Z

Here is the design doc which describes this feature as well as the rationale for why it was written this way:
https://docs.google.com/document/d/13paaXem1sXg6462-bb05M8GHguvQcsXGC8FxDeK6w_Q/edit?usp=sharing

src/material/button/button-lazy-loader.ts

crisbeto · 2023-03-29T06:55:13Z

src/material/button/button-lazy-loader.ts

+   * Traverses the element and its parents (heading toward the document root)
+   * until it finds a mat-button that has not been initialized.
+   */
+  private _closest(element: Element): Element | null {


We can use the native Element.closest API here instead.

@mleibman could probably speak to this best but from what I remember in our last discussions, this is actually a performance improvement. I have added a link to their example at the bottom of our design doc

https://docs.google.com/document/d/13paaXem1sXg6462-bb05M8GHguvQcsXGC8FxDeK6w_Q/edit?usp=sharing

I think that's not exactly the same, because it's setting a symbol on the DOM node whereas here we have an attribute.

Yeah, I'm very skeptical that a userspace closest is faster than the browser native closest. I would need to see some compelling evidence, and then I would go complain to Chrome.

Sounds plausible - I could have just misremembered. I switched to using the native Element.closest and I can revisit this if it really is a performance bottleneck

src/material/button/button-lazy-loader.ts

src/material/button/button.ts

src/material/button/button-ripple.ts

src/material/button/button-base.ts

src/material/button/button-lazy-loader.ts

jelbourn · 2023-03-30T22:29:04Z

src/material/button/button-lazy-loader.ts

+   * Traverses the element and its parents (heading toward the document root)
+   * until it finds a mat-button that has not been initialized.
+   */
+  private _closest(element: Element): Element | null {


Yeah, I'm very skeptical that a userspace closest is faster than the browser native closest. I would need to see some compelling evidence, and then I would go complain to Chrome.

src/material/button/button-lazy-loader.ts

src/material/button/button.ts

jelbourn · 2023-03-30T22:35:06Z

src/material/button/button-ripple.ts

+  constructor(
+    _elementRef: ElementRef<HTMLElement>,
+    readonly ngZone: NgZone,
+    readonly platform: Platform,
+    @Optional() @Inject(MAT_RIPPLE_GLOBAL_OPTIONS) globalOptions?: RippleGlobalOptions,
+    @Optional() @Inject(ANIMATION_MODULE_TYPE) _animationMode?: string,
+    @Optional() @Inject(DOCUMENT) document?: Document,
+  ) {
+    super(_elementRef, ngZone, platform, globalOptions, _animationMode, document);
+    this._buttonEl = _elementRef.nativeElement;
+    this._buttonEl.classList.remove('mat-ripple');
+  }


Prefer the inject function

I was able to do this for document, but not any of the other symbols because they are referenced in the call to super and references to this are not allowed before super is called

src/material/button/icon-button.scss

jelbourn · 2023-03-31T15:17:06Z

@wagnermaciel it doesn't actually have to be a directive instance, it just has to have the same API shape, no?

rough pseudo-code for the idea

type PublicInterface<T> = {[K in keyof T]: T[K]};

class DeferredRipple implements PublicInterface<MatRipple> {
  // ripple stuff
}

class MatButton {
  get ripple(): MatRipple {
    if (!this._rippleInternal) this._rippleInternal = this.createRipple();
    return this._rippleInternal;
  }

  private _rippleInternal = null;
}

(it's possible I'm missing some detail, admittedly)

wagnermaciel · 2023-03-31T16:28:50Z

@jelbourn Previously, when I considered this approach I was against it because I didn't want to be creating & having to maintain a MatRipple-like object. This time around, I just tried creating a new MatRipple(...). I didn't think it'd work bc it's not designed to work that way, but I was able to get it working!

jelbourn

Overall looks good- the only main thing left is adding additional tests for MatButtonLazyLoader

src/material/button/button-base.ts

src/material/button/button-lazy-loader.ts

jelbourn

LGTM

src/material/button/button.spec.ts

isantone · 2023-04-21T06:30:22Z

Hi @wagnermaciel @jelbourn,
Could you please check this PR #26970 for fixing mat-fab and mat-mini-fab ripple effect?
Thanks!

angular-automatic-lock-bot · 2023-05-22T00:08:32Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

angular-robot bot added the detected: feature PR contains a feature commit label Feb 3, 2023

mmalerba reviewed Feb 3, 2023

View reviewed changes

src/material/button/button-lazy-ripple-renderer.ts Outdated Show resolved Hide resolved

wagnermaciel force-pushed the button-perf branch from 127faf5 to 89ee76f Compare February 24, 2023 01:43

wagnermaciel force-pushed the button-perf branch from 6f12cda to 6b2cbec Compare March 3, 2023 16:16

wagnermaciel force-pushed the button-perf branch from d6b4ec3 to 71841bc Compare March 14, 2023 00:57

wagnermaciel force-pushed the button-perf branch from 9b96733 to 3d72aca Compare March 28, 2023 18:47

wagnermaciel marked this pull request as ready for review March 28, 2023 19:03

wagnermaciel requested review from andrewseguin, devversion and josephperrott as code owners March 28, 2023 19:03

wagnermaciel requested review from crisbeto and removed request for devversion and josephperrott March 28, 2023 19:03

crisbeto reviewed Mar 29, 2023

View reviewed changes

wagnermaciel requested a review from crisbeto March 30, 2023 11:48

crisbeto approved these changes Mar 30, 2023

View reviewed changes

src/material/button/button-ripple.ts Outdated Show resolved Hide resolved

jelbourn reviewed Mar 30, 2023

View reviewed changes

wagnermaciel force-pushed the button-perf branch 2 times, most recently from 54ba026 to fc7f0a8 Compare March 31, 2023 03:59

wagnermaciel force-pushed the button-perf branch 2 times, most recently from b253a9a to 369e74e Compare March 31, 2023 19:56

wagnermaciel requested a review from jelbourn March 31, 2023 20:47

jelbourn reviewed Apr 3, 2023

View reviewed changes

wagnermaciel requested a review from jelbourn April 4, 2023 00:25

jelbourn approved these changes Apr 4, 2023

View reviewed changes

src/material/button/button.spec.ts Outdated Show resolved Hide resolved

wagnermaciel added target: minor This PR is targeted for the next minor release action: merge The PR is ready for merge by the caretaker labels Apr 4, 2023

wagnermaciel force-pushed the button-perf branch 2 times, most recently from 22df8c7 to 24757b4 Compare April 5, 2023 04:17

feat(material/button): make button ripples lazy

f4a22d4

wagnermaciel force-pushed the button-perf branch from 24757b4 to f4a22d4 Compare April 5, 2023 04:34

angular-robot bot merged commit d6d3e3e into angular:main Apr 5, 2023
19 of 22 checks passed

isantone mentioned this pull request Apr 21, 2023

fix(material/button): fix ripple style for all types of buttons #26970

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(material/button): make button ripples lazy #26568

feat(material/button): make button ripples lazy #26568

wagnermaciel commented Feb 3, 2023

wagnermaciel commented Mar 28, 2023

crisbeto Mar 29, 2023

wagnermaciel Mar 29, 2023

crisbeto Mar 30, 2023

jelbourn Mar 30, 2023

wagnermaciel Mar 31, 2023

jelbourn Mar 30, 2023

jelbourn Mar 30, 2023

wagnermaciel Mar 31, 2023

jelbourn commented Mar 31, 2023

wagnermaciel commented Mar 31, 2023

jelbourn left a comment

jelbourn left a comment

isantone commented Apr 21, 2023

angular-automatic-lock-bot bot commented May 22, 2023

feat(material/button): make button ripples lazy #26568

feat(material/button): make button ripples lazy #26568

Conversation

wagnermaciel commented Feb 3, 2023

wagnermaciel commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Mar 31, 2023

wagnermaciel commented Mar 31, 2023

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

isantone commented Apr 21, 2023

angular-automatic-lock-bot bot commented May 22, 2023