fix: don't crash when the menu is gone by the time the layer click arrives

CHANGELOG.md

@@ -27,6 +27,7 @@
 * A `select`'s phantom mousedown is no longer treated as an outside click (fixes #744)
 * A re-dispatched layer click now targets the element actually clicked (fixes #771)
 * Guard against undefined `e.data` in the contextmenu handler (fixes #777)
+* Clicking on after the menu was destroyed no longer throws with `useModal: false` (fixes #805)
 
 #### Documentation
 

src/jquery.contextMenu.js

@@ -739,8 +739,20 @@
 
                 // if the click closing is done through windwow event listener rather than a transparent layer
                 if (!root.$layer) {
+                    // There may be no menu left to hide at all: this listener
+                    // outlives the menu it was registered for, so the menu can
+                    // already be gone by the time the next click arrives - a
+                    // `build` menu empties its own options object once it has
+                    // finished hiding (see op.hide()), and a `hide` event
+                    // handler calling $(selector).contextMenu('destroy') tears
+                    // it down outright. `$menu` is left either dropped
+                    // altogether or as an empty jQuery object, and the latter
+                    // used to throw on the $menu[0] dereference below.
+                    // See https://github.com/swisnl/jQuery-contextMenu/issues/805
+                    var menuIsGone = !root.$menu || !root.$menu.length;
+
                     target = document.elementFromPoint(x - $win.scrollLeft(), y - $win.scrollTop());
-                    if (root.$menu === null || typeof root.$menu === 'undefined' || (!root.$menu[0].contains(target) && !isWithinDetachedSubmenus(root, target))) {
+                    if (menuIsGone || (!root.$menu[0].contains(target) && !isWithinDetachedSubmenus(root, target))) {
                         // Choosing an option from a native <select> item's dropdown can
                         // make Firefox fire a spurious click/mousedown shortly
                         // afterwards, at coordinates that don't necessarily land within
@@ -756,6 +768,19 @@
                             return;
                         }
 
+                        // Nothing left to hide, so skip straight to the cleanup:
+                        // this used to call root.$menu.trigger() regardless and
+                        // throw "Cannot read properties of undefined (reading
+                        // 'trigger')", which also kept `onhide` from running, so
+                        // op.layer()'s dismiss listener was never unregistered
+                        // and one more of them piled up per menu opened.
+                        // See https://github.com/swisnl/jQuery-contextMenu/issues/805
+                        if (menuIsGone) {
+                            if (typeof onhide !== 'undefined')
+                                onhide();
+                            return;
+                        }
+
                         root.$menu.trigger('contextmenu:hide');
                         if (typeof onhide !== 'undefined')
                             onhide();

test/specs/issue-805-no-modal-destroy.js

@@ -0,0 +1,109 @@
+const { test, expect } = require('@playwright/test');
+const { fixture } = require('../support/helpers');
+
+// Regression test for https://github.com/swisnl/jQuery-contextMenu/issues/805
+//
+// With `useModal: false` the menu is dismissed by a document-level mousedown
+// listener instead of by the transparent layer. That listener outlives the menu
+// it belongs to, so the next click has to cope with the menu already being
+// gone - here because a `hide` handler destroys it. handle.layerClick() used to
+// dereference the missing menu, which threw and, since the throw happened
+// before the listener could unregister itself, left one more listener behind
+// for every menu that had been opened.
+
+// Count the dismiss listeners the plugin registers, before any page script runs.
+async function countDismissListeners(page) {
+  await page.addInitScript(() => {
+    window.__dismissListeners = 0;
+    const add = document.addEventListener;
+    const remove = document.removeEventListener;
+    document.addEventListener = function (type, fn, capture) {
+      if (type === 'mousedown' && capture === true) {
+        window.__dismissListeners++;
+      }
+      return add.apply(document, arguments);
+    };
+    document.removeEventListener = function (type, fn, capture) {
+      if (type === 'mousedown' && capture === true) {
+        window.__dismissListeners--;
+      }
+      return remove.apply(document, arguments);
+    };
+  });
+}
+
+// Replace the demo's own menu with one that is dismissed without the modal
+// layer and that destroys itself from its `hide` handler.
+async function setUpMenu(page) {
+  await page.evaluate(() => {
+    const $ = window.jQuery;
+    $.contextMenu('destroy');
+    $.contextMenu({
+      selector: '.context-menu-one',
+      useModal: false,
+      build: function () {
+        return {
+          items: {
+            edit: {name: 'Edit', callback: function () {}},
+            copy: {name: 'Copy', callback: function () {}}
+          }
+        };
+      },
+      events: {
+        hide: function () {
+          $('.context-menu-one').contextMenu('destroy');
+        }
+      }
+    });
+  });
+}
+
+test.describe('Issue 805: clicking on after the menu destroyed itself', () => {
+  test('does not throw and does not leak the dismiss listener', async ({ page }) => {
+    const pageErrors = [];
+    page.on('pageerror', (error) => pageErrors.push(error.message));
+
+    await countDismissListeners(page);
+    await page.goto(fixture('callback.html'));
+    await setUpMenu(page);
+
+    await page.click('.context-menu-one', { button: 'right' });
+    await expect(page.locator('.context-menu-root')).toBeVisible();
+    expect(await page.evaluate(() => window.__dismissListeners)).toBe(1);
+
+    // pick an option, which hides the menu and lets the hide handler destroy it
+    await page.locator('.context-menu-root li').first().click();
+    await expect(page.locator('.context-menu-root')).toHaveCount(0);
+
+    // now click somewhere else entirely, with either button
+    await page.mouse.click(5, 5);
+    await page.mouse.click(5, 5, { button: 'right' });
+
+    expect(pageErrors).toEqual([]);
+    expect(await page.evaluate(() => window.__dismissListeners)).toBe(0);
+  });
+
+  test('leaves the dismiss listeners bounded over repeated open/destroy cycles', async ({ page }) => {
+    const pageErrors = [];
+    page.on('pageerror', (error) => pageErrors.push(error.message));
+
+    await countDismissListeners(page);
+    await page.goto(fixture('callback.html'));
+
+    for (let i = 0; i < 3; i++) {
+      await setUpMenu(page);
+
+      await page.click('.context-menu-one', { button: 'right' });
+      await expect(page.locator('.context-menu-root')).toBeVisible();
+
+      await page.locator('.context-menu-root li').first().click();
+      await expect(page.locator('.context-menu-root')).toHaveCount(0);
+
+      await page.mouse.click(5, 5);
+
+      expect(await page.evaluate(() => window.__dismissListeners)).toBe(0);
+    }
+
+    expect(pageErrors).toEqual([]);
+  });
+});

test/unit/issue-805-layerclick-after-destroy.test.js

@@ -0,0 +1,221 @@
+// Regression test for https://github.com/swisnl/jQuery-contextMenu/issues/805
+//
+// With `useModal: false` the menu isn't dismissed by a transparent layer but
+// by a document-level mousedown listener (see op.layer()) that calls
+// handle.layerClick() and unregisters itself again through the `onhide`
+// callback it hands in.
+//
+// That listener outlives the menu it was registered for, so by the time the
+// next click arrives there may be no menu left at all: a `build` menu empties
+// its own options object once it has finished hiding, and a `hide` handler
+// calling $(selector).contextMenu('destroy') tears the menu down outright.
+// handle.layerClick() guarded that with
+//
+//     if (root.$menu === null || typeof root.$menu === 'undefined' || !root.$menu[0].contains(target))
+//
+// and then called root.$menu.trigger('contextmenu:hide') inside the block, so
+// the very case the first two clauses detect was the one that threw
+// "Cannot read properties of undefined (reading 'trigger')". Because the throw
+// happened before `onhide` ran, the listener was never removed either and one
+// more accumulated for every menu that had been opened.
+
+(function () {
+  var origAddEventListener = document.addEventListener;
+  var origRemoveEventListener = document.removeEventListener;
+  var liveListeners = [];
+
+  function trackDismissListeners() {
+    liveListeners = [];
+    document.addEventListener = function (type, fn, capture) {
+      if (type === 'mousedown' && capture === true) {
+        liveListeners.push(fn);
+      }
+      return origAddEventListener.apply(document, arguments);
+    };
+    document.removeEventListener = function (type, fn, capture) {
+      if (type === 'mousedown' && capture === true) {
+        var i = liveListeners.indexOf(fn);
+        if (i > -1) {
+          liveListeners.splice(i, 1);
+        }
+      }
+      return origRemoveEventListener.apply(document, arguments);
+    };
+  }
+
+  function stopTrackingDismissListeners() {
+    document.addEventListener = origAddEventListener;
+    document.removeEventListener = origRemoveEventListener;
+    // don't leave the plugin's listeners behind for the next test
+    while (liveListeners.length) {
+      origRemoveEventListener.call(document, 'mousedown', liveListeners.pop(), true);
+    }
+  }
+
+  // Invoke the registered listeners the way the browser would for a click
+  // somewhere outside the menu. Calling them directly (rather than dispatching
+  // the event) keeps a throw inside handle.layerClick() reportable here: a
+  // listener that throws during a real dispatch only reaches window.onerror.
+  function clickOutside(x, y) {
+    var ev = new MouseEvent('mousedown', {
+      bubbles: true,
+      cancelable: true,
+      clientX: x,
+      clientY: y,
+      button: 0
+    });
+    var error = null;
+
+    $.each(liveListeners.slice(), function (i, listener) {
+      try {
+        listener.call(document, ev);
+      } catch (e) {
+        error = e;
+      }
+    });
+
+    return error;
+  }
+
+  function menuOptions(extra) {
+    return $.extend({
+      selector: '#issue805-trigger',
+      useModal: false
+    }, extra);
+  }
+
+  function openAndPickAnItem() {
+    $('#issue805-trigger').contextMenu({x: 210, y: 210});
+    $('.context-menu-list').filter(':visible').find('li').first().trigger('mouseup');
+  }
+
+  QUnit.module('issue 805: outside click after the menu is gone', {
+    beforeEach: function () {
+      trackDismissListeners();
+      // #qunit-fixture is rendered off-screen, where document.elementFromPoint()
+      // would never find anything, so put the trigger on the body instead.
+      $('<div id="issue805-trigger" style="position:fixed;top:200px;left:200px;width:80px;height:40px;"></div>')
+        .appendTo(document.body);
+    },
+    afterEach: function () {
+      stopTrackingDismissListeners();
+      $.contextMenu('destroy');
+      $('#issue805-trigger').remove();
+      $('#context-menu-layer').remove();
+      $('.context-menu-list').remove();
+    }
+  });
+
+  QUnit.test('clicking outside after a build menu tore itself down does not throw', function (assert) {
+    var done = assert.async();
+
+    $.contextMenu(menuOptions({
+      build: function () {
+        return {items: {copy: {name: 'Copy', callback: function () {}}}};
+      }
+    }));
+
+    openAndPickAnItem();
+    assert.equal(liveListeners.length, 1, 'the dismiss listener was registered');
+
+    // wait for the hide animation to complete: that is when a built menu
+    // empties its options object, taking $menu with it
+    setTimeout(function () {
+      var error = clickOutside(10, 10);
+
+      assert.equal(error, null, 'clicking outside did not throw' + (error ? ' (' + error.message + ')' : ''));
+      assert.equal(liveListeners.length, 0, 'the dismiss listener unregistered itself');
+      done();
+    }, 200);
+  });
+
+  QUnit.test('clicking outside after a hide handler destroyed the menu does not throw', function (assert) {
+    var done = assert.async();
+
+    $.contextMenu(menuOptions({
+      build: function () {
+        return {items: {copy: {name: 'Copy', callback: function () {}}}};
+      },
+      events: {
+        hide: function () {
+          $('#issue805-trigger').contextMenu('destroy');
+        }
+      }
+    }));
+
+    openAndPickAnItem();
+
+    setTimeout(function () {
+      var error = clickOutside(10, 10);
+
+      assert.equal(error, null, 'clicking outside did not throw' + (error ? ' (' + error.message + ')' : ''));
+      assert.equal(liveListeners.length, 0, 'the dismiss listener unregistered itself');
+      done();
+    }, 200);
+  });
+
+  QUnit.test('clicking outside with an emptied $menu does not throw either', function (assert) {
+    var done = assert.async();
+    var root = null;
+
+    $.contextMenu(menuOptions({
+      items: {copy: {name: 'Copy', callback: function () {}}},
+      events: {
+        hide: function (opt) {
+          root = opt;
+        }
+      }
+    }));
+
+    openAndPickAnItem();
+
+    setTimeout(function () {
+      assert.ok(root, 'the hide handler ran');
+      // $menu can be left as an empty jQuery object rather than dropped
+      // altogether, which the old guard did not cover at all
+      root.$menu = $();
+
+      var error = clickOutside(10, 10);
+
+      assert.equal(error, null, 'clicking outside did not throw' + (error ? ' (' + error.message + ')' : ''));
+      assert.equal(liveListeners.length, 0, 'the dismiss listener unregistered itself');
+      done();
+    }, 200);
+  });
+
+  QUnit.test('the dismiss listeners do not accumulate over repeated open/hide cycles', function (assert) {
+    var done = assert.async();
+    var cycles = 3;
+
+    function cycle(remaining) {
+      if (remaining === 0) {
+        assert.equal(liveListeners.length, 0, 'no dismiss listeners are left behind');
+        return done();
+      }
+
+      // the hide handler destroys the registration, so set it up every round
+      $.contextMenu(menuOptions({
+        build: function () {
+          return {items: {copy: {name: 'Copy', callback: function () {}}}};
+        },
+        events: {
+          hide: function () {
+            $('#issue805-trigger').contextMenu('destroy');
+          }
+        }
+      }));
+
+      openAndPickAnItem();
+
+      setTimeout(function () {
+        var error = clickOutside(10, 10);
+
+        assert.equal(error, null, 'cycle ' + (cycles - remaining + 1) + ' did not throw');
+        assert.equal(liveListeners.length, 0, 'cycle ' + (cycles - remaining + 1) + ' left no dismiss listener behind');
+        cycle(remaining - 1);
+      }, 200);
+    }
+
+    cycle(cycles);
+  });
+})();