From d9a4fadd4cf6643522694756068a0f5c2013d709 Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:22:59 +0200 Subject: [PATCH 1/3] fix: $.contextMenu('update') threw when a build menu had not been shown $.contextMenu('update') walks every registered menu and hands each one to op.update(), which immediately dereferences opt.$menu. A `build` menu only gets its $menu the first time it is actually shown, so any page that has a build menu registered (or a registration that lingered as null after a failed destroy) made a plain $.contextMenu('update') throw a TypeError. This is what the reporter hit when calling update() from an events.show handler. op.update() now bails out for a registration without a menu element, and the context-scoped branch of the 'update' operation resolves the menus registered against that context instead of passing the context element itself, which could never work since op.update() expects a menu's options object. Also document that a function-based `disabled` is re-evaluated on every open, so calling update() from events.show is not needed for that. Closes #740 --- documentation/docs/items.md | 2 + src/jquery.contextMenu.js | 27 +++-- test/unit/issue-740-update.test.js | 165 +++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 7 deletions(-) create mode 100644 test/unit/issue-740-update.test.js diff --git a/documentation/docs/items.md b/documentation/docs/items.md index 2b49cca8..39b237f4 100644 --- a/documentation/docs/items.md +++ b/documentation/docs/items.md @@ -178,6 +178,8 @@ Specifies if the command is disabled (`true`) or enabled (`false`). May be a callback returning a `boolean`. The callback is executed in the context of the triggering object (so this inside the function refers to the element the context menu was shown for). The first argument is the `key` of the command. The second argument is the `options object`. +The callback is re-evaluated every time the menu is shown, so there is no need to call `$.contextMenu('update')` from an `events.show` handler to pick up a state change. + `disabled`: `boolean` or `function(itemKey, opt)` #### Example diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 03d0b7b0..4f940e8b 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -1912,6 +1912,15 @@ }, update: function (opt, root) { var $trigger = this; + // Nothing to update for a registration whose menu element doesn't + // exist (yet): a `build` menu only gets its $menu when it is first + // shown, and a destroyed registration can linger as null. Both are + // reachable from $.contextMenu('update'), which walks every + // registered menu, so bail out instead of throwing on $menu (see + // https://github.com/swisnl/jQuery-contextMenu/issues/740). + if (!opt || !opt.$menu || !opt.$menu.length) { + return false; + } if (typeof root === 'undefined') { root = opt; op.resize(opt.$menu); @@ -2373,14 +2382,18 @@ case 'update': // Updates visibility and such - if(_hasContext){ - op.update($context); - } else { - for(var menu in menus){ - if(Object.prototype.hasOwnProperty.call(menus, menu)){ - op.update(menus[menu]); - } + for (var menu in menus) { + if (!Object.prototype.hasOwnProperty.call(menus, menu)) { + continue; + } + // when a context was given, only update the menus registered + // against it. Passing the context element itself to op.update() + // (as this used to) can never work, it expects a menu's options + // object and immediately dereferences its $menu. + if (_hasContext && (!menus[menu] || menus[menu].context !== o.context)) { + continue; } + op.update(menus[menu]); } break; diff --git a/test/unit/issue-740-update.test.js b/test/unit/issue-740-update.test.js new file mode 100644 index 00000000..aa0fa4b7 --- /dev/null +++ b/test/unit/issue-740-update.test.js @@ -0,0 +1,165 @@ +QUnit.module('issue 740 - $.contextMenu("update")', { + afterEach: function() { + $.contextMenu('destroy'); + var $fixture = $('#qunit-fixture'); + if ($fixture.length) { + $fixture.html(''); + } + } +}); + +function fixture740() { + var $fixture = $('#qunit-fixture'); + if ($fixture.length === 0) { + $('
').appendTo('body'); + $fixture = $('#qunit-fixture'); + } + return $fixture; +} + +function firstItemDisabled(selector) { + return $(selector).find('li').first().hasClass('context-menu-disabled'); +} + +QUnit.test('update() called from events.show does not throw and applies the disabled function', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var isDisabled = true; + $.contextMenu({ + selector: '.t740a', + events: { + show: function() { + $.contextMenu('update'); + return true; + } + }, + items: { + edit: {name: 'Edit', disabled: function() { return isDisabled; }}, + copy: {name: 'Copy'} + } + }); + + var err = null; + try { + $('.t740a').trigger('contextmenu'); + } catch (e) { + err = e; + } + assert.equal(err, null, 'no exception was thrown'); + assert.ok(firstItemDisabled('.context-menu-list'), 'item is disabled while the function returns true'); +}); + +QUnit.test('update() does not throw when a build menu has never been shown', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
other trigger
'); + + // a `build` menu only gets its $menu the first time it is shown - until then + // its registration has no menu element for update() to walk. + $.contextMenu({ + selector: '.t740b-build', + build: function() { + return {items: {foo: {name: 'Foo'}}}; + } + }); + + $.contextMenu({ + selector: '.t740b', + events: { + show: function() { + $.contextMenu('update'); + return true; + } + }, + items: { + edit: {name: 'Edit', disabled: function() { return true; }} + } + }); + + var err = null; + try { + $('.t740b').trigger('contextmenu'); + } catch (e) { + err = e; + } + assert.equal(err, null, 'no exception was thrown'); + assert.ok(firstItemDisabled('.context-menu-list'), 'the static menu was still updated'); +}); + +QUnit.test('update() from the events.show of a build menu does not throw', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + $.contextMenu({ + selector: '.t740c', + events: { + show: function() { + $.contextMenu('update'); + return true; + } + }, + build: function() { + return { + items: { + edit: {name: 'Edit', disabled: function() { return true; }} + } + }; + } + }); + + var err = null; + try { + $('.t740c').trigger('contextmenu'); + } catch (e) { + err = e; + } + assert.equal(err, null, 'no exception was thrown'); +}); + +QUnit.test('update() scoped to a context updates that context\'s menu', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var isDisabled = false; + $('.t740e-ctx').contextMenu({ + selector: '.t740e', + items: { + edit: {name: 'Edit', disabled: function() { return isDisabled; }} + } + }); + + $('.t740e').trigger('contextmenu'); + assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled'); + + isDisabled = true; + var err = null; + try { + $.contextMenu('update', {context: '.t740e-ctx'}); + } catch (e) { + err = e; + } + assert.equal(err, null, 'no exception was thrown'); + assert.ok(firstItemDisabled('.context-menu-list'), 'item became disabled after the scoped update'); +}); + +QUnit.test('a disabled function is re-evaluated on every open without calling update()', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var isDisabled = true; + $.contextMenu({ + selector: '.t740d', + items: { + edit: {name: 'Edit', disabled: function() { return isDisabled; }} + } + }); + + $('.t740d').trigger('contextmenu'); + assert.ok(firstItemDisabled('.context-menu-list'), 'disabled on the first open'); + + $('.context-menu-list').trigger('contextmenu:hide', {force: true}); + + isDisabled = false; + $('.t740d').trigger('contextmenu'); + assert.notOk(firstItemDisabled('.context-menu-list'), 'enabled on the second open'); +}); From 9420a739b8c67073493c865d53303c53b85d2578 Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:30:53 +0200 Subject: [PATCH 2/3] fix: make $.contextMenu('update') refresh build menus and bind `this` to the trigger Two follow-ups from review. A `build` menu is rebuilt into a fresh options object on every invocation, and only that object gets the $menu. The registration kept in `menus` never does, so $.contextMenu('update') could not refresh an open build menu at all, it just skipped it. Track the built options object per namespace and update that one when it exists. op.update() forwards its `this` to the function-based `disabled`, `visible`, `name` and `icon` options, which are documented to run against the trigger element. Calling it as op.update(...) from the 'update' operation passed the internal `op` object instead, so those callbacks saw the wrong `this`. Bind to the menu's active $trigger. --- src/jquery.contextMenu.js | 23 ++++++++++++- test/unit/issue-740-update.test.js | 53 ++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 4f940e8b..453fd6c5 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -127,6 +127,14 @@ namespaces = {}, // mapping namespace to options menus = {}, + // mapping namespace to the options object a `build` menu was actually + // built from. A build menu is rebuilt on every invocation into a fresh + // options object (see handle.contextmenu), so the registration kept in + // `menus` never carries the on-screen menu's $menu/items and can't be + // refreshed by $.contextMenu('update') on its own. Entries clean + // themselves up when the menu hides (op.hide() empties the built + // options object again). + builtMenus = {}, // registrations keyed by raw DOM element rather than a selector // string - used when `selector` is an Element or jQuery object, // since those can't be used as `namespaces` object keys the way @@ -524,6 +532,7 @@ e.data.$trigger = $currentTrigger; op.create(e.data); + builtMenus[e.data.ns] = e.data; } op.show.call($this, e.data, e.pageX, e.pageY); } @@ -2393,7 +2402,15 @@ if (_hasContext && (!menus[menu] || menus[menu].context !== o.context)) { continue; } - op.update(menus[menu]); + // for a `build` menu the registration isn't the object the + // on-screen menu was built from, so refresh the built + // instance when there is one. + var target = builtMenus[menu] || menus[menu]; + // function-based `disabled`/`visible`/`name`/`icon` options + // are documented to run against the trigger element, so bind + // `this` to it rather than leaving it as the internal `op` + // object that a plain op.update(...) call would pass along. + op.update.call((target && target.$trigger) || $(), target); } break; @@ -2548,6 +2565,7 @@ } delete menus[o.ns]; + delete builtMenus[o.ns]; } catch (e) { menus[o.ns] = null; } @@ -2572,6 +2590,7 @@ namespaces = {}; menus = {}; + builtMenus = {}; elementSelectors = []; counter = 0; initialized = false; @@ -2598,6 +2617,7 @@ } delete menus[ns]; + delete builtMenus[ns]; } catch (e) { menus[ns] = null; } @@ -2618,6 +2638,7 @@ } delete menus[namespaces[o.selector]]; + delete builtMenus[namespaces[o.selector]]; } catch (e) { menus[namespaces[o.selector]] = null; } diff --git a/test/unit/issue-740-update.test.js b/test/unit/issue-740-update.test.js index aa0fa4b7..1e2eb330 100644 --- a/test/unit/issue-740-update.test.js +++ b/test/unit/issue-740-update.test.js @@ -116,6 +116,59 @@ QUnit.test('update() from the events.show of a build menu does not throw', funct assert.equal(err, null, 'no exception was thrown'); }); +QUnit.test('update() refreshes an open build menu, not just static ones', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var isDisabled = false; + $.contextMenu({ + selector: '.t740f', + build: function() { + return { + items: { + edit: {name: 'Edit', disabled: function() { return isDisabled; }} + } + }; + } + }); + + $('.t740f').trigger('contextmenu'); + assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled'); + + isDisabled = true; + $.contextMenu('update'); + assert.ok(firstItemDisabled('.context-menu-list'), 'the on-screen build menu was refreshed'); +}); + +QUnit.test('update() runs function-based options against the trigger element', function(assert) { + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var seenThis = null; + $.contextMenu({ + selector: '.t740g', + items: { + edit: { + name: 'Edit', + disabled: function() { + seenThis = this; + return !!(this && this.hasClass && this.hasClass('lock-it')); + } + } + } + }); + + $('.t740g').trigger('contextmenu'); + assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled'); + + $('.t740g').addClass('lock-it'); + $.contextMenu('update'); + + assert.ok(seenThis && seenThis.jquery, '`this` is a jQuery object, not the internal op object'); + assert.ok(seenThis && seenThis.is('.t740g'), '`this` is the trigger element'); + assert.ok(firstItemDisabled('.context-menu-list'), 'the trigger-dependent disabled state was applied'); +}); + QUnit.test('update() scoped to a context updates that context\'s menu', function(assert) { var $fixture = fixture740(); $fixture.append('
right click me
'); From 4febe562073cc1a74e23653d8b3d93d5d6eb1acb Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 17:17:35 +0200 Subject: [PATCH 3/3] fix: bind `this` to the trigger when a promise-based sub-menu updates The promise-resolution path called op.update() as a plain function, so function-based disabled/visible/name/icon options received the internal `op` object as `this` instead of the trigger element the docs promise. Same defect the previous commit fixed for $.contextMenu('update'), one call site further along. Also corrects the builtMenus comment: entries are removed on destroy, not on hide. --- src/jquery.contextMenu.js | 14 ++++++++--- test/unit/issue-740-update.test.js | 40 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 453fd6c5..61e2703f 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -131,9 +131,10 @@ // built from. A build menu is rebuilt on every invocation into a fresh // options object (see handle.contextmenu), so the registration kept in // `menus` never carries the on-screen menu's $menu/items and can't be - // refreshed by $.contextMenu('update') on its own. Entries clean - // themselves up when the menu hides (op.hide() empties the built - // options object again). + // refreshed by $.contextMenu('update') on its own. Entries are removed + // along with their `menus` entry on destroy; a hidden build menu keeps + // its entry, but op.hide() has emptied the options object by then so + // op.update() below skips it on the $menu guard. builtMenus = {}, // registrations keyed by raw DOM element rather than a selector // string - used when `selector` is an Element or jQuery object, @@ -2124,7 +2125,12 @@ opt.$menu.data('_scrollTopAtShow', root.$menu.scrollTop()); } } - op.update(opt, root); // Correctly update position if user is already hovered over menu item + // bind `this` to the trigger, same as every other op.update() + // call site: function-based disabled/visible/name/icon options + // are documented to run against the trigger element, and a + // plain op.update(...) call would hand them the internal `op` + // object instead. + op.update.call(root.$trigger || $(), opt, root); // Correctly update position if user is already hovered over menu item root.positionSubmenu.call(opt.$node, opt.$menu); // positionSubmenu, will only do anything if user already hovered over menu item that just got new subitems. } diff --git a/test/unit/issue-740-update.test.js b/test/unit/issue-740-update.test.js index 1e2eb330..0bfd39e3 100644 --- a/test/unit/issue-740-update.test.js +++ b/test/unit/issue-740-update.test.js @@ -216,3 +216,43 @@ QUnit.test('a disabled function is re-evaluated on every open without calling up $('.t740d').trigger('contextmenu'); assert.notOk(firstItemDisabled('.context-menu-list'), 'enabled on the second open'); }); + +QUnit.test('a sub-menu resolving from a promise runs function-based options against the trigger', function(assert) { + var done = assert.async(); + var $fixture = fixture740(); + $fixture.append('
right click me
'); + + var seenThis = null; + var deferred = $.Deferred(); + + $.contextMenu({ + selector: '.t740h', + items: { + more: { + name: 'More', + items: deferred.promise() + } + } + }); + + $('.t740h').trigger('contextmenu'); + + // resolving the promise runs op.update() for the freshly built sub-menu, + // which must bind `this` to the trigger like every other update path + deferred.resolve({ + sub: { + name: 'Sub', + disabled: function() { + seenThis = this; + return false; + } + } + }); + + setTimeout(function() { + assert.ok(seenThis, 'the sub-menu item\'s disabled function ran'); + assert.ok(seenThis && seenThis.jquery, '`this` is a jQuery object, not the internal op object'); + assert.ok(seenThis && seenThis.jquery && seenThis.is('.t740h'), '`this` is the trigger element'); + done(); + }, 50); +});