-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(tooltip): wrong position when using OnPush change detection #3671
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import { | |
OnDestroy, | ||
Renderer, | ||
OnInit, | ||
ChangeDetectorRef | ||
ChangeDetectorRef, | ||
} from '@angular/core'; | ||
import { | ||
Overlay, | ||
|
@@ -155,7 +155,7 @@ export class MdTooltip implements OnInit, OnDestroy { | |
@Optional() private _dir: Dir) { | ||
|
||
// The mouse events shouldn't be bound on iOS devices, because | ||
// they can prevent the first tap from firing it's click event. | ||
// they can prevent the first tap from firing its click event. | ||
if (!_platform.IOS) { | ||
_renderer.listen(_elementRef.nativeElement, 'mouseenter', () => this.show()); | ||
_renderer.listen(_elementRef.nativeElement, 'mouseleave', () => this.hide()); | ||
|
@@ -311,6 +311,8 @@ export class MdTooltip implements OnInit, OnDestroy { | |
// Must wait for the message to be painted to the tooltip so that the overlay can properly | ||
// calculate the correct positioning based on the size of the text. | ||
this._tooltipInstance.message = message; | ||
this._tooltipInstance._updateView(); | ||
|
||
this._ngZone.onMicrotaskEmpty.first().subscribe(() => { | ||
if (this._tooltipInstance) { | ||
this._overlayRef.updatePosition(); | ||
|
@@ -393,7 +395,7 @@ export class TooltipComponent { | |
// Mark for check so if any parent component has set the | ||
// ChangeDetectionStrategy to OnPush it will be checked anyways | ||
this._changeDetectorRef.markForCheck(); | ||
setTimeout(() => { this._closeOnInteraction = true; }, 0); | ||
setTimeout(() => this._closeOnInteraction = true, 0); | ||
}, delay); | ||
} | ||
|
||
|
@@ -439,8 +441,8 @@ export class TooltipComponent { | |
case 'after': this._transformOrigin = isLtr ? 'left' : 'right'; break; | ||
case 'left': this._transformOrigin = 'right'; break; | ||
case 'right': this._transformOrigin = 'left'; break; | ||
case 'above': this._transformOrigin = 'bottom'; break; | ||
case 'below': this._transformOrigin = 'top'; break; | ||
case 'above': this._transformOrigin = 'bottom'; break; | ||
case 'below': this._transformOrigin = 'top'; break; | ||
default: throw new MdTooltipInvalidPositionError(value); | ||
} | ||
} | ||
|
@@ -461,4 +463,13 @@ export class TooltipComponent { | |
this.hide(0); | ||
} | ||
} | ||
|
||
/** | ||
* Ensures that the tooltip text has rendered. Mainly used for rendering the initial text | ||
* before positioning a tooltip, which can be problematic in components with OnPush change | ||
* detection. | ||
*/ | ||
_updateView(): void { | ||
this._changeDetectorRef.detectChanges(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason why you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I thought I tried it and it wasn't working. Looking at it again, it seems like it's the same. I'll change it. |
||
} | ||
} |
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.
Does there need to be a test component w/
OnPush
that would have exhibited the original behavior?Uh oh!
There was an error while loading. Please reload this page.
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.
It is using the one that we test
OnPush
against: https://github.com/angular/material2/pull/3671/files#diff-90d01e051e22bab8526ca923091f589fR391