Skip to content

Commit a96fcbb

Browse files
floooatclaude
andcommitted
fix(185122): don't anchor product tour steps to hidden elements
A stored step selector that only matches a display:none node (a collapsed menu's label, the hidden copy of a responsive menu) was treated as found: the popover pinned to the viewport corner, the overlay blocked the page and a CLICK step had no way forward, so the visitor was stuck until the tour was unpublished (and reloads resumed it). Resolve steps to the first rendered match instead; with none, the existing 5s wait ends in onElementNotFound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent e2554ca commit a96fcbb

2 files changed

Lines changed: 122 additions & 3 deletions

File tree

‎src/GleapTours.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { querySelectorSafe } from './GleapSafeSelector';
1+
import { querySelectorAllSafe } from './GleapSafeSelector';
22

33
const GleapTours = (function () {
44
'use strict';
@@ -74,6 +74,19 @@ const GleapTours = (function () {
7474
function isElementVisible(el) {
7575
return !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length);
7676
}
77+
// A selector recorded on one layout can match a display:none node on another
78+
// (a collapsed menu's label, the hidden copy of a responsive menu). Anchoring a
79+
// step to it pins the popover to the viewport corner while the overlay blocks
80+
// the page, so only rendered matches count.
81+
function findRenderedElement(selector) {
82+
const matches = querySelectorAllSafe(selector);
83+
for (let i = 0; i < matches.length; i++) {
84+
if (isElementVisible(matches[i])) {
85+
return matches[i];
86+
}
87+
}
88+
return null;
89+
}
7790
let currentState = {};
7891
function setState(key, value) {
7992
currentState[key] = value;
@@ -238,7 +251,7 @@ const GleapTours = (function () {
238251
if (typeof elemObj === 'string') {
239252
// Stored selectors can be hand typed and invalid; that has to end in
240253
// onElementNotFound below instead of an exception inside the retry timer.
241-
elemObj = querySelectorSafe(element);
254+
elemObj = findRenderedElement(element);
242255
}
243256
if (element && !elemObj && attemptTime >= 0) {
244257
setTimeout(() => {
@@ -273,7 +286,7 @@ const GleapTours = (function () {
273286
// Re-query the element if the cached reference is no longer in the DOM.
274287
if (!document.body.contains(activeHighlight) && activeStep.element && typeof activeStep.element === 'string') {
275288
try {
276-
var freshEl = querySelectorSafe(activeStep.element);
289+
var freshEl = findRenderedElement(activeStep.element);
277290
if (freshEl) {
278291
activeHighlight = freshEl;
279292
setState('__activeElement', freshEl);

‎src/GleapTours.test.js‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/**
2+
* @jest-environment jsdom
3+
*/
4+
import GleapTours from './GleapTours';
5+
6+
// jsdom has no layout, so every element reports no client rects. Treat an
7+
// element as rendered unless it or an ancestor is display:none, which is what
8+
// a browser does for v-show / [hidden] / collapsed-menu labels.
9+
const isHiddenInTest = (el) => {
10+
for (let node = el; node && node.style; node = node.parentElement) {
11+
if (node.style.display === 'none') {
12+
return true;
13+
}
14+
}
15+
return false;
16+
};
17+
18+
describe('GleapTours step target resolution', () => {
19+
let tour;
20+
let onElementNotFound;
21+
22+
beforeEach(() => {
23+
jest.useFakeTimers();
24+
jest.spyOn(Element.prototype, 'getClientRects').mockImplementation(function () {
25+
return isHiddenInTest(this) ? [] : [{ top: 0, left: 0, width: 40, height: 20 }];
26+
});
27+
onElementNotFound = jest.fn(() => tour.destroy());
28+
});
29+
30+
afterEach(() => {
31+
if (tour) {
32+
tour.destroy();
33+
}
34+
tour = null;
35+
jest.runOnlyPendingTimers();
36+
jest.useRealTimers();
37+
jest.restoreAllMocks();
38+
document.body.innerHTML = '';
39+
});
40+
41+
const startTour = (selector) => {
42+
tour = GleapTours({
43+
animate: false,
44+
allowClose: false,
45+
onElementNotFound,
46+
steps: [{ element: selector, popover: { description: 'Click on Library', showButtons: [] } }],
47+
});
48+
tour.drive();
49+
};
50+
51+
it('highlights a rendered target right away', () => {
52+
document.body.innerHTML = '<nav><a class="menu-item"><span class="label">Library</span></a></nav>';
53+
startTour('.menu-item > .label');
54+
55+
expect(tour.getActiveElement()).toBe(document.querySelector('.label'));
56+
expect(onElementNotFound).not.toHaveBeenCalled();
57+
});
58+
59+
it('treats a target that only matches a display:none element as not found (#185122)', () => {
60+
// Collapsed sidebar: the label the step was recorded on is hidden via v-show.
61+
document.body.innerHTML =
62+
'<nav><a class="menu-item"><i class="icon"></i><span class="label" style="display: none">Library</span></a></nav>';
63+
startTour('.menu-item > .label');
64+
65+
expect(tour.getActiveElement()).toBeUndefined();
66+
expect(document.querySelector('.label').classList.contains('gleap-tour-active-element')).toBe(false);
67+
68+
jest.advanceTimersByTime(5200);
69+
70+
expect(onElementNotFound).toHaveBeenCalledTimes(1);
71+
expect(onElementNotFound.mock.calls[0][0].element).toBe('.menu-item > .label');
72+
});
73+
74+
it('skips hidden matches and highlights the first rendered one', () => {
75+
// Responsive layouts render the same menu twice and hide one of them.
76+
document.body.innerHTML =
77+
'<div class="mobile" style="display: none"><a class="menu-item"><span class="label">Library</span></a></div>' +
78+
'<div class="desktop"><a class="menu-item"><span class="label">Library</span></a></div>';
79+
startTour('.menu-item > .label');
80+
81+
expect(tour.getActiveElement()).toBe(document.querySelector('.desktop .label'));
82+
expect(onElementNotFound).not.toHaveBeenCalled();
83+
});
84+
85+
it('keeps waiting for a hidden target that becomes visible within the timeout', () => {
86+
document.body.innerHTML = '<nav><a class="menu-item"><span class="label" style="display: none">Library</span></a></nav>';
87+
startTour('.menu-item > .label');
88+
89+
jest.advanceTimersByTime(1000);
90+
document.querySelector('.label').style.display = '';
91+
jest.advanceTimersByTime(200);
92+
93+
expect(tour.getActiveElement()).toBe(document.querySelector('.label'));
94+
jest.advanceTimersByTime(5000);
95+
expect(onElementNotFound).not.toHaveBeenCalled();
96+
});
97+
98+
it('still quits when the selector matches nothing', () => {
99+
document.body.innerHTML = '<nav></nav>';
100+
startTour('.menu-item > .label');
101+
102+
jest.advanceTimersByTime(5200);
103+
104+
expect(onElementNotFound).toHaveBeenCalledTimes(1);
105+
});
106+
});

0 commit comments

Comments
 (0)