fix: only trigger the step destroy event on real teardown

docs-src/src/content/docs/guides/usage.md

@@ -353,29 +353,65 @@ 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 — 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
+> 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

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,
@@ -402,6 +397,7 @@ export interface StepOptionsWhen {
  * @extends {Evented}
  */
 export class Step extends Evented {
+  _advanceOnCleanup?: (() => void) | null;
   _resolvedAttachTo: StepOptionsAttachTo | null;
   _resolvedExtraHighlightElements?: HTMLElement[];
   _originalTabIndexes: Map<Element, string>;
@@ -479,6 +475,11 @@ export class Step extends Evented {
    * @private
    */
   _teardownElements() {
+    if (this._advanceOnCleanup) {
+      this._advanceOnCleanup();
+      this._advanceOnCleanup = null;
+    }
+
     destroyTooltip(this);
 
     if (this.shepherdElementComponent) {
@@ -724,7 +725,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) {
@@ -737,17 +738,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

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);
 }