fix: $.contextMenu('update') threw when a build menu had not been shown - #800
Conversation
$.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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9a4fadd4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!opt || !opt.$menu || !opt.$menu.length) { | ||
| return false; |
There was a problem hiding this comment.
Update the active instance of build menus
When a build menu is already open and its dynamic state changes, $.contextMenu('update') still cannot refresh it. The options object stored in menus is the original registration and never receives $menu; handle.contextmenu creates a separate cloned options object for each invocation and stores that on the trigger. Consequently this guard also returns for an active build menu, silently leaving its labels, visibility, and disabled state stale rather than merely skipping an unshown menu.
Useful? React with 👍 / 👎.
| if (_hasContext && (!menus[menu] || menus[menu].context !== o.context)) { | ||
| continue; | ||
| } | ||
| op.update(menus[menu]); |
There was a problem hiding this comment.
Bind scoped updates to the menu trigger
When a custom-context menu has been opened and $.contextMenu('update', {context: ...}) is called, invoking op.update as a method makes its this value the internal op object. op.update forwards that value as the this context of function-based disabled and visible options, although those callbacks are documented to run against the triggering element. Thus callbacks that inspect this produce the wrong state on the newly repaired scoped-update path; invoke the update with the menu's active $trigger instead.
Useful? React with 👍 / 👎.
… 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.
|
Thanks, both Codex points were legitimate and are addressed in 9420a73. "Update the active instance of build menus" — correct, and the more important of the two. A "Bind scoped updates to the menu trigger" — also correct, and it was not limited to the scoped path: the pre-existing Two regression tests added, both verified failing without the follow-up commit:
Nothing was deliberately left unaddressed. |
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.
|
👍 |
Problem
$.contextMenu('update')walks every registered menu and hands each one toop.update(), which immediately dereferencesopt.$menu. Abuildmenu only gets its$menuthe first time it is actually shown, so as soon as a page has anybuildmenu registered, a plain$.contextMenu('update')throws:The same happens for a registration that lingered as
nullafter a faileddestroy.That is what the reporter of #740 ran into: they called
$.contextMenu('update')from anevents.showhandler in order to refresh an item'sdisabledstate, and the menu blew up on open. The stack in the issue is from the abandoned3.xbranch, butmasterhas the equivalent defect, reproduced here as a unit test.While confirming this, a second problem in the same
case 'update':block turned up: the context-scoped branch calledop.update($context), passing the context element whereop.update()expects a menu's options object. It dereferences$context.$menu, which is alwaysundefined, so$.contextMenu('update', {context: ...})could never do anything but throw.Fix
op.update()returns early for a registration without a menu element, instead of throwing.Note on the reporter's actual question
They asked whether calling
update()fromevents.showis the right way to toggle an entry's enabled state per open. On 2.x it is not needed at all:op.show()already runsop.update()on every open, so a function-baseditem.disabledis re-evaluated each time the menu is shown. Added a sentence to thedisableddocs saying so, plus a regression test covering it. Theupdate()crash is still a real bug worth fixing, sinceupdate()is a documented public operation.Tests
test/unit/issue-740-update.test.js:update()fromevents.showdoes not throw and applies thedisabledfunctionupdate()does not throw when abuildmenu has never been shown (fails on master)update()from theevents.showof abuildmenu does not throw (fails on master)update()scoped to a context updates that context's menu (fails on master)disabledfunction is re-evaluated on every open without callingupdate()npm test(47 tests) and the Playwright acceptance suite (22 tests) are green.Closes #740