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.

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): fix infinite loop in self-managed tooltips #4269

Merged
merged 7 commits into from
Apr 29, 2024
14 changes: 7 additions & 7 deletions packages/tooltip/src/Tooltip.ts
Expand Up @@ -205,12 +205,12 @@ export class Tooltip extends SpectrumElement {
);
}

private get triggerElement(): HTMLElement {
private get triggerElement(): HTMLElement | null {
// Resolve the parent element of the assigned slot (if one exists) or of the Tooltip.
let start: HTMLElement = this.assignedSlot || this;
let root = start.getRootNode();
if (window.__swc.DEBUG) {
if (root === document) {
if (root === document) {
if (window.__swc.DEBUG) {
window.__swc.warn(
this,
`Self managed <${this.localName}> elements walk up the composed tree to acquire a trigger element. No trigger element was found before the document.`,
Expand All @@ -219,8 +219,8 @@ export class Tooltip extends SpectrumElement {
level: 'high',
}
);
return root as HTMLElement;
}
return null;
}
let triggerElement = (start.parentElement ||
(root as ShadowRoot).host ||
Expand All @@ -231,8 +231,8 @@ export class Tooltip extends SpectrumElement {
triggerElement.assignedSlot || (triggerElement as HTMLElement);
root = start.getRootNode();
/* c8 ignore next 13 */
if (window.__swc.DEBUG) {
if (root === document) {
if (root === document) {
if (window.__swc.DEBUG) {
window.__swc.warn(
this,
`Self managed <${this.localName}> elements walk up the composed tree to acquire a trigger element. No trigger element was found before the document.`,
Expand All @@ -241,8 +241,8 @@ export class Tooltip extends SpectrumElement {
level: 'high',
}
);
return root as HTMLElement;
}
return null;
}
triggerElement = (start.parentElement ||
(root as ShadowRoot).host ||
Expand Down
16 changes: 15 additions & 1 deletion packages/tooltip/test/tooltip.test.ts
Expand Up @@ -22,7 +22,7 @@ import {
} from '@open-wc/testing';
import { Button } from '@spectrum-web-components/button';
import '@spectrum-web-components/button/sp-button.js';
import { stub } from 'sinon';
import { spy, stub } from 'sinon';
import { testForLitDevWarnings } from '../../../test/testing-helpers.js';
import { sendMouse } from '../../../test/plugins/browser.js';

Expand Down Expand Up @@ -214,6 +214,20 @@ describe('Tooltip', () => {

expect(typeof el.tipElement).to.not.equal('undefined');
});

it('self managed does not attach event listeners if no trigger was found', async () => {
const documentEventsSpy = spy(document, 'addEventListener');
const el = await fixture<Tooltip>(
html`
<sp-tooltip self-managed>Help text.</sp-tooltip>
`
);

await elementUpdated(el);
expect(documentEventsSpy.callCount).to.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we confirm it doesn't attach to anything else, too?

Suggested change
expect(documentEventsSpy.callCount).to.equal(0);
expect(documentEventsSpy.callCount).to.equal(0);
expect(el.overlayElement?.triggerElement).to.be.null;

expect(el.overlayElement?.triggerElement).to.be.null;
documentEventsSpy.restore();
});
describe('dev mode', () => {
let consoleWarnStub!: ReturnType<typeof stub>;
before(() => {
Expand Down