Skip to content

perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled #19764

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

Merged
merged 2 commits into from
Jul 14, 2020
Merged

perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled #19764

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72cf7e0
perf(tooltip): Defer hooking up events until there's a message and th…
kseamon Jun 25, 2020
acbc194
comment update
kseamon Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/material/tooltip/tooltip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
private _disabled: boolean = false;
private _tooltipClass: string|string[]|Set<string>|{[key: string]: any};
private _scrollStrategy: () => ScrollStrategy;
private _viewInitialized = false;

/** Allows the user to define the position of the tooltip relative to the parent element */
@Input('matTooltipPosition')
Expand Down Expand Up @@ -173,6 +174,8 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
// If tooltip is disabled, hide immediately.
if (this._disabled) {
this.hide(0);
} else {
this._setupPointerEvents();
}
}

Expand Down Expand Up @@ -202,14 +205,17 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
@Input('matTooltip')
get message() { return this._message; }
set message(value: string) {
this._ariaDescriber.removeDescription(this._elementRef.nativeElement, this._message);
if (this._message) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than checking message here, could AriaDescriber just check this and no-op immediately?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating in #19777

this._ariaDescriber.removeDescription(this._elementRef.nativeElement, this._message);
}

// If the message is not a string (e.g. number), convert it to a string and trim it.
this._message = value != null ? `${value}`.trim() : '';

if (!this._message && this._isTooltipVisible()) {
this.hide(0);
} else {
this._setupPointerEvents();
this._updateTooltipMessage();
this._ngZone.runOutsideAngular(() => {
// The `AriaDescriber` has some functionality that avoids adding a description if it's the
Expand Down Expand Up @@ -276,6 +282,7 @@ export class MatTooltip implements OnDestroy, AfterViewInit {

ngAfterViewInit() {
// This needs to happen after view init so the initial values for all inputs have been set.
this._viewInitialized = true;
Copy link
Member

Choose a reason for hiding this comment

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

I think the only case where this would help is if the message changes quickly before ngAfterViewInit has fired, but I don't know if that's actually possible, or if it is, it's very unlikely. In theory it can happen if people were to go directly through the setter, but they'd have to do it inside a constructor for it to matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better to be safe, right?

Copy link
Member

@crisbeto crisbeto Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO it's overly-protective since people would have to actively go out of their way to make it behave differently, and even if it does, it won't actually break, it'll just bind some events before they're actually needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was there a reason that it hooked up in ngAfterViewInit before? My concern was that adding the call to the message and disabled setters would cause the events to be attached earlier than they were before without this check.

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 there's a specific reason for it. My guess is that we had the lifecycle hook defined already so we reused it instead of adding another one.

this._setupPointerEvents();

this._focusMonitor.monitor(this._elementRef)
Expand Down Expand Up @@ -543,6 +550,12 @@ export class MatTooltip implements OnDestroy, AfterViewInit {

/** Binds the pointer events to the tooltip trigger. */
private _setupPointerEvents() {
// Optimization: Defer hooking up events if there's no message or the tooltip is disabled.
if (this._disabled || !this.message || !this._viewInitialized ||
Copy link
Member

Choose a reason for hiding this comment

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

I think that this check should be at the call site, rather than inside setupPointEvents, otherwise the caller has no way of knowing that the events were actually bound. We can pull out this logic into a utility method if it needs to be checked in multiple places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's safer to be here - otherwise it could accidentally be omitted. Would adding a return boolean value here be ok for if the caller needs to know?

Alternatively, they can just check this._passiveListeners.size

Copy link
Member

Choose a reason for hiding this comment

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

This is a private API so we can probably get away with not returning what the result was. Let's keep it like this for now and we can rework it if it becomes an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this behavior with the names in your follow-up PR

this._passiveListeners.size) {
return;
}

// The mouse events shouldn't be bound on mobile devices, because they can prevent the
// first tap from firing its click event or can cause the tooltip to open for clicks.
if (!this._platform.IOS && !this._platform.ANDROID) {
Expand Down