fix: $.contextMenu('update') threw when a build menu had not been shown

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

src/jquery.contextMenu.js

@@ -127,6 +127,15 @@
         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 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,
         // since those can't be used as `namespaces` object keys the way
@@ -524,6 +533,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);
                 }
@@ -1912,6 +1922,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);
@@ -2106,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.
                 }
 
@@ -2373,14 +2397,26 @@
 
             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;
                     }
+                    // 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;
 
@@ -2535,6 +2571,7 @@
                             }
 
                             delete menus[o.ns];
+                            delete builtMenus[o.ns];
                         } catch (e) {
                             menus[o.ns] = null;
                         }
@@ -2559,6 +2596,7 @@
 
                     namespaces = {};
                     menus = {};
+                    builtMenus = {};
                     elementSelectors = [];
                     counter = 0;
                     initialized = false;
@@ -2585,6 +2623,7 @@
                                 }
 
                                 delete menus[ns];
+                                delete builtMenus[ns];
                             } catch (e) {
                                 menus[ns] = null;
                             }
@@ -2605,6 +2644,7 @@
                         }
 
                         delete menus[namespaces[o.selector]];
+                        delete builtMenus[namespaces[o.selector]];
                     } catch (e) {
                         menus[namespaces[o.selector]] = null;
                     }

test/unit/issue-740-update.test.js

@@ -0,0 +1,258 @@
+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) {
+    $('<div id="qunit-fixture">').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('<div class="t740a">right click me</div>');
+
+  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('<div class="t740b">right click me</div><div class="t740b-build">other trigger</div>');
+
+  // 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('<div class="t740c">right click me</div>');
+
+  $.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() refreshes an open build menu, not just static ones', function(assert) {
+  var $fixture = fixture740();
+  $fixture.append('<div class="t740f">right click me</div>');
+
+  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('<div class="t740g">right click me</div>');
+
+  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('<div class="t740e-ctx"><div class="t740e">right click me</div></div>');
+
+  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('<div class="t740d">right click me</div>');
+
+  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');
+});
+
+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('<div class="t740h">right click me</div>');
+
+  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);
+});