From bb50a427cbe8424cbf69e81c40447deed0a3c023 Mon Sep 17 00:00:00 2001 From: Chuck Carpenter Date: Wed, 12 Aug 2026 15:22:10 +0200 Subject: [PATCH 1/2] fix: only trigger the step `destroy` event on real teardown `_setupElements()` rebuilds a step's element on every show, and it tore down the previous element by calling the public `destroy()`. So re-showing a step emitted `before-show` -> `destroy` -> `show`, which made `destroy` useless as a teardown hook: anything a step set up in `beforeShowPromise` got torn back down in the middle of the next show. Use `_teardownElements()` instead. It already exists to do exactly this teardown without emitting the public event, and `updateStepOptions()` was already using it. `destroy` now fires once, and only when the step is really being thrown away. Two things had to come along with it: - `advanceOn`'s only unbind path was `step.on('destroy', ...)`, so dropping the per-show event would have leaked a DOM listener on every show. `bindAdvance` now returns a cleanup function that teardown calls, which also closes out an old TODO about binding/unbinding on show/hide. - `_teardownElements()` wasn't idempotent. It clears the stored tabindex map but never resets `target`, so running it twice permanently dropped a target's original `tabindex`. Guarding on `isHTMLElement(this.el)` covers all three `el` states at once and fixes it. That was already reachable through `updateStepOptions()`. Fixes #3443 --- docs-src/src/content/docs/guides/usage.md | 50 +++++- shepherd.js/src/step.ts | 25 +-- shepherd.js/src/utils/bind.ts | 47 +++-- shepherd.js/test/unit/step.spec.js | 208 +++++++++++++++++++++- shepherd.js/test/unit/utils/bind.spec.js | 73 ++++++-- 5 files changed, 339 insertions(+), 64 deletions(-) diff --git a/docs-src/src/content/docs/guides/usage.md b/docs-src/src/content/docs/guides/usage.md index f889eba67..e89e7ee78 100644 --- a/docs-src/src/content/docs/guides/usage.md +++ b/docs-src/src/content/docs/guides/usage.md @@ -321,29 +321,61 @@ when: { ##### Step Methods - `show()`: Show this step -- `hide()`: Hide this step +- `hide()`: Hide this step. The step's element stays in the DOM, so the step can + be shown again later. - `cancel()`: Hide this step and trigger the `cancel` event - `complete()`: Hide this step and trigger the `complete` event - `scrollTo()`: Scroll to this step's element - `isOpen()`: Returns true if the step is currently shown -- `destroy()`: Remove the element +- `destroy()`: Permanently tear the step down — removes its element from the DOM, + destroys the Floating UI instance, and triggers the `destroy` event +- `updateStepOptions(options)`: Merge new options into the step and re-render its + element in place +- `getElement()`: Returns the step's element — `undefined` if the step has never + been shown, `null` if it has been destroyed +- `getTarget()`: Returns the step's resolved `attachTo` element - `on(eventName, handler, [context])`: Bind an event - `off(eventName, [handler])`: Unbind an event - `once(eventName, handler, [context])`: Bind just the next instance of an event ##### Step Events -- `before-show` -- `show` -- `before-hide` -- `hide` -- `complete` -- `cancel` -- `destroy` +- `before-show`: Triggered at the start of every `show()`, before the step's + element is created +- `show`: Triggered at the end of every `show()`, once the element is in the DOM + and positioned +- `before-hide`: Triggered at the start of every `hide()` +- `hide`: Triggered at the end of every `hide()` +- `complete`: Triggered by `step.complete()` +- `cancel`: Triggered by `step.cancel()` +- `destroy`: Triggered when the step is disposed of for good — by + `step.destroy()`, by `tour.removeStep(id)`, or for every step in the tour when + the tour completes or is cancelled Please note that `complete` and `cancel` are only ever triggered if you call the associated methods in your code. +##### Step Lifecycle + +| What happens | Events, in order | +| -------------------------------------------------- | ------------------------------ | +| A step is shown for the first time | `before-show`, `show` | +| Advancing away with `next()`, `back()`, `show(id)` | `before-hide`, `hide` | +| The same step is shown again later | `before-show`, `show` | +| `tour.removeStep(id)` on the step that is open | `before-hide`, `hide`, `destroy` | +| `tour.complete()` or `tour.cancel()` | `destroy`, once for every step | + +Shepherd rebuilds a step's element from scratch on every `show()`, but that is an +implementation detail — `destroy` fires **once**, and only when the step is +really being thrown away. Use `destroy` to release anything you allocated for the +step, and `before-hide` / `hide` for work that should run every time the step +goes away. + +> **Behavior change** — `destroy` used to also fire every time an already-shown +> step was shown again, in between `before-show` and `show`, because the element +> is recreated on each show. Recreating the element no longer triggers `destroy`. +> See [#3443](https://github.com/shipshapecode/shepherd/issues/3443). + ### Advancing on Actions You can use the `advanceOn` option, or the Next button, to advance steps. If you diff --git a/shepherd.js/src/step.ts b/shepherd.js/src/step.ts index 9e1587a0f..77ddc808e 100644 --- a/shepherd.js/src/step.ts +++ b/shepherd.js/src/step.ts @@ -1,12 +1,7 @@ import { deepmerge } from 'deepmerge-ts'; import { Evented } from './evented.ts'; import autoBind from './utils/auto-bind.ts'; -import { - isElement, - isHTMLElement, - isFunction, - isUndefined -} from './utils/type-check.ts'; +import { isElement, isHTMLElement, isFunction } from './utils/type-check.ts'; import { bindAdvance } from './utils/bind.ts'; import { parseAttachTo, @@ -361,6 +356,7 @@ export interface StepOptionsWhen { * @extends {Evented} */ export class Step extends Evented { + _advanceOnCleanup?: (() => void) | null; _resolvedAttachTo: StepOptionsAttachTo | null; _resolvedExtraHighlightElements?: HTMLElement[]; _originalTabIndexes: Map; @@ -438,6 +434,11 @@ export class Step extends Evented { * @private */ _teardownElements() { + if (this._advanceOnCleanup) { + this._advanceOnCleanup(); + this._advanceOnCleanup = null; + } + destroyTooltip(this); if (this.shepherdElementComponent) { @@ -683,7 +684,7 @@ export class Step extends Evented { this.options.classes = this._getClassOptions(options); - this.destroy(); + this._teardownElements(); this.id = this.options.id || `step-${uuid()}`; if (when) { @@ -696,17 +697,21 @@ export class Step extends Evented { /** * Create the element and set up the FloatingUI instance + * + * The element is recreated on every show, so any previously mounted element is + * torn down first. That teardown is internal — it must not emit the public + * `destroy` event, which means "this step is gone for good". * @private */ _setupElements() { - if (!isUndefined(this.el)) { - this.destroy(); + if (isHTMLElement(this.el)) { + this._teardownElements(); } this.el = this._createTooltipContent(); if (this.options.advanceOn) { - bindAdvance(this); + this._advanceOnCleanup = bindAdvance(this) ?? null; } // The tooltip implementation details are handled outside of the Step diff --git a/shepherd.js/src/utils/bind.ts b/shepherd.js/src/utils/bind.ts index bfe54ebf5..eea7939cc 100644 --- a/shepherd.js/src/utils/bind.ts +++ b/shepherd.js/src/utils/bind.ts @@ -25,40 +25,33 @@ function _setupAdvanceOnHandler(step: Step, selector?: string) { /** * Bind the event handler for advanceOn * @param step The step instance + * @return A function that removes the listener, or `undefined` if nothing was bound */ -export function bindAdvance(step: Step) { +export function bindAdvance(step: Step): (() => void) | undefined { // An empty selector matches the step element const { event, selector } = step.options.advanceOn || {}; - if (event) { - const handler = _setupAdvanceOnHandler(step, selector); - // TODO: this should also bind/unbind on show/hide - let el: Element | null = null; + if (!event) { + console.error('advanceOn was defined, but no event name was passed.'); + return; + } - if (!isUndefined(selector)) { - el = document.querySelector(selector); + const handler = _setupAdvanceOnHandler(step, selector); - if (!el) { - return console.error( - `No element was found for the selector supplied to advanceOn: ${selector}` - ); - } - } + if (!isUndefined(selector)) { + const el = document.querySelector(selector); - if (el) { - el.addEventListener(event, handler); - step.on('destroy', () => { - return (el as HTMLElement).removeEventListener(event, handler); - }); - } else { - document.body.addEventListener(event, handler, true); - step.on('destroy', () => { - return document.body.removeEventListener(event, handler, true); - }); + if (!el) { + console.error( + `No element was found for the selector supplied to advanceOn: ${selector}` + ); + return; } - } else { - return console.error( - 'advanceOn was defined, but no event name was passed.' - ); + + el.addEventListener(event, handler); + return () => el.removeEventListener(event, handler); } + + document.body.addEventListener(event, handler, true); + return () => document.body.removeEventListener(event, handler, true); } diff --git a/shepherd.js/test/unit/step.spec.js b/shepherd.js/test/unit/step.spec.js index 3a7ec9d9f..82e0c7d4a 100644 --- a/shepherd.js/test/unit/step.spec.js +++ b/shepherd.js/test/unit/step.spec.js @@ -429,21 +429,43 @@ describe('Tour | Step', () => { }); describe('_setupElements()', () => { - it('calls destroy on the step if the content element is already set', () => { + it('tears down the existing element without triggering `destroy`', () => { const step = new Step(tour, {}); - let destroyCalled = false; + const teardownSpy = vi.spyOn(step, '_teardownElements'); + const destroySpy = vi.fn(); + step.on('destroy', destroySpy); step.el = document.createElement('a'); - step.destroy = () => (destroyCalled = true); + step._setupElements(); + expect( - destroyCalled, - '_setupElements method called destroy with element set' - ).toBeTruthy(); + teardownSpy, + '_setupElements tore down the previously mounted element' + ).toHaveBeenCalledTimes(1); + expect( + destroySpy, + 'recreating the element did not trigger the public `destroy` event' + ).not.toHaveBeenCalled(); + }); + + it('does not tear down again if the step was already destroyed', () => { + const step = new Step(tour, {}); + step.el = document.createElement('a'); + step.destroy(); + + const teardownSpy = vi.spyOn(step, '_teardownElements'); + step._setupElements(); + + expect( + teardownSpy, + '_setupElements skipped teardown for an already destroyed step' + ).not.toHaveBeenCalled(); }); it('calls destroy on the tooltip if it already exists', () => { const step = new Step(tour, {}); let destroyCalled = false; + step.el = document.createElement('a'); step.cleanup = () => { destroyCalled = true; }; @@ -917,4 +939,178 @@ describe('Tour | Step', () => { expect(testElement.getAttribute('tabindex')).toBe('3'); }); }); + + describe('step lifecycle', () => { + let instance; + let testElement; + + beforeEach(() => { + testElement = document.createElement('div'); + testElement.id = 'lifecycle-test-element'; + document.body.appendChild(testElement); + }); + + afterEach(() => { + instance?.complete(); + testElement?.remove(); + }); + + it('does not trigger `destroy` when a step is shown again', () => { + instance = new Shepherd.Tour({ + steps: [ + { id: 'first', text: 'First' }, + { id: 'second', text: 'Second' } + ] + }); + + const destroySpy = vi.fn(); + instance.getById('first').on('destroy', destroySpy); + + instance.start(); + instance.next(); + instance.back(); + + expect( + destroySpy, + 'recreating the element on show does not trigger `destroy`' + ).not.toHaveBeenCalled(); + }); + + it('triggers `destroy` once for each step when the tour completes', () => { + instance = new Shepherd.Tour({ + steps: [ + { id: 'first', text: 'First' }, + { id: 'second', text: 'Second' } + ] + }); + + const firstDestroy = vi.fn(); + const secondDestroy = vi.fn(); + instance.getById('first').on('destroy', firstDestroy); + instance.getById('second').on('destroy', secondDestroy); + + instance.start(); + instance.next(); + instance.complete(); + + expect(firstDestroy).toHaveBeenCalledTimes(1); + expect(secondDestroy).toHaveBeenCalledTimes(1); + }); + + it('does not accumulate `advanceOn` listeners across shows', () => { + const addSpy = vi.spyOn(testElement, 'addEventListener'); + const removeSpy = vi.spyOn(testElement, 'removeEventListener'); + + instance = new Shepherd.Tour({ + steps: [ + { + id: 'first', + text: 'First', + advanceOn: { selector: '#lifecycle-test-element', event: 'click' } + }, + { id: 'second', text: 'Second' } + ] + }); + + instance.start(); // binds + instance.next(); + instance.back(); // unbinds, then binds again + instance.complete(); // unbinds + + const countFor = (spy) => + spy.mock.calls.filter(([event]) => event === 'click').length; + + expect(countFor(addSpy), 'advanceOn bound once per show').toBe(2); + expect(countFor(removeSpy), 'every bound listener was removed').toBe(2); + + addSpy.mockRestore(); + removeSpy.mockRestore(); + }); + + // https://github.com/shipshapecode/shepherd/issues/3443 + it('does not run `destroy` cleanup when re-showing a step whose target is set up in `beforeShowPromise`', async () => { + // `Tour.show()` does not return the step's show promise, so let the + // `beforeShowPromise` chain settle before asserting + const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); + let opened = 0; + let closed = 0; + + // Stands in for opening/closing a three-dots menu that holds the target + const openMenu = () => { + opened++; + document.querySelector('#menu-item')?.remove(); + const item = document.createElement('button'); + item.id = 'menu-item'; + testElement.appendChild(item); + }; + const closeMenu = () => { + closed++; + document.querySelector('#menu-item')?.remove(); + }; + + instance = new Shepherd.Tour({ + steps: [ + { id: 'intro', text: 'Intro' }, + { + id: 'menu-step', + text: 'Lives inside the menu', + attachTo: { element: '#menu-item', on: 'right' }, + beforeShowPromise: () => Promise.resolve(openMenu()), + when: { destroy: closeMenu } + } + ] + }); + + await instance.start(); + instance.next(); + await settle(); + + expect(opened, 'menu opened for the step').toBe(1); + expect(closed, 'no cleanup yet').toBe(0); + + instance.back(); + await settle(); + instance.next(); + await settle(); + + expect(opened, 'menu re-opened on re-show').toBe(2); + expect(closed, 're-showing the step did not run `destroy` cleanup').toBe( + 0 + ); + expect( + instance.getById('menu-step').getTarget()?.id, + 'step is attached to the live target' + ).toBe('menu-item'); + + instance.complete(); + + expect(closed, 'cleanup ran exactly once, at the end of the tour').toBe( + 1 + ); + expect(document.querySelector('#menu-item')).toBeNull(); + }); + + it('preserves the original tabindex across `updateStepOptions`', () => { + testElement.setAttribute('tabindex', '2'); + + instance = new Shepherd.Tour({ + steps: [ + { + id: 'first', + text: 'First', + attachTo: { element: '#lifecycle-test-element', on: 'top' } + } + ] + }); + + instance.start(); + expect(testElement.getAttribute('tabindex')).toBe('0'); + + // Tearing down and rebuilding the element must not lose the stored value + instance.getById('first').updateStepOptions({ text: 'Updated' }); + instance.getById('first').hide(); + + expect(testElement.getAttribute('tabindex')).toBe('2'); + }); + }); }); diff --git a/shepherd.js/test/unit/utils/bind.spec.js b/shepherd.js/test/unit/utils/bind.spec.js index 4c945dfa4..beb800115 100644 --- a/shepherd.js/test/unit/utils/bind.spec.js +++ b/shepherd.js/test/unit/utils/bind.spec.js @@ -79,23 +79,72 @@ describe('Bind Utils', function () { expect(hasAdvanced, '`next()` triggered for advanceOn').toBeTruthy(); }); - it('calls `removeEventListener` when destroyed', () => { - return new Promise((done) => { - const bodySpy = vi.spyOn(document.body, 'removeEventListener'); - const step = new Step(tourProto, { - advanceOn: { event: advanceOnEventName } - }); + it('returns a cleanup function that calls `removeEventListener`', () => { + const bodySpy = vi.spyOn(document.body, 'removeEventListener'); + const step = new Step(tourProto, { + advanceOn: { event: advanceOnEventName } + }); - step.isOpen = () => true; + step.isOpen = () => true; - bindAdvance(step); - step.trigger('destroy'); + const cleanup = bindAdvance(step); + expect(cleanup, 'bindAdvance returned a cleanup function').toBeTypeOf( + 'function' + ); - expect(bodySpy).toHaveBeenCalled(); - bodySpy.mockRestore(); + cleanup(); - done(); + expect(bodySpy).toHaveBeenCalledWith( + advanceOnEventName, + expect.any(Function), + true + ); + bodySpy.mockRestore(); + }); + + it('removes the listener bound to a selector', () => { + const step = new Step(tourProto, { + advanceOn: { + selector: `.${advanceOnSelector}`, + event: advanceOnEventName + } }); + + step.isOpen = () => true; + + const cleanup = bindAdvance(step); + const linkSpy = vi.spyOn(link, 'removeEventListener'); + + cleanup(); + + expect(linkSpy).toHaveBeenCalledWith( + advanceOnEventName, + expect.any(Function) + ); + linkSpy.mockRestore(); + }); + + // `console.error` is already replaced with a mock in setupTests.js + it('returns undefined when there is nothing to bind', () => { + expect( + bindAdvance(new Step(tourProto, { advanceOn: {} })), + 'no event name passed' + ).toBeUndefined(); + expect(console.error).toHaveBeenCalledWith( + 'advanceOn was defined, but no event name was passed.' + ); + + expect( + bindAdvance( + new Step(tourProto, { + advanceOn: { selector: '.does-not-exist', event: 'click' } + }) + ), + 'selector matched no element' + ).toBeUndefined(); + expect(console.error).toHaveBeenCalledWith( + 'No element was found for the selector supplied to advanceOn: .does-not-exist' + ); }); }); }); From c46adfdb31a42bc28215b3ee19625eeb8cd25dc2 Mon Sep 17 00:00:00 2001 From: Chuck Carpenter Date: Wed, 12 Aug 2026 15:28:55 +0200 Subject: [PATCH 2/2] docs: scope the destroy once-only claim to element recreation `Step.destroy()` has no destroyed-state guard, so calling it explicitly and then completing the tour emits `destroy` twice. Say what is actually guaranteed: recreating the element on show does not emit the event. --- docs-src/src/content/docs/guides/usage.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs-src/src/content/docs/guides/usage.md b/docs-src/src/content/docs/guides/usage.md index 0d42d930d..43c18852b 100644 --- a/docs-src/src/content/docs/guides/usage.md +++ b/docs-src/src/content/docs/guides/usage.md @@ -398,10 +398,14 @@ associated methods in your code. | `tour.complete()` or `tour.cancel()` | `destroy`, once for every step | Shepherd rebuilds a step's element from scratch on every `show()`, but that is -an implementation detail — `destroy` fires **once**, and only when the step is -really being thrown away. Use `destroy` to release anything you allocated for -the step, and `before-hide` / `hide` for work that should run every time the -step goes away. +an implementation detail — recreating the element does **not** emit `destroy`. +The event fires only when the step is actually being thrown away, so use it to +release anything you allocated for the step, and `before-hide` / `hide` for work +that should run every time the step goes away. + +`destroy()` is not guarded against running more than once, so calling it +yourself and then completing the tour will emit `destroy` twice. Keep your +teardown idempotent if you do both. > **Behavior change** — `destroy` used to also fire every time an already-shown > step was shown again, in between `before-show` and `show`, because the element