Skip to content

Commit b01bfff

Browse files
bbralabbrala
authored
fix: $.contextMenu('update') threw when a build menu had not been shown (#800)
* 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 * 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. * 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.
1 parent 1d1cec6 commit b01bfff

3 files changed

Lines changed: 308 additions & 8 deletions

File tree

‎documentation/docs/items.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,8 @@ Specifies if the command is disabled (`true`) or enabled (`false`).
178178

179179
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`.
180180

181+
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.
182+
181183
`disabled`: `boolean` or `function(itemKey, opt)`
182184

183185
#### Example

‎src/jquery.contextMenu.js‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@
127127
namespaces = {},
128128
// mapping namespace to options
129129
menus = {},
130+
// mapping namespace to the options object a `build` menu was actually
131+
// built from. A build menu is rebuilt on every invocation into a fresh
132+
// options object (see handle.contextmenu), so the registration kept in
133+
// `menus` never carries the on-screen menu's $menu/items and can't be
134+
// refreshed by $.contextMenu('update') on its own. Entries are removed
135+
// along with their `menus` entry on destroy; a hidden build menu keeps
136+
// its entry, but op.hide() has emptied the options object by then so
137+
// op.update() below skips it on the $menu guard.
138+
builtMenus = {},
130139
// registrations keyed by raw DOM element rather than a selector
131140
// string - used when `selector` is an Element or jQuery object,
132141
// since those can't be used as `namespaces` object keys the way
@@ -524,6 +533,7 @@
524533
e.data.$trigger = $currentTrigger;
525534

526535
op.create(e.data);
536+
builtMenus[e.data.ns] = e.data;
527537
}
528538
op.show.call($this, e.data, e.pageX, e.pageY);
529539
}
@@ -1912,6 +1922,15 @@
19121922
},
19131923
update: function (opt, root) {
19141924
var $trigger = this;
1925+
// Nothing to update for a registration whose menu element doesn't
1926+
// exist (yet): a `build` menu only gets its $menu when it is first
1927+
// shown, and a destroyed registration can linger as null. Both are
1928+
// reachable from $.contextMenu('update'), which walks every
1929+
// registered menu, so bail out instead of throwing on $menu (see
1930+
// https://github.com/swisnl/jQuery-contextMenu/issues/740).
1931+
if (!opt || !opt.$menu || !opt.$menu.length) {
1932+
return false;
1933+
}
19151934
if (typeof root === 'undefined') {
19161935
root = opt;
19171936
op.resize(opt.$menu);
@@ -2106,7 +2125,12 @@
21062125
opt.$menu.data('_scrollTopAtShow', root.$menu.scrollTop());
21072126
}
21082127
}
2109-
op.update(opt, root); // Correctly update position if user is already hovered over menu item
2128+
// bind `this` to the trigger, same as every other op.update()
2129+
// call site: function-based disabled/visible/name/icon options
2130+
// are documented to run against the trigger element, and a
2131+
// plain op.update(...) call would hand them the internal `op`
2132+
// object instead.
2133+
op.update.call(root.$trigger || $(), opt, root); // Correctly update position if user is already hovered over menu item
21102134
root.positionSubmenu.call(opt.$node, opt.$menu); // positionSubmenu, will only do anything if user already hovered over menu item that just got new subitems.
21112135
}
21122136

@@ -2373,14 +2397,26 @@
23732397

23742398
case 'update':
23752399
// Updates visibility and such
2376-
if(_hasContext){
2377-
op.update($context);
2378-
} else {
2379-
for(var menu in menus){
2380-
if(Object.prototype.hasOwnProperty.call(menus, menu)){
2381-
op.update(menus[menu]);
2382-
}
2400+
for (var menu in menus) {
2401+
if (!Object.prototype.hasOwnProperty.call(menus, menu)) {
2402+
continue;
2403+
}
2404+
// when a context was given, only update the menus registered
2405+
// against it. Passing the context element itself to op.update()
2406+
// (as this used to) can never work, it expects a menu's options
2407+
// object and immediately dereferences its $menu.
2408+
if (_hasContext && (!menus[menu] || menus[menu].context !== o.context)) {
2409+
continue;
23832410
}
2411+
// for a `build` menu the registration isn't the object the
2412+
// on-screen menu was built from, so refresh the built
2413+
// instance when there is one.
2414+
var target = builtMenus[menu] || menus[menu];
2415+
// function-based `disabled`/`visible`/`name`/`icon` options
2416+
// are documented to run against the trigger element, so bind
2417+
// `this` to it rather than leaving it as the internal `op`
2418+
// object that a plain op.update(...) call would pass along.
2419+
op.update.call((target && target.$trigger) || $(), target);
23842420
}
23852421
break;
23862422

@@ -2535,6 +2571,7 @@
25352571
}
25362572

25372573
delete menus[o.ns];
2574+
delete builtMenus[o.ns];
25382575
} catch (e) {
25392576
menus[o.ns] = null;
25402577
}
@@ -2559,6 +2596,7 @@
25592596

25602597
namespaces = {};
25612598
menus = {};
2599+
builtMenus = {};
25622600
elementSelectors = [];
25632601
counter = 0;
25642602
initialized = false;
@@ -2585,6 +2623,7 @@
25852623
}
25862624

25872625
delete menus[ns];
2626+
delete builtMenus[ns];
25882627
} catch (e) {
25892628
menus[ns] = null;
25902629
}
@@ -2605,6 +2644,7 @@
26052644
}
26062645

26072646
delete menus[namespaces[o.selector]];
2647+
delete builtMenus[namespaces[o.selector]];
26082648
} catch (e) {
26092649
menus[namespaces[o.selector]] = null;
26102650
}

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

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
QUnit.module('issue 740 - $.contextMenu("update")', {
2+
afterEach: function() {
3+
$.contextMenu('destroy');
4+
var $fixture = $('#qunit-fixture');
5+
if ($fixture.length) {
6+
$fixture.html('');
7+
}
8+
}
9+
});
10+
11+
function fixture740() {
12+
var $fixture = $('#qunit-fixture');
13+
if ($fixture.length === 0) {
14+
$('<div id="qunit-fixture">').appendTo('body');
15+
$fixture = $('#qunit-fixture');
16+
}
17+
return $fixture;
18+
}
19+
20+
function firstItemDisabled(selector) {
21+
return $(selector).find('li').first().hasClass('context-menu-disabled');
22+
}
23+
24+
QUnit.test('update() called from events.show does not throw and applies the disabled function', function(assert) {
25+
var $fixture = fixture740();
26+
$fixture.append('<div class="t740a">right click me</div>');
27+
28+
var isDisabled = true;
29+
$.contextMenu({
30+
selector: '.t740a',
31+
events: {
32+
show: function() {
33+
$.contextMenu('update');
34+
return true;
35+
}
36+
},
37+
items: {
38+
edit: {name: 'Edit', disabled: function() { return isDisabled; }},
39+
copy: {name: 'Copy'}
40+
}
41+
});
42+
43+
var err = null;
44+
try {
45+
$('.t740a').trigger('contextmenu');
46+
} catch (e) {
47+
err = e;
48+
}
49+
assert.equal(err, null, 'no exception was thrown');
50+
assert.ok(firstItemDisabled('.context-menu-list'), 'item is disabled while the function returns true');
51+
});
52+
53+
QUnit.test('update() does not throw when a build menu has never been shown', function(assert) {
54+
var $fixture = fixture740();
55+
$fixture.append('<div class="t740b">right click me</div><div class="t740b-build">other trigger</div>');
56+
57+
// a `build` menu only gets its $menu the first time it is shown - until then
58+
// its registration has no menu element for update() to walk.
59+
$.contextMenu({
60+
selector: '.t740b-build',
61+
build: function() {
62+
return {items: {foo: {name: 'Foo'}}};
63+
}
64+
});
65+
66+
$.contextMenu({
67+
selector: '.t740b',
68+
events: {
69+
show: function() {
70+
$.contextMenu('update');
71+
return true;
72+
}
73+
},
74+
items: {
75+
edit: {name: 'Edit', disabled: function() { return true; }}
76+
}
77+
});
78+
79+
var err = null;
80+
try {
81+
$('.t740b').trigger('contextmenu');
82+
} catch (e) {
83+
err = e;
84+
}
85+
assert.equal(err, null, 'no exception was thrown');
86+
assert.ok(firstItemDisabled('.context-menu-list'), 'the static menu was still updated');
87+
});
88+
89+
QUnit.test('update() from the events.show of a build menu does not throw', function(assert) {
90+
var $fixture = fixture740();
91+
$fixture.append('<div class="t740c">right click me</div>');
92+
93+
$.contextMenu({
94+
selector: '.t740c',
95+
events: {
96+
show: function() {
97+
$.contextMenu('update');
98+
return true;
99+
}
100+
},
101+
build: function() {
102+
return {
103+
items: {
104+
edit: {name: 'Edit', disabled: function() { return true; }}
105+
}
106+
};
107+
}
108+
});
109+
110+
var err = null;
111+
try {
112+
$('.t740c').trigger('contextmenu');
113+
} catch (e) {
114+
err = e;
115+
}
116+
assert.equal(err, null, 'no exception was thrown');
117+
});
118+
119+
QUnit.test('update() refreshes an open build menu, not just static ones', function(assert) {
120+
var $fixture = fixture740();
121+
$fixture.append('<div class="t740f">right click me</div>');
122+
123+
var isDisabled = false;
124+
$.contextMenu({
125+
selector: '.t740f',
126+
build: function() {
127+
return {
128+
items: {
129+
edit: {name: 'Edit', disabled: function() { return isDisabled; }}
130+
}
131+
};
132+
}
133+
});
134+
135+
$('.t740f').trigger('contextmenu');
136+
assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled');
137+
138+
isDisabled = true;
139+
$.contextMenu('update');
140+
assert.ok(firstItemDisabled('.context-menu-list'), 'the on-screen build menu was refreshed');
141+
});
142+
143+
QUnit.test('update() runs function-based options against the trigger element', function(assert) {
144+
var $fixture = fixture740();
145+
$fixture.append('<div class="t740g">right click me</div>');
146+
147+
var seenThis = null;
148+
$.contextMenu({
149+
selector: '.t740g',
150+
items: {
151+
edit: {
152+
name: 'Edit',
153+
disabled: function() {
154+
seenThis = this;
155+
return !!(this && this.hasClass && this.hasClass('lock-it'));
156+
}
157+
}
158+
}
159+
});
160+
161+
$('.t740g').trigger('contextmenu');
162+
assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled');
163+
164+
$('.t740g').addClass('lock-it');
165+
$.contextMenu('update');
166+
167+
assert.ok(seenThis && seenThis.jquery, '`this` is a jQuery object, not the internal op object');
168+
assert.ok(seenThis && seenThis.is('.t740g'), '`this` is the trigger element');
169+
assert.ok(firstItemDisabled('.context-menu-list'), 'the trigger-dependent disabled state was applied');
170+
});
171+
172+
QUnit.test('update() scoped to a context updates that context\'s menu', function(assert) {
173+
var $fixture = fixture740();
174+
$fixture.append('<div class="t740e-ctx"><div class="t740e">right click me</div></div>');
175+
176+
var isDisabled = false;
177+
$('.t740e-ctx').contextMenu({
178+
selector: '.t740e',
179+
items: {
180+
edit: {name: 'Edit', disabled: function() { return isDisabled; }}
181+
}
182+
});
183+
184+
$('.t740e').trigger('contextmenu');
185+
assert.notOk(firstItemDisabled('.context-menu-list'), 'item starts out enabled');
186+
187+
isDisabled = true;
188+
var err = null;
189+
try {
190+
$.contextMenu('update', {context: '.t740e-ctx'});
191+
} catch (e) {
192+
err = e;
193+
}
194+
assert.equal(err, null, 'no exception was thrown');
195+
assert.ok(firstItemDisabled('.context-menu-list'), 'item became disabled after the scoped update');
196+
});
197+
198+
QUnit.test('a disabled function is re-evaluated on every open without calling update()', function(assert) {
199+
var $fixture = fixture740();
200+
$fixture.append('<div class="t740d">right click me</div>');
201+
202+
var isDisabled = true;
203+
$.contextMenu({
204+
selector: '.t740d',
205+
items: {
206+
edit: {name: 'Edit', disabled: function() { return isDisabled; }}
207+
}
208+
});
209+
210+
$('.t740d').trigger('contextmenu');
211+
assert.ok(firstItemDisabled('.context-menu-list'), 'disabled on the first open');
212+
213+
$('.context-menu-list').trigger('contextmenu:hide', {force: true});
214+
215+
isDisabled = false;
216+
$('.t740d').trigger('contextmenu');
217+
assert.notOk(firstItemDisabled('.context-menu-list'), 'enabled on the second open');
218+
});
219+
220+
QUnit.test('a sub-menu resolving from a promise runs function-based options against the trigger', function(assert) {
221+
var done = assert.async();
222+
var $fixture = fixture740();
223+
$fixture.append('<div class="t740h">right click me</div>');
224+
225+
var seenThis = null;
226+
var deferred = $.Deferred();
227+
228+
$.contextMenu({
229+
selector: '.t740h',
230+
items: {
231+
more: {
232+
name: 'More',
233+
items: deferred.promise()
234+
}
235+
}
236+
});
237+
238+
$('.t740h').trigger('contextmenu');
239+
240+
// resolving the promise runs op.update() for the freshly built sub-menu,
241+
// which must bind `this` to the trigger like every other update path
242+
deferred.resolve({
243+
sub: {
244+
name: 'Sub',
245+
disabled: function() {
246+
seenThis = this;
247+
return false;
248+
}
249+
}
250+
});
251+
252+
setTimeout(function() {
253+
assert.ok(seenThis, 'the sub-menu item\'s disabled function ran');
254+
assert.ok(seenThis && seenThis.jquery, '`this` is a jQuery object, not the internal op object');
255+
assert.ok(seenThis && seenThis.jquery && seenThis.is('.t740h'), '`this` is the trigger element');
256+
done();
257+
}, 50);
258+
});

0 commit comments

Comments
 (0)