fix: detect the {x, y} overload by key presence (#812)

[bbrala]

Closes #812

The bug

$.fn.contextMenu(operation) decided between the {x, y} positioning overload and a menu definition with:

} else if (typeof operation.x !== 'undefined' && typeof operation.y !== 'undefined') {

An event that carries no pointer position (a keyboard-originated or synthetic one) has pageX/pageY undefined, so $(el).contextMenu({x: e.pageX, y: e.pageY}) produced {x: undefined, y: undefined}. That failed the test above, fell through to the $.isPlainObject() branch, was treated as a menu definition and reached $.contextMenu('create', ...), which threw No selector specified for what was clearly a positioning call.

The fix

Detection is now by key presence rather than by value: an object carrying an x or a y key is a positioning call regardless of what those values are. A menu definition still wins whenever the object also carries a definition-only key (items, selector or build), so an options object that happens to have an x/y of its own cannot be misrouted. The plugin has no x/y option, so that guard is belt and braces, but it keeps the widened detection strictly non-breaking.

For the values themselves I chose the graceful fallback, not a thrown error: when the pair is not a usable coordinate pair, the menu is shown at the element-relative default position, exactly as $(el).contextMenu() with no argument does. Reasoning:

• The reported trigger is a legitimate case, not a mistake. A keyboard activation genuinely has no pointer position, and the caller's intent is "show the menu on this element". Throwing turns a recoverable case into a crash on a code path the caller cannot always predict.
• It needs no new machinery. position() already routes to determinePosition() when it has no coordinates, and $(el).contextMenu() already means "position relative to the element". The fallback just leaves pageX/pageY unset on the triggered event and reuses that path, so there is one behaviour for "no coordinates" instead of two.
• The house style elsewhere in the plugin is to fall back for missing optional data (the layerClick fakeClick guard, the e.data guard in the contextmenu handler). It throws for structurally impossible input, such as a create without a selector, which this is not.

Details:

• A half-filled pair ({x: 123, y: undefined}) and a half-omitted one ({x: 123}) fall back too, rather than computing an offset of NaN or throwing.
• 0 is treated as a real coordinate everywhere. The default position() used to bail out to determinePosition() on !x && !y, which also caught the perfectly valid page origin. It now checks for actual numbers, with the 'maintain' branch moved ahead of that check.
• Numeric strings ({x: '123', y: '456'}, e.g. read off a data attribute) still work, since the positioning arithmetic always accepted them. They are coerced before going on the event, so e.pageX is always a real number.

Docs

documentation/docs/plugin-commands.md now states that x and y are page coordinates, the same space as event.pageX / event.pageY and scroll inclusive, not viewport coordinates and not relative to the trigger, with a short conversion snippet for viewport-based sources such as getBoundingClientRect(), a canvas or a map library. It also documents the fallback. See #726 for a worked example of why this matters.

Backwards compatibility

No currently working call shape changes. test/unit/issue-812-xy-overload.test.js pins the ones that share this dispatch chain: no argument, 'hide', 'destroy', false/true (disable/enable), a plain definition object, a definition object that also carries x/y, and {x: 123, y: 456} reaching position() unchanged. Everything else this PR touches used to throw or to misposition, so the only behaviour changes are crash-to-working ones. Nothing new is thrown.

The one deliberate behaviour change is {x: 0, y: 0}: it used to be caught by the !x && !y bail-out and positioned relative to the element, and now lands at the page origin as documented. The existing open contextMenu at 0,0 test still passes.

Tests

New test/unit/issue-812-xy-overload.test.js: undefined coordinates no longer throw and fall back, an omitted x or y key falls back, finite and negative coordinates are forwarded unchanged, numeric strings arrive as numbers, {x: 0, y: 0} is honoured by both the routing and the default position(), a half-filled pair falls back without producing NaN css, non-numeric values fall back, a definition object carrying x/y still registers a menu, and the call shapes listed above are unchanged.

Full unit suite green (92 tests) and the 36 acceptance tests pass, since positioning is exercised there. dist/ is intentionally untouched; test/integration/html is gitignored, so the docs edit brings no generated HTML with it.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6fb3d90a3

ℹ️ 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".

[chatgpt-codex-connector]

Recognize single-key coordinate operations

When callers actually omit one coordinate, such as .contextMenu({x: 123}), this predicate returns false because it requires both keys. The argument then enters the plain-object menu-definition branch and ultimately throws No selector specified, rather than using determinePosition as the new documentation and changelog promise. The half-filled test does not catch this because it supplies a y key whose value is undefined; treat the presence of either coordinate key as the positioning overload so genuinely missing properties also fall back.

Useful? React with 👍 / 👎.

[bbrala]

Thanks, addressed in 1da51e9.

Recognize single-key coordinate operations (P2) - agreed and fixed. {x: 123} with the y key genuinely absent behaved differently from {x: 123, y: undefined}, which is exactly the inconsistency this PR set out to remove, and it contradicted what the docs and changelog now promise. The predicate now accepts either key.

Because that widens what counts as a positioning call, I paired it with a guard in the other direction: an object that also carries a definition-only key (items, selector or build) is still treated as a menu definition, so an options object that happens to have an x/y of its own cannot be hijacked into the positioning branch. There is no x/y option in the plugin, so this is belt and braces rather than a known case, but it keeps the widening strictly non-breaking.

New tests cover both: an omitted x and an omitted y key each fall back to determinePosition, and a definition object carrying x/y still registers a menu. I also pinned the neighbouring call shapes that share this dispatch chain - no argument, 'hide', 'destroy', true/false, and the plain definition object - so the widened detection is demonstrably non-breaking, and added negative coordinates alongside the existing 0 case.

92 unit tests and 36 acceptance tests green locally; CI was green on the first commit across jQuery 1.12.4 / 2.2.4 / 3.7.1 / 4.0.0.