From fa7189c1bd455321b284928aa27a2894426d1ed2 Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:43:36 +0200 Subject: [PATCH 1/2] fix: autoHide never applied to a nested trigger with a different trigger mode Registering two menus on nested elements with different trigger modes, e.g. `trigger: 'left'` on a button inside a row registered with `trigger: 'right'`, left the inner menu unusable once the outer one had been opened, and autoHide never kicked in for it. Three things went wrong, all in the hand-off between the menu that is closing and the one that should open: - handle.layerClick() decided whether the click that dismissed the open menu should also open another one from the *closing* menu's own `trigger` option. A left-click while a right-click menu was open therefore never re-dispatched, so the nested trigger's menu simply never showed. The button is now matched against the triggers that are actually registered for the element under the cursor, and the menu asked to show still validates the button against its own `trigger` option as before. - Resolving what the click landed on required `root.$layer` to still be there 50ms later. autoHide's own timer routinely closes the menu inside that window, which left the lingering layer element itself reported as the click target. - op.hide() unbound the document-level keyboard and autoHide handlers unconditionally, so a menu that had just been shown while the previous one was still tearing down lost its handlers again and never auto hid. The same applied to `$currentTrigger`. Both are now only cleared when they still belong to the menu being hidden. Closes #727 --- documentation/_data/nav.js | 1 + documentation/demo.md | 1 + .../demo/nested-triggers-autohide.md | 65 ++++++++++ src/jquery.contextMenu.js | 118 +++++++++++++++--- test/specs/nested-triggers-autohide.js | 91 ++++++++++++++ 5 files changed, 256 insertions(+), 20 deletions(-) create mode 100644 documentation/demo/nested-triggers-autohide.md create mode 100644 test/specs/nested-triggers-autohide.js diff --git a/documentation/_data/nav.js b/documentation/_data/nav.js index 4e78373e..2081195f 100644 --- a/documentation/_data/nav.js +++ b/documentation/_data/nav.js @@ -54,6 +54,7 @@ module.exports = [ { id: 'menu-title-fontawesome', text: 'Menu title with Font Awesome icons', url: '/demo/menu-title-fontawesome.html' }, { id: 'menu-promise', text: 'Menu with promise', url: '/demo/menu-promise.html' }, { id: 'on-dom-element', text: 'Context Menu on DOM Element', url: '/demo/on-dom-element.html' }, + { id: 'nested-triggers-autohide', text: 'Nested Triggers With Autohide', url: '/demo/nested-triggers-autohide.html' }, { id: 'sub-menus', text: 'Submenus', url: '/demo/sub-menus.html' }, { id: 'sub-menus-promise', text: 'Submenus (promise)', url: '/demo/sub-menus-promise.html' }, { id: 'trigger-custom', text: 'Custom Activated Context Menu', url: '/demo/trigger-custom.html' }, diff --git a/documentation/demo.md b/documentation/demo.md index 3973d90e..deda1118 100644 --- a/documentation/demo.md +++ b/documentation/demo.md @@ -63,6 +63,7 @@ title: jQuery contextMenu — Demo gallery * [Swipe Trigger](demo/trigger-swipe.html) * [Hover Activated Context Menu](demo/trigger-hover.html) * [Hover Activated Context Menu With Autohide](demo/trigger-hover-autohide.html) +* [Nested Triggers With Autohide](demo/nested-triggers-autohide.html) * [Custom Activated Context Menu](demo/trigger-custom.html) * [Disabled Menu](demo/disabled-menu.html) * [Disabled Command](demo/disabled.html) diff --git a/documentation/demo/nested-triggers-autohide.md b/documentation/demo/nested-triggers-autohide.md new file mode 100644 index 00000000..c950af29 --- /dev/null +++ b/documentation/demo/nested-triggers-autohide.md @@ -0,0 +1,65 @@ +--- +currentMenu: nested-triggers-autohide +--- + +# Demo: Nested Triggers With Autohide + + + + + +- [Example code](#example-code) +- [Example HTML](#example-html) + + + + + left click me! +
+ right click anywhere in this box + + +
+ +## Example code + + + +## Example HTML + diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 707ad57e..fa505c9e 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -117,6 +117,9 @@ var // currently active contextMenu trigger $currentTrigger = null, + // options object of the menu that currently owns the document-level + // keyboard/autoHide handlers, see op.show() and op.hide() + documentHandlerOwner = null, // is contextMenu initialized with at least one menu? initialized = false, // window handle @@ -440,6 +443,41 @@ pageX: null, pageY: null }, + // Does a click with this mouse button, on this element, activate any + // registered menu other than the one that is currently open? Used by + // handle.layerClick(), which has to decide whether the click that just + // dismissed the open menu should also open another one. It used to + // decide that from the *open* menu's own `trigger` option, which + // silently swallowed the click whenever the element under the cursor + // was registered with a different trigger - e.g. a `trigger: 'left'` + // element nested inside a `trigger: 'right'` element, where the outer + // menu is open. See https://github.com/swisnl/jQuery-contextMenu/issues/727 + // Note this only decides whether to re-dispatch at all; the menu that + // gets asked to show still validates the button against its own + // `trigger` option (see handle.contextmenu). + activatesOtherTrigger = function (target, button) { + var wanted = button === 0 ? 'left' : (button === 2 ? 'right' : null), + activates = false; + + if (!target || !wanted) { + return false; + } + + $.each(menus, function (ns, o) { + if (!o || o.trigger !== wanted || !o.selector) { + return true; + } + + if ($(target).closest(o.selector).length) { + activates = true; + return false; + } + + return true; + }); + + return activates; + }, // determine zIndex zindex = function ($t) { var zin = 0, @@ -721,8 +759,15 @@ var triggerAction = ((root.trigger === 'left' && button === 0) || (root.trigger === 'right' && button === 2)); // find the element that would've been clicked, wasn't the layer in the way - if (document.elementFromPoint && root.$layer) { - root.$layer.hide(); + // Something else - autoHide's own timer, most likely - may have closed + // the menu during the delay above, in which case root.$layer is already + // gone even though the layer element itself lingers in the document for + // another few milliseconds. Look the layer up by id in that case, so we + // still resolve what the click actually landed on instead of reporting + // the layer itself. See https://github.com/swisnl/jQuery-contextMenu/issues/727 + if (document.elementFromPoint) { + var $blockingLayer = root.$layer && root.$layer.length ? root.$layer : $('#context-menu-layer'); + $blockingLayer.hide(); target = document.elementFromPoint(x - $win.scrollLeft(), y - $win.scrollTop()); // also need to try and focus this element if we're in a contenteditable area, @@ -745,7 +790,7 @@ e.target = target; } $(target).trigger(e); - root.$layer.show(); + $blockingLayer.show(); } if (root.hideOnSecondTrigger && triggerAction && root.$menu !== null && typeof root.$menu !== 'undefined') { @@ -783,20 +828,32 @@ } var openTargetMenu; - if (target && triggerAction) { - openTargetMenu = function () { + // Re-open on whatever the click landed on, once the current + // menu is out of the way. `triggerAction` describes the menu + // that is closing, so it can't be the whole answer here: + // clicking a trigger registered with a *different* trigger + // option (a left-click trigger nested inside a right-click + // one, say) is a perfectly valid activation for that other + // menu. See https://github.com/swisnl/jQuery-contextMenu/issues/727 + if (target && (triggerAction || activatesOtherTrigger(target, button))) { + var showTarget = function () { $(target).contextMenu({x: x, y: y, button: button}); }; - // Re-opening the very same menu on another trigger normally - // waits for the hide animation to finish before showing it - // again, which is what makes the menu collapse and expand - // again. With animation.animateOnReopen disabled, show it - // right away instead, so op.show() just moves the menu that - // is still on screen (see #739). - if (!reopensSameMenuWithoutAnimation(root, target)) { - root.$trigger.one('contextmenu:hidden', openTargetMenu); - openTargetMenu = null; + // Normally we have to wait for the open menu to finish hiding + // before showing the next one. When it is already gone - it may + // have been closed by autoHide while this click was pending - + // 'contextmenu:hidden' will never fire again, so show right away + // instead of waiting for an event that isn't coming. + // Nor when the very same menu is being re-opened with + // animation.animateOnReopen disabled (see #739): there is + // deliberately no hide animation to wait for in that case, + // op.show() just moves the menu that is still on screen. + if (root.$trigger && root.$trigger.hasClass('context-menu-active') + && !reopensSameMenuWithoutAnimation(root, target)) { + root.$trigger.one('contextmenu:hidden', showTarget); + } else { + openTargetMenu = showTarget; } } @@ -810,10 +867,14 @@ // select's own horizontal span. if (root !== null && typeof root !== 'undefined' && root.$menu !== null && typeof root.$menu !== 'undefined' && !isNearRecentSelectChange(root, x, y)) { root.$menu.trigger('contextmenu:hide'); + } - if (openTargetMenu) { - openTargetMenu(); - } + // Show the next menu after the current one has been told to + // hide. Outside the block above on purpose: when the menu was + // already gone there is nothing to hide, but the click still + // has to open what it landed on (see #727). + if (openTargetMenu) { + openTargetMenu(); } }, 50); }, @@ -1426,6 +1487,10 @@ .data('contextMenu', opt) .addClass('context-menu-active'); + // this menu now owns the document-level handlers registered below, + // so a *previous* menu still finishing its teardown knows not to + // unbind them again. See op.hide(). + documentHandlerOwner = opt; // register key handler $(document).off('keydown.contextMenu').on('keydown.contextMenu', handle.key); // register autoHide handler @@ -1480,8 +1545,11 @@ } } - // remove handle - $currentTrigger = null; + // remove handle, unless another menu has already taken over as the + // current one - see the note further down about overlapping hide/show + if (!$currentTrigger || !$currentTrigger.length || $currentTrigger.is($trigger)) { + $currentTrigger = null; + } // remove selected opt.$menu.find('.' + opt.classNames.hover).trigger('contextmenu:blur'); opt.$selected = null; @@ -1497,7 +1565,15 @@ } // unregister key and mouse handlers // $(document).off('.contextMenuAutoHide keydown.contextMenu'); // http://bugs.jquery.com/ticket/10705 - $(document).off('.contextMenuAutoHide').off('keydown.contextMenu'); + // Only when they still belong to this menu: hiding one menu can overlap + // with showing another (clicking a nested trigger registered with a + // different `trigger` option, say), and blindly unbinding here would + // strip the freshly opened menu of its keyboard and autoHide handlers. + // See https://github.com/swisnl/jQuery-contextMenu/issues/727 + if (documentHandlerOwner === opt) { + documentHandlerOwner = null; + $(document).off('.contextMenuAutoHide').off('keydown.contextMenu'); + } // hide menu if (opt.$menu) { opt.$menu[opt.animation.hide](animationDuration(opt.animation, 'hideDuration'), function () { @@ -2768,6 +2844,8 @@ elementSelectors = []; counter = 0; initialized = false; + // the document-level handlers just went away with everything else + documentHandlerOwner = null; $('#context-menu-layer, .context-menu-list').remove(); } else if (useElementSelector) { diff --git a/test/specs/nested-triggers-autohide.js b/test/specs/nested-triggers-autohide.js new file mode 100644 index 00000000..99062532 --- /dev/null +++ b/test/specs/nested-triggers-autohide.js @@ -0,0 +1,91 @@ +const { test, expect } = require('@playwright/test'); +const { fixture } = require('../support/helpers'); + +// While a menu is open the transparent modal layer covers the whole viewport, +// so Playwright's actionability checks would refuse to click a page element. +// Drive the raw mouse instead, exactly like a user would. +async function clickAt(page, x, y, button) { + await page.mouse.move(x, y); + await page.mouse.down({ button: button || 'left' }); + await page.mouse.up({ button: button || 'left' }); +} + +// Move the pointer well away from both triggers and from the menu itself, in +// a couple of steps so the plugin's document-level mousemove handler runs. +async function moveAway(page) { + await page.mouse.move(1150, 650); + await page.mouse.move(1160, 660); +} + +// The inner trigger sits in the top left of the outer one. Clicking the outer +// near its bottom left keeps both the click and the resulting menu clear of +// the inner trigger and of any menu opened from it. +async function points(page) { + const outer = await page.locator('.context-menu-two').boundingBox(); + const inner = await page.locator('.context-menu-one').boundingBox(); + + return { + inner: [inner.x + inner.width / 2, inner.y + inner.height / 2], + outer: [outer.x + 20, outer.y + outer.height - 20], + }; +} + +test.describe('Test autoHide with nested triggers', () => { + test.beforeEach(async ({ page }) => { + await page.goto(fixture('nested-triggers-autohide.html')); + }); + + test('Ensure the inner (left-click) menu auto hides', async ({ page }) => { + const at = await points(page); + + await clickAt(page, at.inner[0], at.inner[1]); + await expect(page.locator('.menu-one')).toBeVisible(); + + await moveAway(page); + + await expect(page.locator('.menu-one')).toBeHidden(); + }); + + test('Ensure the outer (right-click) menu auto hides', async ({ page }) => { + const at = await points(page); + + await clickAt(page, at.outer[0], at.outer[1], 'right'); + await expect(page.locator('.menu-two')).toBeVisible(); + + await moveAway(page); + + await expect(page.locator('.menu-two')).toBeHidden(); + }); + + test('Ensure the inner menu opens and auto hides while the outer menu is open', async ({ page }) => { + const at = await points(page); + + await clickAt(page, at.outer[0], at.outer[1], 'right'); + await expect(page.locator('.menu-two')).toBeVisible(); + + await clickAt(page, at.inner[0], at.inner[1]); + await expect(page.locator('.menu-one')).toBeVisible(); + await expect(page.locator('.menu-two')).toBeHidden(); + + await moveAway(page); + + await expect(page.locator('.menu-one')).toBeHidden(); + await expect(page.locator('#context-menu-layer')).toHaveCount(0); + }); + + test('Ensure the outer menu opens and auto hides while the inner menu is open', async ({ page }) => { + const at = await points(page); + + await clickAt(page, at.inner[0], at.inner[1]); + await expect(page.locator('.menu-one')).toBeVisible(); + + await clickAt(page, at.outer[0], at.outer[1], 'right'); + await expect(page.locator('.menu-two')).toBeVisible(); + await expect(page.locator('.menu-one')).toBeHidden(); + + await moveAway(page); + + await expect(page.locator('.menu-two')).toBeHidden(); + await expect(page.locator('#context-menu-layer')).toHaveCount(0); + }); +}); From e6d0377c39f4bd94d1960e3faede8db2351a364d Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:52:16 +0200 Subject: [PATCH 2/2] fix: dispatch the nested trigger's menu directly instead of via a bubbling event Re-opening through $.fn.contextMenu's coordinate overload triggers a bubbling synthetic 'contextmenu' event, which an application listener on an ancestor of the trigger cannot tell apart from a real right-click. On a plain left-click on a nested left-trigger that is exactly the leak #754 removed from handle.click. The trigger lookup now returns the matched element and its options, so the menu can be dispatched straight to handle.contextmenu the way handle.click does. It resolves the innermost match, mirroring which delegated handler a bubbling event would have reached first, and skips selector-string registrations whose context does not contain the element. Covered by an added assertion that a jQuery 'contextmenu' listener on an ancestor stays silent while the nested menu opens on left-click. --- src/jquery.contextMenu.js | 71 ++++++++++++++++++-------- test/specs/nested-triggers-autohide.js | 10 ++++ 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index fa505c9e..5f77c0d9 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -443,24 +443,23 @@ pageX: null, pageY: null }, - // Does a click with this mouse button, on this element, activate any - // registered menu other than the one that is currently open? Used by - // handle.layerClick(), which has to decide whether the click that just - // dismissed the open menu should also open another one. It used to - // decide that from the *open* menu's own `trigger` option, which - // silently swallowed the click whenever the element under the cursor - // was registered with a different trigger - e.g. a `trigger: 'left'` - // element nested inside a `trigger: 'right'` element, where the outer - // menu is open. See https://github.com/swisnl/jQuery-contextMenu/issues/727 - // Note this only decides whether to re-dispatch at all; the menu that - // gets asked to show still validates the button against its own - // `trigger` option (see handle.contextmenu). - activatesOtherTrigger = function (target, button) { + // Which registered menu, if any, does a click with this mouse button on + // this element activate? Returns {el, options} for the innermost match, + // mirroring how a bubbling event reaches the closest delegated handler + // first. Used by handle.layerClick(), which has to decide whether the + // click that just dismissed the open menu should also open another one. + // It used to decide that from the *open* menu's own `trigger` option, + // which silently swallowed the click whenever the element under the + // cursor was registered with a different trigger - e.g. a + // `trigger: 'left'` element nested inside a `trigger: 'right'` element, + // where the outer menu is open. + // See https://github.com/swisnl/jQuery-contextMenu/issues/727 + matchTriggerForClick = function (target, button) { var wanted = button === 0 ? 'left' : (button === 2 ? 'right' : null), - activates = false; + match = null; if (!target || !wanted) { - return false; + return null; } $.each(menus, function (ns, o) { @@ -468,15 +467,27 @@ return true; } - if ($(target).closest(o.selector).length) { - activates = true; - return false; + var el = $(target).closest(o.selector)[0]; + if (!el) { + return true; + } + + // Selector-string registrations are delegated from o.context, so + // they only apply to elements inside it. Element/jQuery-object + // selectors are bound to the element(s) directly and aren't + // scoped that way (see the 'create' operation). + if (typeof o.selector === 'string' && o.context && o.context !== el && !$.contains(o.context, el)) { + return true; + } + + if (!match || $.contains(match.el, el)) { + match = {el: el, options: o}; } return true; }); - return activates; + return match; }, // determine zIndex zindex = function ($t) { @@ -835,8 +846,28 @@ // option (a left-click trigger nested inside a right-click // one, say) is a perfectly valid activation for that other // menu. See https://github.com/swisnl/jQuery-contextMenu/issues/727 - if (target && (triggerAction || activatesOtherTrigger(target, button))) { + var match = matchTriggerForClick(target, button); + if (target && (triggerAction || match)) { var showTarget = function () { + if (match) { + // Dispatch straight to the menu we matched, the way + // handle.click does. Going through $.fn.contextMenu + // would trigger a *bubbling* synthetic 'contextmenu' + // event instead, which any application listener on an + // ancestor cannot tell apart from a real right-click - + // the leak fixed in + // https://github.com/swisnl/jQuery-contextMenu/issues/754 + handle.contextmenu.call(match.el, $.Event('contextmenu', { + data: match.options, + pageX: x, + pageY: y, + mouseButton: button, + target: match.el, + currentTarget: match.el + })); + return; + } + $(target).contextMenu({x: x, y: y, button: button}); }; diff --git a/test/specs/nested-triggers-autohide.js b/test/specs/nested-triggers-autohide.js index 99062532..675dd327 100644 --- a/test/specs/nested-triggers-autohide.js +++ b/test/specs/nested-triggers-autohide.js @@ -63,9 +63,19 @@ test.describe('Test autoHide with nested triggers', () => { await clickAt(page, at.outer[0], at.outer[1], 'right'); await expect(page.locator('.menu-two')).toBeVisible(); + // A plain left-click must not reach an application's own 'contextmenu' + // listener on an ancestor of the trigger, see issue #754. + await page.evaluate(() => { + window.ancestorContextMenuEvents = 0; + window.$(document.body).on('contextmenu', () => { + window.ancestorContextMenuEvents++; + }); + }); + await clickAt(page, at.inner[0], at.inner[1]); await expect(page.locator('.menu-one')).toBeVisible(); await expect(page.locator('.menu-two')).toBeHidden(); + expect(await page.evaluate(() => window.ancestorContextMenuEvents)).toBe(0); await moveAway(page);