fix: accept a raw Element as the context option - #817
Open
bbrala wants to merge 2 commits into
Open
Conversation
`context` was normalised with `if (!o.context || !o.context.length)`, which tested a shape that only jQuery objects and strings have. A raw DOM Element has no `length` at all, so it was always dropped and the registration silently fell back to `document`: the menu worked, but page-wide rather than scoped to the element the caller passed. The same check misfired the other way for `<form>` and `<select>` elements, whose `length` is their control/option count, so an empty one was dropped and a non-empty one accepted. Normalise by type instead: resolve the value once and keep it when it is an element or the document, in the spirit of the existing isElementSelector() helper used for `selector`. `o.context` stays a raw DOM node, which is what every consumer expects (`$.contains(o.context, el)`, `o.context !== el`). A context that resolves to no element now falls back to `document` as well. Previously a selector string matching nothing left `o.context` undefined and `$context` empty, so the registration was stored but bound to nothing at all and could never fire.
Backwards compatibility review of the previous commit. Registering with a raw Element context is the fix #809 asks for, but the same normalisation also decides several shapes that are ignored *today*, and quietly honouring those would un-scope menus that currently work - with no error, the menu simply stops appearing outside the context element. So the change is now strictly additive. Only an Element, and only for 'create', takes the new branch; everything else keeps the legacy length test verbatim: * <form>/<select> (and anything else with a numeric `length`, such as `window`) keep the old length-based decision, so an empty one is still ignored. * A context selector matching nothing still registers nothing. * 'destroy'/'update' still ignore an Element context. There `context` means the trigger element, not a container, and scoping it would turn a $.contextMenu('destroy', {context: element}) that tears everything down today into a silent no-op. An ignored Element context also meant the menu was tracked in `namespaces`, which is what lets $.contextMenu('destroy', selector) find it. Keep tracking it so that teardown keeps working, and unbind from the menu's own context rather than always from `document`, which is where the handler now lives. The legacy cases are pinned by tests, and documented as deprecated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #809
Problem
contextwas normalised with a check that only jQuery objects and strings answer meaningfully:A raw DOM
Elementhas no.length, so it was dropped and the registration silently fell back todocument. The menu still worked, but page-wide instead of scoped to the element that was passed, with no error and no warning.Fix
An Element is now honoured, on the
createoperation, as a newly-supported shape:o.contextstays a raw DOM node, which is what every consumer expects:$.contains(o.context, el)ando.context !== elin the trigger matching,menus[menu].context !== o.contextinupdate,$(o.context).off(o.ns)indestroy. Nothing downstream needed to change.Backwards compatibility
The change is deliberately additive: it only affects a shape that is ignored today and does nothing useful. Everything the old check already decided is left byte-for-byte alone, because newly honouring an ignored context does not fail loudly, it just stops a working menu from appearing outside the context element.
Kept exactly as-is, and now pinned by tests:
Element(create)<form>/<select>withlength === 0<form>/<select>withlength > 0length(e.g.window)Elementpassed todestroy/updateTwo notes on the choices there:
<form>/<select>is not carved out by tag name. The rule is "a value carrying a numericlengthkeeps the legacy length test", which is exactly the set of inputs the old code decided by length,windowincluded. An empty<form>still falls back todocument.destroy/updateare excluded on purpose. For those operationscontextmeans the trigger element rather than a container (that is what$.fn.contextMenu('destroy')passes), and an Element has never been accepted there: it falls through to the "no context, no selector" branch, which tears down every registered menu. Scoping it would silently turn$.contextMenu('destroy', {context: element})from a full teardown into a no-op, i.e. a leak.One knock-on effect had to be handled to keep an existing call working. An ignored Element context meant the menu was also tracked in
namespaces, and that map is what lets$.contextMenu('destroy', '.selector')find a registration. Honouring the context would have dropped it from that map and silently broken destroy-by-selector for exactly the callers this PR is for. So Element-context registrations keep being tracked, and the destroy-by-selector branch now unbinds from the menu's own context instead of always fromdocument, which is where the handler actually lives now. For every pre-existing registration that context isdocument, so that line is a no-op change.No jQuery 3-only APIs are used, jQuery 1.12+ stays supported.
Tests
New
test/unit/issue-809-context-element.test.js. With the new branch disabled, exactly two assertions fail and everything else still passes, which is the check that the change really is additive:Elementascontextscopes the registration (outside triggers do not open the menu, inside ones do)issue-731's Element case, whose comment described the unscoped behaviour as knowingly-broken and is now updatedAlso asserted, passing both before and after, so a future change cannot quietly regress them:
<form>context still scopes, an empty<form>/<select>context is still ignored$.contextMenu('destroy', {context: element})still tears everything downcontextat all are unchangedThe test file is wrapped in an IIFE: Karma loads every unit test file as a plain script into one shared global scope, and the helper names collide with those in the #731 file otherwise.
Verification
npm run test-unit: 90 passednpm run test-accept: 36 passed (run against a locally rebuiltdist/;dist/is not part of this PR)npx eslint src/jquery.contextMenu.js test: cleanDocs
contextwas not documented at all. Added a### contextsection todocumentation/docs.mdwith the accepted shapes and an explicit note on the two ignored ones, flagged as deprecated so they can start scoping in a major release. Changelog entry added.