fix: accept a raw Element as the context option (#817)

* fix: accept a raw Element as the context option

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

* fix: narrow the context fix to newly-accepted Element shapes only

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.

Author: bbrala

CHANGELOG.md

@@ -28,12 +28,14 @@
 * A `select`'s phantom mousedown is no longer treated as an outside click (fixes #744)
 * A re-dispatched layer click now targets the element actually clicked (fixes #771)
 * Guard against undefined `e.data` in the contextmenu handler (fixes #777)
+* A raw `Element` passed as `context` now scopes the registration instead of being silently ignored (fixes #809). An empty `<form>`/`<select>` and a selector string matching nothing are still ignored, as they are today
 * Inputs imported from an HTML5 `<menu>` are named after their `<label>` again instead of falling back to the `name` attribute (fixes #811). Note that this is a visible change: a labelled input imported through `$.contextMenu.fromMenu()` or `$.contextMenu('html5')` now shows its label text where it used to show its `name` attribute. Inputs without a label, without an id, or with an empty label keep showing the `name` attribute exactly as before.
 * `$(...).contextMenu({x, y})` with missing or non-numeric coordinates now falls back to the element-relative position instead of throwing `No selector specified`, and an explicit `{x: 0, y: 0}` is honoured (fixes #812)
 * Clicking on after the menu was destroyed no longer throws with `useModal: false` (fixes #805)
 
 #### Documentation
 
+* Documented the `context` option (fixes #809)
 * Documented using custom SVG icons without a gulp build step (fixes #762)
 * 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)

documentation/docs.md

@@ -13,6 +13,7 @@ title: jQuery contextMenu — Documentation
 - [Update contextMenu state](#update-contextmenu-state)
 - [Options (at registration)](#options-at-registration)
   - [selector](#selector)
+  - [context](#context)
   - [items](#items)
   - [appendTo](#appendto)
   - [trigger](#trigger)
@@ -66,6 +67,42 @@ $.contextMenu({
 });
 ```
 
+### context
+
+Limits the registration to a single container element: only elements matching the [selector](#selector) inside that container trigger this menu. Elements matching the selector elsewhere on the page are left alone, so the same selector can be registered more than once with a different menu per container.
+
+`context`: `string`, `DOMElement` or `jQuery object` default: `document`
+
+A selector string is resolved against the document and its first match is used. A jQuery object matching nothing is treated as if no context was given, and the menu is registered for the whole document.
+
+Two shapes are ignored for historical reasons, and the menu is registered for the whole document instead. Both will start scoping in a future major release, so do not rely on them:
+
+* a `<form>` or `<select>` element with no controls or options in it, because such an element reports a length of `0`,
+* a selector string matching no element, which registers nothing at all.
+
+#### Example
+```javascript
+// scope the menu to a container, selected with a selector string
+$.contextMenu({
+    selector: '.context-menu',
+    context: 'div#panel'
+});
+
+// scope the menu to a container, selected with a dom element
+var element = document.getElementById('panel');
+$.contextMenu({
+    selector: '.context-menu',
+    context: element
+});
+
+// scope the menu to a container, selected with a jQuery object.
+// $(container).contextMenu(options) is shorthand for this.
+$.contextMenu({
+    selector: '.context-menu',
+    context: $('#panel')
+});
+```
+
 ### items
 
 Object with [items](docs/items.html) to be listed in contextMenu. See [items](docs/items.html) for a full documentation on how to build your menu items.

src/jquery.contextMenu.js

@@ -2747,14 +2747,51 @@
         var $document = $(document);
         var $context = $document;
         var _hasContext = false;
+        // was `context` given as a raw Element? Those used to be ignored, which
+        // means they were also tracked in `namespaces` - see the 'create'
+        // operation, which keeps doing that so destroy-by-selector keeps
+        // working for them.
+        var _contextFromElement = false;
 
         // an Element / jQuery object can't be used as a delegated-event
         // selector string, so it's normalized here once and handled
         // separately by the 'create'/'destroy' operations below.
         var useElementSelector = isElementSelector(o.selector);
         var $elements = useElementSelector ? (o.selector.jquery ? o.selector : $(o.selector)) : null;
 
-        if (!o.context || !o.context.length) {
+        // `context` may be given as a selector string, an Element, a jQuery
+        // object or `document`, and is normalized here to a single DOM node:
+        // every consumer of `o.context` treats it as a raw node
+        // (`o.context !== el`, `$.contains(o.context, el)`, ...).
+        //
+        // A raw Element used to be dropped by the `!o.context.length` test
+        // below - an Element has no `length` at all - which silently left the
+        // menu registered for the whole document instead of scoped to the
+        // element the caller passed. That is what is fixed here.
+        // See https://github.com/swisnl/jQuery-contextMenu/issues/809
+        //
+        // Everything else is left exactly as it was, on purpose. Newly
+        // honouring a context that is ignored today un-scopes menus that work
+        // today, silently and without an error, so it is a breaking change and
+        // belongs in a major release:
+        //  - values carrying a numeric `length` keep the legacy length test.
+        //    That covers <form> and <select> (whose `length` is their
+        //    control/option count, so an empty one is still ignored) and
+        //    `window` (its frame count), as well as strings and jQuery objects.
+        //  - only 'create' honours an Element. For 'destroy' and 'update',
+        //    `context` means the *trigger* element rather than a container -
+        //    that is what `$.fn.contextMenu('destroy')` passes - and an Element
+        //    has never been accepted there. Scoping those too would turn a
+        //    `$.contextMenu('destroy', {context: element})` that tears
+        //    everything down today into a silent no-op.
+        if (!o.context) {
+            o.context = document;
+        } else if (operation === 'create' && o.context.nodeType === 1 && typeof o.context.length !== 'number') {
+            $context = $(o.context);
+            // an element is never the document, so this is always a real scope
+            _hasContext = true;
+            _contextFromElement = true;
+        } else if (!o.context.length) {
             o.context = document;
         } else {
             // you never know what they throw at you...
@@ -2815,7 +2852,13 @@
                     $elements.each(function () {
                         elementSelectors.push({el: this, ns: o.ns});
                     });
-                } else if (!_hasContext) {
+                } else if (!_hasContext || _contextFromElement) {
+                    // An Element `context` used to be ignored, so the menu was
+                    // registered globally *and* tracked here, which is what
+                    // makes `$.contextMenu('destroy', selector)` able to find
+                    // it. Keep tracking it now that the context is honoured,
+                    // otherwise that teardown call would silently stop working.
+                    // See https://github.com/swisnl/jQuery-contextMenu/issues/809
                     namespaces[o.selector] = o.ns;
                 }
                 menus[o.ns] = o;
@@ -3005,23 +3048,30 @@
                         }
                     });
                 } else if (namespaces[o.selector]) {
+                    var selectorNs = namespaces[o.selector];
+                    // the handler lives on the menu's own context, which is
+                    // `document` for a global registration but the element
+                    // itself when the menu was registered with an Element
+                    // context (see the 'create' operation)
+                    var selectorNsContext = (menus[selectorNs] && menus[selectorNs].context) || document;
+
                     $visibleMenu = $('.context-menu-list').filter(':visible');
                     if ($visibleMenu.length && $visibleMenu.data().contextMenuRoot.$trigger.is(o.selector)) {
                         $visibleMenu.trigger('contextmenu:hide', {force: true});
                     }
 
                     try {
-                        if (menus[namespaces[o.selector]].$menu) {
-                            menus[namespaces[o.selector]].$menu.remove();
+                        if (menus[selectorNs].$menu) {
+                            menus[selectorNs].$menu.remove();
                         }
 
-                        delete menus[namespaces[o.selector]];
-                        delete builtMenus[namespaces[o.selector]];
+                        delete menus[selectorNs];
+                        delete builtMenus[selectorNs];
                     } catch (e) {
-                        menus[namespaces[o.selector]] = null;
+                        menus[selectorNs] = null;
                     }
 
-                    $document.off(namespaces[o.selector]);
+                    $(selectorNsContext).off(selectorNs);
                 }
                 break;
 

test/unit/issue-731-selector-html-injection.test.js

@@ -153,17 +153,20 @@ QUnit.test('context may be a selector string', function(assert) {
   assert.equal(counter.shown, 1, 'a trigger inside the string context opens the menu');
 });
 
-// A raw Element has no `length`, so `!o.context.length` sends it down the "no
-// context" branch and it silently becomes `document` - the registration is not
-// scoped to the element at all. That is pre-existing behaviour, unrelated to
-// this change; all that is asserted here is that an Element context still
-// yields a working menu.
+// A raw Element used to be dropped here (it has no `length`, which is what the
+// old normalisation tested), leaving the registration bound to `document`
+// instead of scoped to the element. See
+// https://github.com/swisnl/jQuery-contextMenu/issues/809 for that fix and
+// test/unit/issue-809-context-element.test.js for its own coverage.
 QUnit.test('context may be an Element', function(assert) {
   var fixture = setupScopedFixture();
   var counter = {shown: 0};
 
   registerScopedMenu(fixture.$container.get(0), counter);
 
+  fixture.$outside.trigger($.Event('contextmenu'));
+  assert.equal(counter.shown, 0, 'a trigger outside the Element context is not handled');
+
   fixture.$inside.trigger($.Event('contextmenu'));
   assert.equal(counter.shown, 1, 'a trigger inside the Element context opens the menu');
 });