diff --git a/CHANGELOG.md b/CHANGELOG.md index e2fb2846..710c8b8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ * 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) +* `$(...).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 @@ -35,6 +36,7 @@ * 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) +* Documented that `$(...).contextMenu({x, y})` takes page coordinates (fixes #812) ### 2.10.2 diff --git a/documentation/docs/plugin-commands.md b/documentation/docs/plugin-commands.md index bf2ac7e5..4477cfc1 100644 --- a/documentation/docs/plugin-commands.md +++ b/documentation/docs/plugin-commands.md @@ -42,6 +42,18 @@ $(".some-selector").contextMenu(); $(".some-selector").contextMenu({x: 123, y: 123}); ``` +`x` and `y` are **page** coordinates, the same space as `event.pageX` / `event.pageY`, so they include the document scroll. They are not viewport coordinates and they are not relative to the trigger element. Coming from a viewport-based source (`event.clientX` / `event.clientY`, `getBoundingClientRect()`, a canvas or map library) add the current scroll offset: + +``` +var rect = element.getBoundingClientRect(); +$(".some-selector").contextMenu({ + x: rect.left + window.scrollX, + y: rect.bottom + window.scrollY +}); +``` + +When either `x` or `y` is missing or is not a number, which happens when they are read off an event that carries no pointer position such as a keyboard or synthetic one, the menu falls back to `determinePosition` and is positioned relative to the trigger element, just like `$(".some-selector").contextMenu()`. + ## Manually hide a contextMenu hide the contextMenu of the first element of the selector: diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index f771df49..61876800 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -220,14 +220,18 @@ position: function (opt, x, y) { var offset; // determine contextMenu position - if (!x && !y) { - opt.determinePosition.call(this, opt.$menu); - return; - } else if (x === 'maintain' && y === 'maintain') { + if (x === 'maintain' && y === 'maintain') { // x and y must not be changed (after re-show on command click) offset = opt.$menu.position(); + } else if (!isCoordinate(x) || !isCoordinate(y)) { + // No usable coordinates: no mouse position at all, or only + // one of the two. Note this deliberately tests for real + // numbers instead of truthiness, so an explicit 0 still + // positions the menu at the page origin. + opt.determinePosition.call(this, opt.$menu); + return; } else { - // x and y are given (by mouse event) + // x and y are given (by mouse event), as page coordinates var offsetParentOffset = opt.$menu.offsetParent().offset(); offset = {top: y - offsetParentOffset.top, left: x -offsetParentOffset.left}; } @@ -2576,6 +2580,39 @@ (selector.nodeType === 1 || (typeof selector.jquery !== 'undefined' && typeof selector.length === 'number')); } + // is the given value usable as a page coordinate? Finite numbers are, and + // so are numeric strings, which the positioning arithmetic has always + // accepted (`{x: el.dataset.x, ...}`). Note 0 is a perfectly valid + // coordinate, so this can never be a truthiness check. + function isCoordinate(value) { + if (typeof value === 'string') { + return value.trim() !== '' && isFinite(Number(value)); + } + + return typeof value === 'number' && isFinite(value); + } + + // is the given `$.fn.contextMenu()` argument the {x, y} positioning + // overload rather than a menu definition? Decided on key *presence*, not on + // the values: an event that carries no pointer position (a keyboard or + // synthetic one) yields {x: undefined, y: undefined}, which is still + // clearly meant as a position and must not be mistaken for a menu + // definition. A menu definition wins whenever the object also carries a + // definition-only key, so an options object with an `x`/`y` of its own + // keeps being registered as a menu. + // See https://github.com/swisnl/jQuery-contextMenu/issues/812 + function isCoordinateOperation(operation) { + if (!operation || typeof operation !== 'object') { + return false; + } + + if ('items' in operation || 'selector' in operation || 'build' in operation) { + return false; + } + + return 'x' in operation || 'y' in operation; + } + // resolve a caller-supplied "selector-ish" option (`context`, `appendTo`, // the element passed to `fromMenu()`, ...) to a jQuery object without ever // letting a string be evaluated as HTML. `$(string)` builds a detached DOM @@ -2613,12 +2650,18 @@ if (this.length > 0) { // this is not a build on demand menu if (typeof operation === 'undefined') { this.first().trigger('contextmenu'); - } else if (typeof operation.x !== 'undefined' && typeof operation.y !== 'undefined') { - this.first().trigger($.Event('contextmenu', { - pageX: operation.x, - pageY: operation.y, - mouseButton: operation.button - })); + } else if (isCoordinateOperation(operation)) { + var eventProperties = {mouseButton: operation.button}; + // Only a complete pair of real numbers can position the menu. + // Anything else - undefined coordinates from an event without a + // pointer position, a half-filled pair - falls back to the + // element-relative default position, i.e. what + // `$(...).contextMenu()` does, by leaving pageX/pageY unset. + if (isCoordinate(operation.x) && isCoordinate(operation.y)) { + eventProperties.pageX = Number(operation.x); + eventProperties.pageY = Number(operation.y); + } + this.first().trigger($.Event('contextmenu', eventProperties)); } else if (operation === 'hide') { var $menu = this.first().data('contextMenu') ? this.first().data('contextMenu').$menu : null; if ($menu) { diff --git a/test/unit/issue-812-xy-overload.test.js b/test/unit/issue-812-xy-overload.test.js new file mode 100644 index 00000000..f985c77b --- /dev/null +++ b/test/unit/issue-812-xy-overload.test.js @@ -0,0 +1,270 @@ +QUnit.module('issue 812 - $.fn.contextMenu({x, y}) overload', { + beforeEach: function() { + var $fixture = $('#qunit-fixture'); + if ($fixture.length === 0) { + $('
').appendTo('body'); + $fixture = $('#qunit-fixture'); + } + $fixture.html(''); + }, + afterEach: function() { + $.contextMenu('destroy'); + var $fixture = $('#qunit-fixture'); + if ($fixture.length) { + $fixture.html(''); + } + } +}); + +// Registers a menu on .issue-812-trigger and records how it gets positioned. +// Returns the recorder so a test can inspect what reached `position` / +// `determinePosition`. +function registerIssue812Menu(extraOptions) { + var recorder = { + positionArgs: [], + determinePositionCalls: 0, + showCalls: 0 + }; + + $.contextMenu($.extend({ + selector: '.issue-812-trigger', + determinePosition: function($menu) { + recorder.determinePositionCalls++; + $menu.css({top: 0, left: 0}); + }, + events: { + show: function() { + recorder.showCalls++; + } + }, + items: { + copy: {name: 'Copy'} + } + }, extraOptions || {})); + + return {recorder: recorder}; +} + +QUnit.test('{x: undefined, y: undefined} does not throw "No selector specified"', function(assert) { + // Regression test for https://github.com/swisnl/jQuery-contextMenu/issues/812 + // pageX/pageY are undefined for keyboard-originated or synthetic events, so + // {x: e.pageX, y: e.pageY} legitimately ends up with undefined values. That + // used to fall through to the plain-object branch and be treated as a menu + // definition, throwing "No selector specified". + var menu = registerIssue812Menu(); + + var thrown = null; + try { + $('.issue-812-trigger').contextMenu({x: undefined, y: undefined}); + } catch (e) { + thrown = e; + } + + assert.equal(thrown, null, 'showing with undefined coordinates did not throw' + (thrown ? ' (got: ' + thrown.message + ')' : '')); + assert.equal(menu.recorder.showCalls, 1, 'the menu was shown'); + assert.equal(menu.recorder.determinePositionCalls, 1, 'it fell back to the element-relative default position'); +}); + +QUnit.test('finite coordinates are forwarded as page coordinates', function(assert) { + var recorded = []; + var menu = registerIssue812Menu({ + position: function(opt, x, y) { + recorded.push([x, y]); + opt.$menu.css({top: 0, left: 0}); + } + }); + + $('.issue-812-trigger').contextMenu({x: 123, y: 456}); + + assert.equal(menu.recorder.showCalls, 1, 'the menu was shown'); + assert.deepEqual(recorded, [[123, 456]], 'x and y reached position() unchanged'); +}); + +QUnit.test('numeric strings keep working and arrive as numbers', function(assert) { + // The positioning arithmetic has always coped with numeric strings, e.g. + // coordinates read straight off a data attribute, so they must not start + // silently falling back. + var recorded = []; + var menu = registerIssue812Menu({ + position: function(opt, x, y) { + recorded.push([x, y]); + opt.$menu.css({top: 0, left: 0}); + } + }); + + $('.issue-812-trigger').contextMenu({x: '123', y: '456'}); + + assert.equal(menu.recorder.determinePositionCalls, 0, 'numeric strings did not fall back'); + assert.deepEqual(recorded, [[123, 456]], 'numeric strings reached position() as numbers'); +}); + +QUnit.test('zero is a valid coordinate and is not treated as "no coordinates"', function(assert) { + var recorded = []; + var menu = registerIssue812Menu({ + position: function(opt, x, y) { + recorded.push([x, y]); + opt.$menu.css({top: 0, left: 0}); + } + }); + + $('.issue-812-trigger').contextMenu({x: 0, y: 0}); + + assert.equal(menu.recorder.showCalls, 1, 'the menu was shown'); + assert.deepEqual(recorded, [[0, 0]], '0/0 reached position() as real coordinates'); +}); + +QUnit.test('the default position() places a menu at 0/0 instead of falling back', function(assert) { + // The default position() used to bail out to determinePosition() on + // `!x && !y`, which also caught the perfectly valid page origin. + var menu = registerIssue812Menu(); + + $('.issue-812-trigger').contextMenu({x: 0, y: 0}); + + assert.equal(menu.recorder.determinePositionCalls, 0, 'determinePosition() was not used for an explicit 0/0'); +}); + +QUnit.test('a half-filled coordinate pair falls back instead of positioning at NaN', function(assert) { + var menu = registerIssue812Menu(); + + var thrown = null; + try { + $('.issue-812-trigger').contextMenu({x: 123, y: undefined}); + } catch (e) { + thrown = e; + } + + assert.equal(thrown, null, 'showing with only one coordinate did not throw'); + assert.equal(menu.recorder.determinePositionCalls, 1, 'it fell back to the element-relative default position'); + + var $menu = $('.issue-812-trigger').data('contextMenu').$menu; + assert.notOk(isNaN(parseFloat($menu.css('top'))), 'the menu top is a real number'); + assert.notOk(isNaN(parseFloat($menu.css('left'))), 'the menu left is a real number'); +}); + +QUnit.test('non-numeric coordinates fall back to the element-relative position', function(assert) { + var menu = registerIssue812Menu(); + + var thrown = null; + try { + $('.issue-812-trigger').contextMenu({x: 'nope', y: null}); + } catch (e) { + thrown = e; + } + + assert.equal(thrown, null, 'showing with non-numeric coordinates did not throw'); + assert.equal(menu.recorder.showCalls, 1, 'the menu was shown'); + assert.equal(menu.recorder.determinePositionCalls, 1, 'it fell back to the element-relative default position'); +}); + +QUnit.test('an omitted coordinate key falls back as well', function(assert) { + // Raised in review: {x: 123} - the key genuinely missing rather than + // undefined - must behave the same as {x: 123, y: undefined}. + var menu = registerIssue812Menu(); + + var thrown = null; + try { + $('.issue-812-trigger').contextMenu({x: 123}); + } catch (e) { + thrown = e; + } + + assert.equal(thrown, null, 'showing with only an x key did not throw'); + assert.equal(menu.recorder.determinePositionCalls, 1, 'it fell back to the element-relative default position'); + + $('.issue-812-trigger').contextMenu('hide'); + $('.issue-812-trigger').contextMenu({y: 123}); + assert.equal(menu.recorder.determinePositionCalls, 2, 'the same goes for only a y key'); +}); + +QUnit.test('negative coordinates are real coordinates', function(assert) { + var recorded = []; + var menu = registerIssue812Menu({ + position: function(opt, x, y) { + recorded.push([x, y]); + opt.$menu.css({top: 0, left: 0}); + } + }); + + $('.issue-812-trigger').contextMenu({x: -5, y: -10}); + + assert.equal(menu.recorder.determinePositionCalls, 0, 'negative coordinates did not fall back'); + assert.deepEqual(recorded, [[-5, -10]], 'negative coordinates reached position() unchanged'); +}); + +QUnit.test('a menu definition carrying x/y keys is still a menu definition', function(assert) { + // The overload is detected by key presence, so a definition object that + // happens to have x/y properties of its own must not be hijacked into the + // positioning branch. + var shown = 0; + + $('#qunit-fixture').contextMenu({ + selector: '.issue-812-trigger', + x: 10, + y: 20, + events: { + show: function() { + shown++; + } + }, + items: { + copy: {name: 'Copy'} + } + }); + + $('.issue-812-trigger').trigger($.Event('contextmenu')); + + assert.equal(shown, 1, 'the definition was registered as a menu, not treated as coordinates'); +}); + +QUnit.test('a plain object without x/y keys is still treated as a menu definition', function(assert) { + var shown = 0; + + $('#qunit-fixture').contextMenu({ + selector: '.issue-812-trigger', + events: { + show: function() { + shown++; + } + }, + items: { + copy: {name: 'Copy'} + } + }); + + $('.issue-812-trigger').trigger($.Event('contextmenu')); + + assert.equal(shown, 1, 'the jQuery-fn create shorthand still registers a menu'); +}); + +QUnit.test('every other $.fn.contextMenu() call shape is unchanged', function(assert) { + // Pins the operations that share the same dispatch chain as the {x, y} + // overload, so widening the coordinate detection cannot break them. + var menu = registerIssue812Menu(); + var $trigger = $('.issue-812-trigger'); + + // no argument at all: show, positioned relative to the element + $trigger.contextMenu(); + assert.equal(menu.recorder.showCalls, 1, 'no-argument form shows the menu'); + assert.equal(menu.recorder.determinePositionCalls, 1, 'no-argument form positions relative to the element'); + + // 'hide' + $trigger.contextMenu('hide'); + assert.notOk($trigger.hasClass('context-menu-active'), '"hide" closed the menu'); + + // false / true: disable and re-enable the trigger + $trigger.contextMenu(false); + assert.ok($trigger.hasClass('context-menu-disabled'), 'false disables the trigger'); + $trigger.trigger($.Event('contextmenu')); + assert.equal(menu.recorder.showCalls, 1, 'a disabled trigger does not show the menu'); + + $trigger.contextMenu(true); + assert.notOk($trigger.hasClass('context-menu-disabled'), 'true re-enables the trigger'); + $trigger.trigger($.Event('contextmenu')); + assert.equal(menu.recorder.showCalls, 2, 'a re-enabled trigger shows the menu again'); + $trigger.contextMenu('hide'); + + // 'destroy' + $trigger.contextMenu('destroy'); + $trigger.trigger($.Event('contextmenu')); + assert.equal(menu.recorder.showCalls, 2, '"destroy" unregistered the menu'); +});