From 664c29516193125afc1416b05603baf54674fbcc Mon Sep 17 00:00:00 2001 From: bjorn Date: Thu, 30 Jul 2026 09:17:54 +0200 Subject: [PATCH 1/2] docs: make the inline SVG icon example idempotent The example added in #794 prepended an unconditionally: icon: function (opt, $itemElement) { $itemElement.prepend('...'); return 'context-menu-icon-inline'; } A callback `icon` is invoked twice before the menu is ever visible - once from op.create() and again from the op.update() that op.show() runs on every show - and once more on every subsequent open or update. The prepend has no guard, so the item already carries two icons on its first display and gains another with every open. The returned 'context-menu-icon-inline' class reads as a done-marker, but nothing ever checked it. Guard on the injected element instead, which is what makes the callback safe to re-run. Two related doc fixes: * The example put `context-menu-icon` on the child . That is the plugin's own item-level class (classNames.icon), which carries an absolutely positioned ::before from the base-context-menu-icon mixin. Use a class of your own instead. * items.md never said the callback re-runs, which is what allowed the example to be written this way. Say so, and point at the guard pattern. The canonical example there needs no guard because $itemElement.html() replaces content rather than adding to it. Reported by Codex review on #794, after the PR had already been merged. --- CHANGELOG.md | 1 + documentation/docs/customize.md | 15 ++++++++++++++- documentation/docs/items.md | 7 +++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e8a7b48..815cef42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ * Added a dynamic per-row title example to the menu-title demo (fixes #769) * The asynchronous create demo now works on right click (fixes #735) * Documented that `$(...).contextMenu({x, y})` takes page coordinates (fixes #812) +* Made the inline SVG icon example idempotent and documented that a callback `icon` re-runs on every show/update ### 2.10.2 diff --git a/documentation/docs/customize.md b/documentation/docs/customize.md index 0a063c37..68a2ca7e 100644 --- a/documentation/docs/customize.md +++ b/documentation/docs/customize.md @@ -42,13 +42,26 @@ var items = { // or inject an inline (or ) directly into the item and // return a class name to mark it as done (see the icon option docs) icon: function (opt, $itemElement) { - $itemElement.prepend('...'); + // A callback icon runs again every time the menu is shown or + // updated, not just once, so anything that adds to the item has to + // check first. Without the guard every open would prepend another + // and the item would keep growing. + if (!$itemElement.children('svg.my-inline-icon').length) { + $itemElement.prepend('...'); + } return 'context-menu-icon-inline'; } } } ``` +Give the injected element a class of your own rather than `context-menu-icon`, +which this plugin already uses on the menu item itself. + +Anything that replaces the item's content instead of adding to it is idempotent +on its own and needs no guard, which is why the +[icon option](items#icon) example can call `$itemElement.html(...)` directly. + ## Customize CSS You can use the _variables.scss to adjust variables on pretty much everything you want to change. diff --git a/documentation/docs/items.md b/documentation/docs/items.md index 7873e426..3dc8febf 100644 --- a/documentation/docs/items.md +++ b/documentation/docs/items.md @@ -148,6 +148,13 @@ Specifies the icon class to set for the item. When using a string icons must be defined in CSS with selectors like `.context-menu-item.context-menu-icon-edit`, where `edit` is the icon class specified. When using a callback you can return a class string to use that as the class on the item. You can also modify the element by using the `$itemElement` argument. + +The callback is invoked every time the menu is shown or updated, not only when +it is first built, so that the icon can reflect current state. Write it to be +idempotent: replacing the item's content (as in the example below) is safe, +while adding to it needs a guard so repeated opens do not stack up duplicates. +See [using your own SVG icons](customize#using-your-own-svg-icons-without-a-build-step) +for that pattern. `icon`: `string` or `function(opt, $itemElement, itemKey, item)` From ea2366a8f324980a6ab4d0945379960f60a00577 Mon Sep 17 00:00:00 2001 From: bjorn Date: Thu, 30 Jul 2026 09:43:20 +0200 Subject: [PATCH 2/2] fix: replace the previous class when a callback icon is re-evaluated Addresses the Codex review point on the docs change in this PR: the new guidance said a callback `icon` re-runs "so that the icon can reflect current state", but a callback whose class tracks that state did not actually work. op.update() removed `item._icon` before re-invoking the callback, then applied the result without ever storing it. `item._icon` therefore stayed at whatever op.create() had produced, so only that very first class was ever removed and every class returned after it stayed on the item: create -> _icon='state-one', item has state-one update -> removeClass('state-one'), addClass('state-two') _icon unchanged update -> removeClass('state-one') no-op, addClass('state-three') item now carries state-two AND state-three Assign the result back to `item._icon` so the next update removes the class the previous one applied. Rather than documenting the limitation, which would enshrine it, this makes the documented contract true. The alternative was to soften the docs and leave a callback `icon` unable to express changing state at all, which is the whole reason it is re-evaluated. Backwards compatibility: a callback returning a constant string - the common case by far - is unaffected, since removing and re-adding the same class is a no-op either way. Only a callback returning *different* strings changes, and only by dropping stale classes, which is the bug. A callback returning an element rather than a string is unaffected: `removeClass()` ignores a non-string argument, exactly as it did before. `item._icon` is internal and read only in op.create() and op.update(). Also documents that a returned class is swapped on each call, while anything the callback does to $itemElement itself is not, so only that part needs to be idempotent. --- CHANGELOG.md | 1 + documentation/docs/items.md | 13 ++- src/jquery.contextMenu.js | 14 ++- .../issue-794-icon-callback-classes.test.js | 108 ++++++++++++++++++ 4 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 test/unit/issue-794-icon-callback-classes.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 815cef42..79a9b251 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * Upgraded the embedded jQuery UI Position to 1.13.2 (CVE-2021-41184) (fixes #765) * Caller-supplied selector strings are no longer evaluated as HTML (fixes #731) * `item.icon` is no longer interpolated into markup on the Font Awesome paths (fixes #810) +* A callback `item.icon` returning a changing class no longer leaves every class it has ever returned on the item * A left-click trigger no longer leaks a synthetic `contextmenu` event to unrelated ancestor listeners (fixes #754) * `$.contextMenu('update')` no longer throws when a `build` menu has not been shown yet (fixes #740) * `autoHide` now works for a nested trigger registered with a different trigger mode (fixes #727) diff --git a/documentation/docs/items.md b/documentation/docs/items.md index 3dc8febf..f87783d6 100644 --- a/documentation/docs/items.md +++ b/documentation/docs/items.md @@ -150,10 +150,15 @@ When using a string icons must be defined in CSS with selectors like `.context-m When using a callback you can return a class string to use that as the class on the item. You can also modify the element by using the `$itemElement` argument. The callback is invoked every time the menu is shown or updated, not only when -it is first built, so that the icon can reflect current state. Write it to be -idempotent: replacing the item's content (as in the example below) is safe, -while adding to it needs a guard so repeated opens do not stack up duplicates. -See [using your own SVG icons](customize#using-your-own-svg-icons-without-a-build-step) +it is first built, so that the icon can reflect current state. A returned class +string may therefore change between calls, and the previously returned one is +removed from the item before the new one is applied. + +Anything the callback does to `$itemElement` itself is not undone that way, so +write that part to be idempotent: replacing the item's content (as in the +example below) is safe, while adding to it needs a guard so repeated opens do +not stack up duplicates. See +[using your own SVG icons](customize#using-your-own-svg-icons-without-a-build-step) for that pattern. `icon`: `string` or `function(opt, $itemElement, itemKey, item)` diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index ceace1d5..4db728ac 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -2191,12 +2191,18 @@ $item[disabled ? 'addClass' : 'removeClass'](root.classNames.disabled); if (typeof item.icon === 'function') { + // Store what the callback returned, so the *next* update + // removes this class rather than the creation-time one. + // Without that, a callback whose class tracks changing + // state left every class it had ever returned on the + // item: `item._icon` stayed at its op.create() value, so + // only that first class was ever removed. $item.removeClass(item._icon); - var iconResult = item.icon.call(this, $trigger, $item, key, item); - if(typeof(iconResult) === "string"){ - $item.addClass(iconResult); + item._icon = item.icon.call(this, $trigger, $item, key, item); + if(typeof(item._icon) === "string"){ + $item.addClass(item._icon); } else { - $item.prepend(iconResult); + $item.prepend(item._icon); } } diff --git a/test/unit/issue-794-icon-callback-classes.test.js b/test/unit/issue-794-icon-callback-classes.test.js new file mode 100644 index 00000000..097ee449 --- /dev/null +++ b/test/unit/issue-794-icon-callback-classes.test.js @@ -0,0 +1,108 @@ +// A function-based `item.icon` is invoked on every show/update, not only on +// create, so that the icon can track current state. op.update() removes the +// previous class before re-invoking it, but it used to remove `item._icon` -- +// the *creation-time* result -- without ever storing the new one. A callback +// returning a different class as state changed therefore left every class it +// had ever returned on the item. +// +// See https://github.com/swisnl/jQuery-contextMenu/pull/794#pullrequestreview-4816334449 + +QUnit.module('issue 794 - callback icon classes across updates', { + afterEach: function() { + $.contextMenu('destroy'); + var $fixture = $('#qunit-fixture'); + if ($fixture.length) { + $fixture.html(''); + } + } +}); + +function fixture794() { + var $fixture = $('#qunit-fixture'); + if ($fixture.length === 0) { + $('
').appendTo('body'); + $fixture = $('#qunit-fixture'); + } + return $fixture; +} + +function firstItem794() { + return $('.context-menu-list').first().find('li.context-menu-item').first(); +} + +QUnit.test('a callback returning a changing class leaves only the current one on the item', function(assert) { + var $fixture = fixture794(); + $fixture.append('
right click me
'); + + var state = 'one'; + $.contextMenu({ + selector: '.t794a', + items: { + first: { + name: 'First', + icon: function() { + return 'state-' + state; + } + } + } + }); + + $('.t794a').contextMenu(); + var $item = firstItem794(); + assert.ok($item.hasClass('state-one'), 'the first state\'s class is applied'); + + state = 'two'; + $.contextMenu('update'); + $item = firstItem794(); + assert.ok($item.hasClass('state-two'), 'the new state\'s class is applied'); + assert.notOk($item.hasClass('state-one'), 'the previous state\'s class is removed'); + + state = 'three'; + $.contextMenu('update'); + $item = firstItem794(); + assert.ok($item.hasClass('state-three'), 'the third state\'s class is applied'); + assert.notOk($item.hasClass('state-two'), 'the second state\'s class is removed'); + assert.notOk($item.hasClass('state-one'), 'the first state\'s class is still gone'); + + var stateClasses = ($item.attr('class') || '').split(/\s+/).filter(function(cls) { + return cls.indexOf('state-') === 0; + }); + assert.deepEqual(stateClasses, ['state-three'], 'exactly one state class remains after three updates'); +}); + +// The overwhelmingly common case: a callback that always returns the same +// class. This behaved correctly before and must keep behaving identically, so +// it is pinned here rather than left to inference. +QUnit.test('a callback returning a constant class keeps that class across updates', function(assert) { + var $fixture = fixture794(); + $fixture.append('
right click me
'); + + var calls = 0; + $.contextMenu({ + selector: '.t794b', + items: { + first: { + name: 'First', + icon: function() { + calls++; + return 'constant-icon'; + } + } + } + }); + + $('.t794b').contextMenu(); + assert.ok(firstItem794().hasClass('constant-icon'), 'the class is applied on show'); + + $.contextMenu('update'); + $.contextMenu('update'); + + var $item = firstItem794(); + assert.ok($item.hasClass('constant-icon'), 'the class survives repeated updates'); + assert.ok(calls >= 2, 'the callback really was re-invoked (' + calls + ' calls)'); + + var iconClasses = ($item.attr('class') || '').split(/\s+/).filter(function(cls) { + return cls === 'constant-icon'; + }); + assert.deepEqual(iconClasses, ['constant-icon'], 'the class is not duplicated'); +});