diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e8a7b48..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) @@ -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 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..f87783d6 100644 --- a/documentation/docs/items.md +++ b/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)` 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'); +});