perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled - #19764
Conversation
…e tooltip is not disabled
googlebot
added
the
cla: yes
| /** 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 || |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm fine with this behavior with the names in your follow-up PR
|
|
||
| ngAfterViewInit() { | ||
| // This needs to happen after view init so the initial values for all inputs have been set. | ||
| this._viewInitialized = true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Better to be safe, right?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
jelbourn
added
G
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
LGTM, the comment about message could be resolved here or in the follow-up PR
| get message() { return this._message; } | ||
| set message(value: string) { | ||
| this._ariaDescriber.removeDescription(this._elementRef.nativeElement, this._message); | ||
| if (this._message) { |
There was a problem hiding this comment.
Rather than checking message here, could AriaDescriber just check this and no-op immediately?
| /** 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 || |
There was a problem hiding this comment.
I'm fine with this behavior with the names in your follow-up PR
…e tooltip is not disabled (#19764) * perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled * comment update
…e tooltip is not disabled (angular#19764) * perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled * comment update
…e tooltip is not disabled (angular#19764) * perf(tooltip): Defer hooking up events until there's a message and the tooltip is not disabled * comment update
|
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. |
5 participants
No description provided.