docs: make the inline SVG icon example idempotent

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)
@@ -40,6 +41,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
 

documentation/docs/customize.md

@@ -42,13 +42,26 @@ var items = {
         // or inject an inline <svg> (or <img>) 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('<svg class="context-menu-icon" ...>...</svg>');
+            // 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
+            // <svg> and the item would keep growing.
+            if (!$itemElement.children('svg.my-inline-icon').length) {
+                $itemElement.prepend('<svg class="my-inline-icon" ...>...</svg>');
+            }
             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.

documentation/docs/items.md

@@ -148,6 +148,18 @@ 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. 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)`
 

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

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) {
+    $('<div id="qunit-fixture">').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('<div class="t794a">right click me</div>');
+
+  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('<div class="t794b">right click me</div>');
+
+  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');
+});