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

fix: Tooltip: Don't close if user hovers on opener then tooltip content #5320

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: Tooltip: Don't close if user hovers on opener then tooltip content #5320

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f8419ce
fix: Tooltip: Don't close if user hovers on opener then tooltip content
margaree Jan 15, 2025
aae40f4
increase timeout slightly, clean up
margaree Jan 16, 2025
ebf513a
use private var
margaree Jan 16, 2025
3187893
Merge branch 'main' into tooltip-hover-content-dont-close
margaree Jan 16, 2025
d805da6
fix issue with filter-tags
margaree Jan 17, 2025
6c5d89d
move listeners into render
margaree Jan 17, 2025
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
35 changes: 31 additions & 4 deletions components/tooltip/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,9 @@ class Tooltip extends RtlMixin(LitElement) {
this._isHovering = false;
this._resizeRunSinceTruncationCheck = false;
this._viewportMargin = defaultViewportMargin;

this.#isHoveringTooltip = false;
this.#leftTooltip = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to a better name for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm yeah, I can see it being a little confusing. Idk, maybe mouseLeftTooltip would be a bit clearer. Or perhaps wasHoveringTooltip, or mouseLeavingTooltip?

}

/** @ignore */
Expand Down Expand Up @@ -523,7 +526,7 @@ class Tooltip extends RtlMixin(LitElement) {
return html`
<div class="d2l-tooltip-container">
<div class="d2l-tooltip-position" style=${styleMap(tooltipPositionStyle)}>
<div class="${classMap(contentClasses)}">
<div class="${classMap(contentClasses)}" @mouseenter="${this.#onTooltipMouseEnter}" @mouseleave="${this.#onTooltipMouseLeave}">
<div role="text">
<slot></slot>
</div>
Expand All @@ -532,7 +535,7 @@ class Tooltip extends RtlMixin(LitElement) {
<div class="d2l-tooltip-pointer d2l-tooltip-pointer-outline">
<div></div>
</div>
<div class="d2l-tooltip-pointer">
<div class="d2l-tooltip-pointer" @mouseenter="${this.#onTooltipMouseEnter}" @mouseleave="${this.#onTooltipMouseLeave}">
<div></div>
</div>
</div>`
Expand Down Expand Up @@ -645,6 +648,9 @@ class Tooltip extends RtlMixin(LitElement) {
this.style.height = `${positionRect.height}px`;
}

#isHoveringTooltip;
#leftTooltip;

_addListeners() {
if (!this._target) {
return;
Expand Down Expand Up @@ -840,6 +846,12 @@ class Tooltip extends RtlMixin(LitElement) {
}

_onTargetMouseEnter() {
// came from tooltip so keep showing
if (this.#leftTooltip) {
this._isHovering = true;
return;
}

this._hoverTimeout = setTimeout(async() => {
if (this.showTruncatedOnly) {
await this._updateTruncating();
Expand All @@ -855,7 +867,7 @@ class Tooltip extends RtlMixin(LitElement) {
_onTargetMouseLeave() {
clearTimeout(this._hoverTimeout);
this._isHovering = false;
this._updateShowing();
setTimeout(this._updateShowing.bind(this), 100); // delay to allow for mouseenter to fire if hovering on tooltip
Copy link
Member

Choose a reason for hiding this comment

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

Nit: arrow functions allow you to avoid bind

Suggested change
setTimeout(this._updateShowing.bind(this), 100); // delay to allow for mouseenter to fire if hovering on tooltip
setTimeout(() => this._updateShowing(), 100); // delay to allow for mouseenter to fire if hovering on tooltip

}

_onTargetResize() {
Expand Down Expand Up @@ -932,7 +944,7 @@ class Tooltip extends RtlMixin(LitElement) {
}

_updateShowing() {
this.showing = this._isFocusing || this._isHovering || this.forceShow;
this.showing = this._isFocusing || this._isHovering || this.forceShow || this.#isHoveringTooltip;
}

_updateTarget() {
Expand Down Expand Up @@ -1007,5 +1019,20 @@ class Tooltip extends RtlMixin(LitElement) {
this._resizeRunSinceTruncationCheck = false;
target.removeChild(cloneContainer);
}

#onTooltipMouseEnter() {
if (!this.showing) return;
this.#isHoveringTooltip = true;
this._updateShowing();
}

#onTooltipMouseLeave() {
this.#isHoveringTooltip = false;
this.#leftTooltip = true;
setTimeout(() => {
this.#leftTooltip = false;
this._updateShowing();
}, 100); // delay to allow for mouseenter to fire if hovering on target
Copy link
Member

Choose a reason for hiding this comment

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

Is there a weird race condition if they leave but then come back and leave again within this time period? Might be more obvious if you increase it to like 10s while testing. If this is a problem, you could capture the timeout identifier (the thing returned from setTimeout) and cancel the previous one using clearTimeout.

}
}
customElements.define('d2l-tooltip', Tooltip);
Loading