From 426ffa57b0c7e9f0248da7f518f2c85ce1a57a17 Mon Sep 17 00:00:00 2001 From: Alex Vinober Date: Thu, 19 Oct 2017 15:31:39 -0400 Subject: [PATCH 1/5] Restore dispatching 'click' on right-click --- src/components/dragelement/index.js | 2 +- test/jasmine/tests/dragelement_test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/dragelement/index.js b/src/components/dragelement/index.js index 2b56c42acec..551ea9ae681 100644 --- a/src/components/dragelement/index.js +++ b/src/components/dragelement/index.js @@ -104,7 +104,7 @@ dragElement.init = function init(options) { if(options.prepFn) options.prepFn(e, startX, startY); - if(hasHover) { + if(hasHover && (!e.buttons || (e.buttons && e.buttons !== 2))) { dragCover = coverSlip(); dragCover.style.cursor = window.getComputedStyle(element).cursor; } diff --git a/test/jasmine/tests/dragelement_test.js b/test/jasmine/tests/dragelement_test.js index 74326a13c58..2f8e8c1fbe3 100644 --- a/test/jasmine/tests/dragelement_test.js +++ b/test/jasmine/tests/dragelement_test.js @@ -137,7 +137,7 @@ describe('dragElement', function() { expect(countCoverSlip()).toEqual(0); mouseEvent('mouseup', this.x, this.y); - expect(mockObj.handleClick).not.toHaveBeenCalled(); + expect(mockObj.handleClick).toHaveBeenCalled(); }); it('should fire off click event when down/up without dragging', function() { From 79176406f5d5650d1f0c94c30a2668a94acdce45 Mon Sep 17 00:00:00 2001 From: Alex Vinober Date: Thu, 19 Oct 2017 15:54:20 -0400 Subject: [PATCH 2/5] Refactored --- src/components/dragelement/index.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/components/dragelement/index.js b/src/components/dragelement/index.js index 551ea9ae681..0f67bd04f76 100644 --- a/src/components/dragelement/index.js +++ b/src/components/dragelement/index.js @@ -78,10 +78,6 @@ dragElement.init = function init(options) { element.ontouchstart = onStart; function onStart(e) { - if(e.buttons && e.buttons === 2) { // right click - return; - } - // make dragging and dragged into properties of gd // so that others can look at and modify them gd._dragged = false; @@ -90,6 +86,7 @@ dragElement.init = function init(options) { startX = offset[0]; startY = offset[1]; initialTarget = e.target; + var rightClick = e.buttons && e.buttons === 2; newMouseDownTime = (new Date()).getTime(); if(newMouseDownTime - gd._mouseDownTime < DBLCLICKDELAY) { @@ -104,11 +101,11 @@ dragElement.init = function init(options) { if(options.prepFn) options.prepFn(e, startX, startY); - if(hasHover && (!e.buttons || (e.buttons && e.buttons !== 2))) { + if(hasHover && !rightClick) { dragCover = coverSlip(); dragCover.style.cursor = window.getComputedStyle(element).cursor; } - else { + else if(!hasHover) { // document acts as a dragcover for mobile, bc we can't create dragcover dynamically dragCover = document; cursor = window.getComputedStyle(document.documentElement).cursor; From 5b6a1fa381daa6feb13dfb976fae33687da18226 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Tue, 9 Jan 2018 11:10:27 -0500 Subject: [PATCH 3/5] refactor dragElement to handle clicks also fix a bunch of incorrect tests because assets/click didn't have the right behavior and document places we don't have any right-click support presently --- src/components/annotations/draw.js | 20 ++- src/components/colorbar/draw.js | 4 +- src/components/dragelement/index.js | 92 +++++++++----- src/components/legend/draw.js | 35 +++--- src/components/shapes/draw.js | 6 +- src/plots/cartesian/dragbox.js | 123 +++++++++--------- src/plots/cartesian/graph_interact.js | 4 - src/plots/cartesian/select.js | 16 ++- src/plots/geo/geo.js | 17 +-- src/plots/mapbox/mapbox.js | 11 +- src/plots/ternary/ternary.js | 52 ++++---- src/traces/pie/plot.js | 5 + test/jasmine/assets/click.js | 54 +++++++- test/jasmine/assets/mouse_event.js | 3 + test/jasmine/tests/annotations_test.js | 111 ++++++++++++----- test/jasmine/tests/click_test.js | 151 +++++++++++++++++------ test/jasmine/tests/dragelement_test.js | 68 +++++++--- test/jasmine/tests/geo_test.js | 57 +++++---- test/jasmine/tests/pie_test.js | 20 +-- test/jasmine/tests/scattermapbox_test.js | 47 ++++--- test/jasmine/tests/select_test.js | 15 +-- test/jasmine/tests/ternary_test.js | 112 ++++++++++------- 22 files changed, 645 insertions(+), 378 deletions(-) diff --git a/src/components/annotations/draw.js b/src/components/annotations/draw.js index ca035fe7750..8a5188de729 100644 --- a/src/components/annotations/draw.js +++ b/src/components/annotations/draw.js @@ -593,12 +593,10 @@ function drawRaw(gd, options, index, subplotId, xa, ya) { xcenter + ',' + ycenter + ')' }); }, - doneFn: function(dragged) { - if(dragged) { - Plotly.relayout(gd, update); - var notesBox = document.querySelector('.js-notes-box-panel'); - if(notesBox) notesBox.redraw(notesBox.selectedObj); - } + doneFn: function() { + Plotly.relayout(gd, update); + var notesBox = document.querySelector('.js-notes-box-panel'); + if(notesBox) notesBox.redraw(notesBox.selectedObj); } }); } @@ -673,13 +671,11 @@ function drawRaw(gd, options, index, subplotId, xa, ya) { setCursor(annTextGroupInner, csr); }, - doneFn: function(dragged) { + doneFn: function() { setCursor(annTextGroupInner); - if(dragged) { - Plotly.relayout(gd, update); - var notesBox = document.querySelector('.js-notes-box-panel'); - if(notesBox) notesBox.redraw(notesBox.selectedObj); - } + Plotly.relayout(gd, update); + var notesBox = document.querySelector('.js-notes-box-panel'); + if(notesBox) notesBox.redraw(notesBox.selectedObj); } }); } diff --git a/src/components/colorbar/draw.js b/src/components/colorbar/draw.js index 5c6e3876ce9..f7a78a3d26a 100644 --- a/src/components/colorbar/draw.js +++ b/src/components/colorbar/draw.js @@ -584,10 +584,10 @@ module.exports = function draw(gd, id) { opts.xanchor, opts.yanchor); setCursor(container, csr); }, - doneFn: function(dragged) { + doneFn: function() { setCursor(container); - if(dragged && xf !== undefined && yf !== undefined) { + if(xf !== undefined && yf !== undefined) { Plotly.restyle(gd, {'colorbar.x': xf, 'colorbar.y': yf}, getTrace().index); diff --git a/src/components/dragelement/index.js b/src/components/dragelement/index.js index 0f67bd04f76..4b505d3e5e5 100644 --- a/src/components/dragelement/index.js +++ b/src/components/dragelement/index.js @@ -38,24 +38,44 @@ dragElement.unhoverRaw = unhover.raw; * - Freezes the cursor: whatever mouse cursor the drag element had when the * interaction started gets copied to the coverSlip for use until mouseup * + * If the user executes a drag bigger than MINDRAG, callbacks will fire as: + * prepFn, moveFn (1 or more times), doneFn + * If the user does not drag enough, prepFn and clickFn will fire. + * + * Note: If you cancel contextmenu, clickFn will fire even with a right click + * (unlike native events) so you'll get a `plotly_click` event. Cancel context eg: + * gd.addEventListener('contextmenu', function(e) { e.preventDefault(); }); + * TODO: we should probably turn this into a `config` parameter, so we can fix it + * such that if you *don't* cancel contextmenu, we can prevent partial drags, which + * put you in a weird state. + * + * If the user clicks multiple times quickly, clickFn will fire each time + * but numClicks will increase to help you recognize doubleclicks. + * * @param {object} options with keys: * element (required) the DOM element to drag * prepFn (optional) function(event, startX, startY) * executed on mousedown * startX and startY are the clientX and clientY pixel position * of the mousedown event - * moveFn (optional) function(dx, dy, dragged) - * executed on move + * moveFn (optional) function(dx, dy) + * executed on move, ONLY after we've exceeded MINDRAG + * (we keep executing moveFn if you move back to where you started) * dx and dy are the net pixel offset of the drag, * dragged is true/false, has the mouse moved enough to * constitute a drag - * doneFn (optional) function(dragged, numClicks, e) - * executed on mouseup, or mouseout of window since - * we don't get events after that - * dragged is as in moveFn + * doneFn (optional) function(e) + * executed on mouseup, ONLY if we exceeded MINDRAG (so you can be + * sure that moveFn has been called at least once) * numClicks is how many clicks we've registered within * a doubleclick time - * e is the original event + * e is the original mouseup event + * clickFn (optional) function(numClicks, e) + * executed on mouseup if we have NOT exceeded MINDRAG (ie moveFn + * has not been called at all) + * numClicks is how many clicks we've registered within + * a doubleclick time + * e is the original mousedown event */ dragElement.init = function init(options) { var gd = options.gd; @@ -68,7 +88,9 @@ dragElement.init = function init(options) { newMouseDownTime, cursor, dragCover, - initialTarget; + initialEvent, + initialTarget, + rightClick; if(!gd._mouseDownTime) gd._mouseDownTime = 0; @@ -86,7 +108,8 @@ dragElement.init = function init(options) { startX = offset[0]; startY = offset[1]; initialTarget = e.target; - var rightClick = e.buttons && e.buttons === 2; + initialEvent = e; + rightClick = (e.buttons && e.buttons === 2) || e.ctrlKey; newMouseDownTime = (new Date()).getTime(); if(newMouseDownTime - gd._mouseDownTime < DBLCLICKDELAY) { @@ -133,7 +156,7 @@ dragElement.init = function init(options) { dragElement.unhover(gd); } - if(options.moveFn) options.moveFn(dx, dy, gd._dragged); + if(gd._dragged && options.moveFn && !rightClick) options.moveFn(dx, dy); return Lib.pauseEvent(e); } @@ -164,27 +187,36 @@ dragElement.init = function init(options) { numClicks = Math.max(numClicks - 1, 1); } - if(options.doneFn) options.doneFn(gd._dragged, numClicks, e); - - if(!gd._dragged) { - var e2; - - try { - e2 = new MouseEvent('click', e); - } - catch(err) { - var offset = pointerOffset(e); - e2 = document.createEvent('MouseEvents'); - e2.initMouseEvent('click', - e.bubbles, e.cancelable, - e.view, e.detail, - e.screenX, e.screenY, - offset[0], offset[1], - e.ctrlKey, e.altKey, e.shiftKey, e.metaKey, - e.button, e.relatedTarget); + if(gd._dragged) { + if(options.doneFn) options.doneFn(e); + } + else { + if(options.clickFn) options.clickFn(numClicks, initialEvent); + + // If we haven't dragged, this should be a click. But because of the + // coverSlip changing the element, the natural system might not generate one, + // so we need to make our own. But right clicks don't normally generate + // click events, only contextmenu events, which happen on mousedown. + if(!rightClick) { + var e2; + + try { + e2 = new MouseEvent('click', e); + } + catch(err) { + var offset = pointerOffset(e); + e2 = document.createEvent('MouseEvents'); + e2.initMouseEvent('click', + e.bubbles, e.cancelable, + e.view, e.detail, + e.screenX, e.screenY, + offset[0], offset[1], + e.ctrlKey, e.altKey, e.shiftKey, e.metaKey, + e.button, e.relatedTarget); + } + + initialTarget.dispatchEvent(e2); } - - initialTarget.dispatchEvent(e2); } finishDrag(gd); diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index 7774ec0c23e..6c6d6a54d86 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -334,25 +334,28 @@ module.exports = function draw(gd) { xf = dragElement.align(newX, 0, gs.l, gs.l + gs.w, opts.xanchor); yf = dragElement.align(newY, 0, gs.t + gs.h, gs.t, opts.yanchor); }, - doneFn: function(dragged, numClicks, e) { - if(dragged && xf !== undefined && yf !== undefined) { + doneFn: function() { + if(xf !== undefined && yf !== undefined) { Plotly.relayout(gd, {'legend.x': xf, 'legend.y': yf}); - } else { - var clickedTrace = - fullLayout._infolayer.selectAll('g.traces').filter(function() { - var bbox = this.getBoundingClientRect(); - return (e.clientX >= bbox.left && e.clientX <= bbox.right && - e.clientY >= bbox.top && e.clientY <= bbox.bottom); - }); - if(clickedTrace.size() > 0) { - if(numClicks === 1) { - legend._clickTimeout = setTimeout(function() { handleClick(clickedTrace, gd, numClicks); }, DBLCLICKDELAY); - } else if(numClicks === 2) { - if(legend._clickTimeout) { - clearTimeout(legend._clickTimeout); - } + } + }, + clickFn: function(numClicks, e) { + var clickedTrace = + fullLayout._infolayer.selectAll('g.traces').filter(function() { + var bbox = this.getBoundingClientRect(); + return (e.clientX >= bbox.left && e.clientX <= bbox.right && + e.clientY >= bbox.top && e.clientY <= bbox.bottom); + }); + if(clickedTrace.size() > 0) { + if(numClicks === 1) { + legend._clickTimeout = setTimeout(function() { handleClick(clickedTrace, gd, numClicks); + }, DBLCLICKDELAY); + } else if(numClicks === 2) { + if(legend._clickTimeout) { + clearTimeout(legend._clickTimeout); } + handleClick(clickedTrace, gd, numClicks); } } } diff --git a/src/components/shapes/draw.js b/src/components/shapes/draw.js index a67cab53c89..1af2788580f 100644 --- a/src/components/shapes/draw.js +++ b/src/components/shapes/draw.js @@ -211,11 +211,9 @@ function setupDragElement(gd, shapePath, shapeOptions, index) { dragOptions.moveFn = (dragMode === 'move') ? moveShape : resizeShape; } - function endDrag(dragged) { + function endDrag() { setCursor(shapePath); - if(dragged) { - Plotly.relayout(gd, update); - } + Plotly.relayout(gd, update); } function moveShape(dx, dy) { diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index d0d42fa6b38..ab558194871 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -18,6 +18,7 @@ var Lib = require('../../lib'); var svgTextUtils = require('../../lib/svg_text_utils'); var Color = require('../../components/color'); var Drawing = require('../../components/drawing'); +var Fx = require('../../components/fx'); var setCursor = require('../../lib/setcursor'); var dragElement = require('../../components/dragelement'); var FROM_TL = require('../../constants/alignment').FROM_TL; @@ -52,22 +53,13 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { // within DBLCLICKDELAY so we can check for click or doubleclick events // dragged stores whether a drag has occurred, so we don't have to // redraw unnecessarily, ie if no move bigger than MINDRAG or MINZOOM px - var fullLayout = gd._fullLayout, - zoomlayer = gd._fullLayout._zoomlayer, - isMainDrag = (ns + ew === 'nsew'), - subplots, - xa, - ya, - xs, - ys, - pw, - ph, - xActive, - yActive, - cursor, - isSubplotConstrained, - xaLinked, - yaLinked; + var fullLayout = gd._fullLayout; + var zoomlayer = gd._fullLayout._zoomlayer; + var isMainDrag = (ns + ew === 'nsew'); + var singleEnd = (ns + ew).length === 1; + + var subplots, xa, ya, xs, ys, pw, ph, xActive, yActive, cursor, + isSubplotConstrained, xaLinked, yaLinked; function recomputeAxisLists() { xa = [plotinfo.xaxis]; @@ -165,7 +157,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { } else if(dragModeNow === 'pan') { dragOptions.moveFn = plotDrag; - dragOptions.doneFn = dragDone; + dragOptions.doneFn = dragTail; clearSelect(zoomlayer); } else if(isSelectOrLasso(dragModeNow)) { @@ -173,6 +165,50 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { dragOptions.yaxes = ya; prepSelect(e, startX, startY, dragOptions, dragModeNow); } + }, + clickFn: function(numClicks, evt) { + removeZoombox(gd); + + if(numClicks === 2 && !singleEnd) doubleClick(); + + if(isMainDrag) { + Fx.click(gd, evt, plotinfo.id); + } + else if(numClicks === 1 && singleEnd) { + var ax = ns ? ya[0] : xa[0], + end = (ns === 's' || ew === 'w') ? 0 : 1, + attrStr = ax._name + '.range[' + end + ']', + initialText = getEndText(ax, end), + hAlign = 'left', + vAlign = 'middle'; + + if(ax.fixedrange) return; + + if(ns) { + vAlign = (ns === 'n') ? 'top' : 'bottom'; + if(ax.side === 'right') hAlign = 'right'; + } + else if(ew === 'e') hAlign = 'right'; + + if(gd._context.showAxisRangeEntryBoxes) { + d3.select(dragger) + .call(svgTextUtils.makeEditable, { + gd: gd, + immediate: true, + background: fullLayout.paper_bgcolor, + text: String(initialText), + fill: ax.tickfont ? ax.tickfont.color : '#444', + horizontalAlign: hAlign, + verticalAlign: vAlign + }) + .on('edit', function(text) { + var v = ax.d2r(text); + if(v !== undefined) { + Plotly.relayout(gd, attrStr, v); + } + }); + } + } } }; @@ -281,10 +317,10 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { dimmed = true; } - function zoomDone(dragged, numClicks) { + function zoomDone() { + // more strict than dragged, which allows you to come back to where you started + // and still count as dragged if(Math.min(box.h, box.w) < MINDRAG * 2) { - if(numClicks === 2) doubleClick(); - return removeZoombox(gd); } @@ -293,7 +329,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { if(zoomMode === 'xy' || zoomMode === 'y') zoomAxRanges(ya, (ph - box.b) / ph, (ph - box.t) / ph, updates, yaLinked); removeZoombox(gd); - dragTail(zoomMode); + dragTail(); if(SHOWZOOMOUTTIP && gd.data && gd._context.showTips) { Lib.notifier(Lib._(gd, 'Double-click to zoom back out'), 'long'); @@ -301,47 +337,6 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { } } - function dragDone(dragged, numClicks) { - var singleEnd = (ns + ew).length === 1; - if(dragged) dragTail(); - else if(numClicks === 2 && !singleEnd) doubleClick(); - else if(numClicks === 1 && singleEnd) { - var ax = ns ? ya[0] : xa[0], - end = (ns === 's' || ew === 'w') ? 0 : 1, - attrStr = ax._name + '.range[' + end + ']', - initialText = getEndText(ax, end), - hAlign = 'left', - vAlign = 'middle'; - - if(ax.fixedrange) return; - - if(ns) { - vAlign = (ns === 'n') ? 'top' : 'bottom'; - if(ax.side === 'right') hAlign = 'right'; - } - else if(ew === 'e') hAlign = 'right'; - - if(gd._context.showAxisRangeEntryBoxes) { - d3.select(dragger) - .call(svgTextUtils.makeEditable, { - gd: gd, - immediate: true, - background: fullLayout.paper_bgcolor, - text: String(initialText), - fill: ax.tickfont ? ax.tickfont.color : '#444', - horizontalAlign: hAlign, - verticalAlign: vAlign - }) - .on('edit', function(text) { - var v = ax.d2r(text); - if(v !== undefined) { - Plotly.relayout(gd, attrStr, v); - } - }); - } - } - } - // scroll zoom, on all draggers except corners var scrollViewBox = [0, 0, pw, ph]; // wait a little after scrolling before redrawing @@ -654,9 +649,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { } // dragTail - finish a drag event with a redraw - function dragTail(zoommode) { - if(zoommode === undefined) zoommode = (ew ? 'x' : '') + (ns ? 'y' : ''); - + function dragTail() { // put the subplot viewboxes back to default (Because we're going to) // be repositioning the data in the relayout. But DON'T call // ticksAndAnnotations again - it's unnecessary and would overwrite `updates` diff --git a/src/plots/cartesian/graph_interact.js b/src/plots/cartesian/graph_interact.js index 6958ba02ecc..f560819d631 100644 --- a/src/plots/cartesian/graph_interact.js +++ b/src/plots/cartesian/graph_interact.js @@ -84,10 +84,6 @@ module.exports = function initInteractions(gd) { dragElement.unhover(gd, evt); }; - maindrag.onclick = function(evt) { - Fx.click(gd, evt, subplot); - }; - // corner draggers if(gd._context.showAxisDragHandles) { dragBox(gd, plotinfo, xa._offset - DRAGGERSIZE, ya._offset - DRAGGERSIZE, diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index cf0354ae305..49619e41719 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -257,12 +257,12 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { ); }; - dragOptions.doneFn = function(dragged, numclicks) { + dragOptions.clickFn = function(numClicks) { corners.remove(); throttle.done(throttleID).then(function() { throttle.clear(throttleID); - if(!dragged && numclicks === 2) { + if(numClicks === 2) { // clear selection on doubleclick outlines.remove(); for(i = 0; i < searchTraces.length; i++) { @@ -273,9 +273,15 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { updateSelectedState(gd, searchTraces); gd.emit('plotly_deselect', null); } - else { - dragOptions.gd.emit('plotly_selected', eventData); - } + }); + }; + + dragOptions.doneFn = function() { + corners.remove(); + + throttle.done(throttleID).then(function() { + throttle.clear(throttleID); + dragOptions.gd.emit('plotly_selected', eventData); if(currentPolygon && dragOptions.polygons) { // save last polygons diff --git a/src/plots/geo/geo.js b/src/plots/geo/geo.js index c49ee130e14..c0362781854 100644 --- a/src/plots/geo/geo.js +++ b/src/plots/geo/geo.js @@ -411,19 +411,18 @@ proto.updateFx = function(fullLayout, geoLayout) { }, xaxes: [_this.xaxis], yaxes: [_this.yaxis], - subplot: _this.id + subplot: _this.id, + clickFn: function(numClicks) { + if(numClicks === 2) { + fullLayout._zoomlayer.selectAll('.select-outline').remove(); + } + } }; dragOptions.prepFn = function(e, startX, startY) { prepSelect(e, startX, startY, dragOptions, dragMode); }; - dragOptions.doneFn = function(dragged, numClicks) { - if(numClicks === 2) { - fullLayout._zoomlayer.selectAll('.select-outline').remove(); - } - }; - dragElement.init(dragOptions); } @@ -445,6 +444,10 @@ proto.updateFx = function(fullLayout, geoLayout) { }); bgRect.on('click', function() { + // TODO: like pie and mapbox, this doesn't support right-click + // actually this one is worse, as right-click starts a pan, or leaves + // select in a weird state. + // Also, only tangentially related, we should cancel hover during pan Fx.click(gd, d3.event); }); }; diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index f87c6d52d9b..608ef00ee34 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -177,6 +177,11 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { }); map.on('click', function(evt) { + // TODO: this does not support right-click. If we want to support it, we + // would likely need to change mapbox to use dragElement instead of straight + // mapbox event binding. Or perhaps better, make a simple wrapper with the + // right mousedown, mousemove, and mouseup handlers just for a left/right click + // pie would use this too. Fx.click(gd, evt.originalEvent); }); @@ -389,12 +394,6 @@ proto.updateFx = function(fullLayout) { prepSelect(e, startX, startY, dragOptions, dragMode); }; - dragOptions.doneFn = function(dragged, numClicks) { - if(numClicks === 2) { - fullLayout._zoomlayer.selectAll('.select-outline').remove(); - } - }; - dragElement.init(dragOptions); } else { map.dragPan.enable(); diff --git a/src/plots/ternary/ternary.js b/src/plots/ternary/ternary.js index 995ad34fd87..7a886fe0a1b 100644 --- a/src/plots/ternary/ternary.js +++ b/src/plots/ternary/ternary.js @@ -456,7 +456,6 @@ proto.initInteractions = function() { xaxis: _this.xaxis, yaxis: _this.yaxis }, - doubleclick: doubleClick, subplot: _this.id, prepFn: function(e, startX, startY) { // these aren't available yet when initInteractions @@ -486,6 +485,19 @@ proto.initInteractions = function() { else if(dragModeNow === 'select' || dragModeNow === 'lasso') { prepSelect(e, startX, startY, dragOptions, dragModeNow); } + }, + clickFn: function(numClicks, evt) { + removeZoombox(gd); + + if(numClicks === 2) { + var attrs = {}; + attrs[_this.id + '.aaxis.min'] = 0; + attrs[_this.id + '.baxis.min'] = 0; + attrs[_this.id + '.caxis.min'] = 0; + gd.emit('plotly_doubleclick', null); + Plotly.relayout(gd, attrs); + } + Fx.click(gd, evt, _this.id); } }; @@ -578,15 +590,11 @@ proto.initInteractions = function() { } } - function zoomDone(dragged, numClicks) { - if(mins === mins0) { - if(numClicks === 2) doubleClick(); - - return removeZoombox(gd); - } - + function zoomDone() { removeZoombox(gd); + if(mins === mins0) return; + var attrs = {}; attrs[_this.id + '.aaxis.min'] = mins.a; attrs[_this.id + '.baxis.min'] = mins.b; @@ -665,16 +673,13 @@ proto.initInteractions = function() { } } - function dragDone(dragged, numClicks) { - if(dragged) { - var attrs = {}; - attrs[_this.id + '.aaxis.min'] = mins.a; - attrs[_this.id + '.baxis.min'] = mins.b; - attrs[_this.id + '.caxis.min'] = mins.c; + function dragDone() { + var attrs = {}; + attrs[_this.id + '.aaxis.min'] = mins.a; + attrs[_this.id + '.baxis.min'] = mins.b; + attrs[_this.id + '.caxis.min'] = mins.c; - Plotly.relayout(gd, attrs); - } - else if(numClicks === 2) doubleClick(); + Plotly.relayout(gd, attrs); } function clearSelect() { @@ -684,15 +689,6 @@ proto.initInteractions = function() { zoomContainer.selectAll('.select-outline').remove(); } - function doubleClick() { - var attrs = {}; - attrs[_this.id + '.aaxis.min'] = 0; - attrs[_this.id + '.baxis.min'] = 0; - attrs[_this.id + '.caxis.min'] = 0; - gd.emit('plotly_doubleclick', null); - Plotly.relayout(gd, attrs); - } - // finally, set up hover and click // these event handlers must already be set before dragElement.init // so it can stash them and override them. @@ -708,10 +704,6 @@ proto.initInteractions = function() { dragElement.unhover(gd, evt); }; - dragger.onclick = function(evt) { - Fx.click(gd, evt, _this.id); - }; - dragElement.init(dragOptions); }; diff --git a/src/traces/pie/plot.js b/src/traces/pie/plot.js index 65096dbbf78..c0c895324d0 100644 --- a/src/traces/pie/plot.js +++ b/src/traces/pie/plot.js @@ -170,6 +170,11 @@ module.exports = function plot(gd, cdpie) { } function handleClick() { + // TODO: this does not support right-click. If we want to support it, we + // would likely need to change mapbox to use dragElement instead of straight + // mapbox event binding. Or perhaps better, make a simple wrapper with the + // right mousedown, mousemove, and mouseup handlers just for a left/right click + // mapbox would use this too. var fullLayout2 = gd._fullLayout; var trace2 = gd._fullData[trace.index]; diff --git a/test/jasmine/assets/click.js b/test/jasmine/assets/click.js index a1dff1f1d6f..c9986e6e8e3 100644 --- a/test/jasmine/assets/click.js +++ b/test/jasmine/assets/click.js @@ -1,8 +1,52 @@ var mouseEvent = require('./mouse_event'); +var Lib = require('../../../src/lib'); -module.exports = function click(x, y, opts) { - mouseEvent('mousemove', x, y, opts); - mouseEvent('mousedown', x, y, opts); - mouseEvent('mouseup', x, y, opts); - mouseEvent('click', x, y, opts); +/* + * simulated click event at screen pixel position x, y + * + * @param {number} x: x pixel position on the page (clientX) + * @param {number} y: y pixel position on the page (clientY) + * + * @param {object} opts: optional event options + * @param {bool} opts.altKey - was alt/option pressed during this click? + * @param {bool} opts.ctrlKey - was ctrl pressed during this click? + * @param {bool} opts.metaKey - was meta/command pressed during this click? + * @param {bool} opts.shiftKey - was shift pressed during this click? + * @param {number} opts.button: the button used for the click. + * Do NOT supply `opts.buttons`, it will be automatically generated to match. + * The events generated and button/buttons for each will be tailored to match real events. + * 0 or missing - left button + * 2 - right button + * anything else - we don't use other buttons, so throw an error if we test with them. + * @param {bool} cancelContext: act as though `preventDefault` was called during a `contextmenu` + * handler, which stops native contextmenu and therefore allows mouseup events to be fired. + * Only relevant if button=2 or ctrlKey=true. + */ +module.exports = function click(x, y, optsIn) { + var opts = Lib.extendFlat({}, optsIn || {}); + var button = opts.button || 0; + if(button && button !== 2) throw new Error('unsupported button: ' + button); + if(opts.buttons !== undefined) throw new Error('do not supply opts.buttons'); + var buttons = button ? 2 : 1; + + // TODO: this is the behavior I observe (Chrome 63 Mac) but we should verify that it's consistent: + // - `buttons` is 0 for mouseUp and click events, but you get `button` unchanged + // - ctrlKey or right button triggers contextMenu + // - you only get a mouseup after contextmenu if you preventDefault on the contextmenu + // - you still don't get a `click` event after contextmenu + var rightClick = (button === 2) || opts.ctrlKey; + var callContext = rightClick && !opts.cancelContext; + delete opts.cancelContext; + + var moveOpts = Lib.extendFlat({}, opts); + delete moveOpts.button; + + var downOpts = Lib.extendFlat({buttons: buttons}, opts); + var upOpts = opts; + + mouseEvent('mousemove', x, y, moveOpts); + mouseEvent('mousedown', x, y, downOpts); + if(callContext) mouseEvent('contextmenu', x, y, downOpts); + if(!callContext) mouseEvent('mouseup', x, y, upOpts); + if(!rightClick) mouseEvent('click', x, y, upOpts); }; diff --git a/test/jasmine/assets/mouse_event.js b/test/jasmine/assets/mouse_event.js index 42154e5dde9..5ae9356d0b0 100644 --- a/test/jasmine/assets/mouse_event.js +++ b/test/jasmine/assets/mouse_event.js @@ -8,6 +8,9 @@ module.exports = function(type, x, y, opts) { }; // https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent + if(opts && opts.button) { + fullOpts.button = opts.button; + } if(opts && opts.buttons) { fullOpts.buttons = opts.buttons; } diff --git a/test/jasmine/tests/annotations_test.js b/test/jasmine/tests/annotations_test.js index babbe0da795..4a70710dece 100644 --- a/test/jasmine/tests/annotations_test.js +++ b/test/jasmine/tests/annotations_test.js @@ -1183,8 +1183,44 @@ describe('annotation effects', function() { .then(done); }); + function _click(pos, opts) { + return new Promise(function(resolve) { + click(pos[0], pos[1], opts); + + setTimeout(function() { + resolve(); + }, DBLCLICKDELAY * 1.1); + }); + } + + var pos0Head, pos0, pos1, pos2Head, pos2, clickData; + + function assertClickData(data) { + expect(clickData.length).toBe(data.length); + expect(clickData).toEqual(data); + clickData.splice(0, clickData.length); + } + + function initClickTests() { + var gdBB = gd.getBoundingClientRect(); + pos0Head = [gdBB.left + 200, gdBB.top + 200]; + pos0 = [pos0Head[0], pos0Head[1] - 40]; + pos1 = [gdBB.left + 160, gdBB.top + 340]; + pos2Head = [gdBB.left + 340, gdBB.top + 160]; + pos2 = [pos2Head[0], pos2Head[1] - 40]; + + clickData = []; + + gd.on('plotly_clickannotation', function(evt) { + expect(evt.event).toEqual(jasmine.objectContaining({type: 'click'})); + evt.button = evt.event.button; + if(evt.event.ctrlKey) evt.ctrlKey = true; + delete evt.event; + clickData.push(evt); + }); + } + it('should register clicks and show hover effects on the text box only', function(done) { - var gdBB, pos0Head, pos0, pos1, pos2Head, pos2, clickData; function assertHoverLabel(pos, text, msg) { return new Promise(function(resolve) { @@ -1225,44 +1261,17 @@ describe('annotation effects', function() { return p; } - function _click(pos) { - return new Promise(function(resolve) { - click(pos[0], pos[1]); - - setTimeout(function() { - resolve(); - }, DBLCLICKDELAY * 1.1); - }); - } - - function assertClickData(data) { - expect(clickData).toEqual(data); - clickData.splice(0, clickData.length); - } - makePlot([ {x: 50, y: 50, text: 'hi', width: 50, height: 40, ax: 0, ay: -40, xshift: -50, yshift: 50}, {x: 20, y: 20, text: 'bye', height: 40, showarrow: false}, {x: 80, y: 80, text: 'why?', ax: 0, ay: -40} ], {}) // turn off the default editable: true .then(function() { - clickData = []; - gd.on('plotly_clickannotation', function(evt) { - expect(evt.event).toEqual(jasmine.objectContaining({type: 'click'})); - delete evt.event; - clickData.push(evt); - }); - - gdBB = gd.getBoundingClientRect(); - pos0Head = [gdBB.left + 200, gdBB.top + 200]; - pos0 = [pos0Head[0], pos0Head[1] - 40]; - pos1 = [gdBB.left + 160, gdBB.top + 340]; - pos2Head = [gdBB.left + 340, gdBB.top + 160]; - pos2 = [pos2Head[0], pos2Head[1] - 40]; + initClickTests(); return assertHoverLabels([[pos0, ''], [pos1, ''], [pos2, '']]); }) - // not going to register either of these because captureevents is off + // not going to register any of these because captureevents is off .then(function() { return _click(pos1); }) .then(function() { return _click(pos2Head); }) .then(function() { @@ -1281,7 +1290,8 @@ describe('annotation effects', function() { assertClickData([{ index: 1, annotation: gd.layout.annotations[1], - fullAnnotation: gd._fullLayout.annotations[1] + fullAnnotation: gd._fullLayout.annotations[1], + button: 0 }]); expect(gd._fullLayout.annotations[0].hoverlabel).toBeUndefined(); @@ -1305,7 +1315,8 @@ describe('annotation effects', function() { assertClickData([{ index: 0, annotation: gd.layout.annotations[0], - fullAnnotation: gd._fullLayout.annotations[0] + fullAnnotation: gd._fullLayout.annotations[0], + button: 0 }]); return Plotly.relayout(gd, { @@ -1331,6 +1342,42 @@ describe('annotation effects', function() { .then(done); }); + // Currently annotations do *not* support right-click. + // TODO: if we integrated this with dragElement rather than + // annTextGroupInner.on('click') they could support right-click. + it('does not collect right-click or ctrl-click', function(done) { + var rightClick = {button: 2, cancelContext: true}; + var ctrlClick = {ctrlKey: true, cancelContext: true}; + makePlot([ + {x: 50, y: 50, text: 'hi', width: 50, height: 40, ax: 0, ay: -40, xshift: -50, yshift: 50, hovertext: 'bananas'}, + {x: 20, y: 20, text: 'bye', height: 40, showarrow: false, captureevents: true}, + {x: 80, y: 80, text: 'why?', ax: 0, ay: -40, captureevents: true} + ], {}) // turn off the default editable: true + .then(initClickTests) + .then(function() { return _click(pos1, rightClick); }) + .then(function() { return _click(pos2Head, rightClick); }) + .then(function() { return _click(pos1, ctrlClick); }) + .then(function() { return _click(pos2Head, ctrlClick); }) + .then(function() { return _click(pos0, rightClick); }) + .then(function() { return _click(pos0, ctrlClick); }) + .then(function() { + assertClickData([]); + + // sanity check that we still get left clicks + return _click(pos1); + }) + .then(function() { + assertClickData([{ + index: 1, + annotation: gd.layout.annotations[1], + fullAnnotation: gd._fullLayout.annotations[1], + button: 0 + }]); + }) + .catch(failTest) + .then(done); + }); + it('makes the whole text box a link if the link is the whole text', function(done) { makePlot([ {x: 20, y: 20, text: 'Plot', showarrow: false}, diff --git a/test/jasmine/tests/click_test.js b/test/jasmine/tests/click_test.js index 0841b5ce895..0b77b01715a 100644 --- a/test/jasmine/tests/click_test.js +++ b/test/jasmine/tests/click_test.js @@ -45,11 +45,12 @@ describe('Test click interactions:', function() { var mockCopy, gd; - var pointPos = [344, 216], - blankPos = [63, 356]; + var pointPos = [344, 216]; + var blankPos = [63, 356]; + var marginPos = [100, 50]; - var autoRangeX = [-3.011967491973726, 2.1561305597186564], - autoRangeY = [-0.9910086301469277, 1.389382716298284]; + var autoRangeX = [-3.011967491973726, 2.1561305597186564]; + var autoRangeY = [-0.9910086301469277, 1.389382716298284]; beforeEach(function() { gd = createGraphDiv(); @@ -65,24 +66,62 @@ describe('Test click interactions:', function() { } describe('click events', function() { - var futureData; + var futureData, clickPassthroughs, contextPassthroughs; beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; + clickPassthroughs = 0; + contextPassthroughs = 0; + gd.on('plotly_click', function(data) { futureData = data; }); + + gd.addEventListener('click', function() { + clickPassthroughs++; + }); + gd.addEventListener('contextmenu', function() { + contextPassthroughs++; + }); }); - it('should not be trigged when not on data points', function() { + // Later we want to emit plotly events for clicking in the graph but not on data + // showing the axis values you clicked on. But at the moment these events + // pass through to event handlers attached to gd. + it('should not be triggered when not on data points', function() { click(blankPos[0], blankPos[1]); expect(futureData).toBe(undefined); + // this is a weird one - in the real case the original click never + // happens, it gets canceled by preventDefault in mouseup, but we + // add our own synthetic click. + // But assets/click doesn't know not to generate a click event, so + // here we get both. I don't see a good way to avoid this, but also + // there's something nice about this showing that we do indeed + // generate the synthetic click event. + // TODO: do we actually want this synthetic event, now that dragElement + // has `clickFn` to explicitly manage clicks too? Perhaps leave it in + // for now, in case either 1) users want to catch all clicks on gd, or + // 2) we have a component still using on('click') instead of `clickFn` + expect(clickPassthroughs).toBe(2); + expect(contextPassthroughs).toBe(0); + }); + + // Margin clicks will probably always pass through to gd, right? + // Any reason we should handle these? + it('should not be triggered when in the margin', function() { + click(marginPos[0], marginPos[1]); + expect(futureData).toBe(undefined); + expect(clickPassthroughs).toBe(1); + expect(contextPassthroughs).toBe(0); }); it('should contain the correct fields', function() { click(pointPos[0], pointPos[1]); expect(futureData.points.length).toEqual(1); + expect(clickPassthroughs).toBe(2); + expect(contextPassthroughs).toBe(0); var pt = futureData.points[0]; expect(Object.keys(pt)).toEqual([ @@ -98,49 +137,79 @@ describe('Test click interactions:', function() { expect(evt.clientX).toEqual(pointPos[0]); expect(evt.clientY).toEqual(pointPos[1]); }); - }); - describe('modified click events', function() { - var clickOpts = { - altKey: true, - ctrlKey: true, - metaKey: true, - shiftKey: true - }, - futureData; + var modClickOpts = { + altKey: true, + ctrlKey: true, // this makes it effectively into a right-click + metaKey: true, + shiftKey: true, + button: 0, + cancelContext: true + }; + var rightClickOpts = { + altKey: false, + ctrlKey: false, + metaKey: false, + shiftKey: false, + button: 2, + cancelContext: true + }; - beforeEach(function(done) { - Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + [modClickOpts, rightClickOpts].forEach(function(clickOpts, i) { + it('should not be triggered when not on data points', function() { + click(blankPos[0], blankPos[1], clickOpts); + expect(futureData === undefined).toBe(true, i); + expect(clickPassthroughs).toBe(0, i); + expect(contextPassthroughs).toBe(0, i); + }); - gd.on('plotly_click', function(data) { - futureData = data; + it('should not be triggered when in the margin', function() { + click(marginPos[0], marginPos[1], clickOpts); + expect(futureData === undefined).toBe(true, i); + expect(clickPassthroughs).toBe(0, i); + expect(contextPassthroughs).toBe(0, i); }); - }); - it('should not be trigged when not on data points', function() { - click(blankPos[0], blankPos[1], clickOpts); - expect(futureData).toBe(undefined); - }); + it('should not be triggered if you dont cancel contextmenu', function() { + click(pointPos[0], pointPos[1], Lib.extendFlat({}, clickOpts, {cancelContext: false})); + expect(futureData === undefined).toBe(true, i); + expect(clickPassthroughs).toBe(0, i); + expect(contextPassthroughs).toBe(1, i); + }); - it('should contain the correct fields', function() { - click(pointPos[0], pointPos[1], clickOpts); - expect(futureData.points.length).toEqual(1); + // Testing the specific behavior that some users depend on + // See https://github.com/plotly/plotly.js/issues/2101 + // If and only if you cancel contextmenu, by doing something like: + // + // gd.addEventListener('contextmenu', function(e) { e.preventDefault(); }) + // + // then we pass right-click on data points through to plotly_click events. + // Devs using this need to be aware then to check eventData.event + // and figure out if it had a button or ctrlKey etc. + it('should contain the correct fields', function() { + click(pointPos[0], pointPos[1], clickOpts); + expect(futureData.points.length).toBe(1, i); + expect(clickPassthroughs).toBe(0, i); + expect(contextPassthroughs).toBe(0, i); - var pt = futureData.points[0]; - expect(Object.keys(pt)).toEqual([ - 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', - 'x', 'y', 'xaxis', 'yaxis' - ]); - expect(pt.curveNumber).toEqual(0); - expect(pt.pointNumber).toEqual(11); - expect(pt.x).toEqual(0.125); - expect(pt.y).toEqual(2.125); + var pt = futureData.points[0]; + expect(Object.keys(pt)).toEqual([ + 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', + 'x', 'y', 'xaxis', 'yaxis' + ]); + expect(pt.curveNumber).toEqual(0); + expect(pt.pointNumber).toEqual(11); + expect(pt.x).toEqual(0.125); + expect(pt.y).toEqual(2.125); - var evt = futureData.event; - expect(evt.clientX).toEqual(pointPos[0]); - expect(evt.clientY).toEqual(pointPos[1]); - Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { - expect(evt[opt]).toEqual(clickOpts[opt], opt); + var evt = futureData.event; + expect(evt.clientX).toEqual(pointPos[0]); + expect(evt.clientY).toEqual(pointPos[1]); + Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { + if(opt !== 'cancelContext') { + expect(evt[opt]).toBe(clickOpts[opt], opt + ': ' + i); + } + }); }); }); }); diff --git a/test/jasmine/tests/dragelement_test.js b/test/jasmine/tests/dragelement_test.js index 2f8e8c1fbe3..420fa06d6f9 100644 --- a/test/jasmine/tests/dragelement_test.js +++ b/test/jasmine/tests/dragelement_test.js @@ -61,13 +61,13 @@ describe('dragElement', function() { expect(args[2]).toEqual(Math.floor(this.y)); }); - it('should pass dragged, dx and dy to moveFn on mousemove', function() { + it('should pass dx and dy to moveFn on mousemove', function() { var args = []; var options = { element: this.element, gd: this.gd, - moveFn: function(dx, dy, dragged) { - args = [dx, dy, dragged]; + moveFn: function() { + args = arguments; } }; dragElement.init(options); @@ -76,34 +76,66 @@ describe('dragElement', function() { mouseEvent('mousemove', this.x + 10, this.y + 10); mouseEvent('mouseup', this.x, this.y); + expect(args.length).toBe(2); expect(args[0]).toEqual(10); expect(args[1]).toEqual(10); - expect(args[2]).toBe(true); }); - it('should pass dragged and numClicks to doneFn on mouseup', function() { + it('should pass the event to doneFn on mouseup after mousemove', function() { var args = []; var options = { element: this.element, gd: this.gd, - doneFn: function(dragged, numClicks) { - args = [dragged, numClicks]; + doneFn: function() { + args = arguments; + }, + clickFn: function() { + expect('should not call clickFn').toBe(true); } }; dragElement.init(options); mouseEvent('mousedown', this.x, this.y); + mouseEvent('mousemove', this.x + 10, this.y + 10); mouseEvent('mouseup', this.x, this.y); - expect(args[0]).toBe(false); - expect(args[1]).toEqual(1); + expect(args.length).toBe(1); + expect(args[0].type).toBe('mouseup'); + }); + + it('should pass numClicks and event to clickFn on mouseup after no/small mousemove', function() { + var args = []; + var options = { + element: this.element, + gd: this.gd, + clickFn: function() { + args = arguments; + }, + moveFn: function() { + expect('should not call moveFn').toBe(true); + }, + doneFn: function() { + expect('should not call doneFn').toBe(true); + } + }; + dragElement.init(options); mouseEvent('mousedown', this.x, this.y); - mouseEvent('mousemove', this.x + 10, this.y + 10); mouseEvent('mouseup', this.x, this.y); - expect(args[0]).toBe(true); - expect(args[1]).toEqual(2); + expect(args.length).toBe(2); + expect(args[0]).toEqual(1); + // click gets the mousedown event, as that's guaranteed to have + // the correct target + expect(args[1].type).toBe('mousedown'); + + mouseEvent('mousedown', this.x, this.y); + mouseEvent('mousemove', this.x + 3, this.y + 3); + mouseEvent('mouseup', this.x, this.y); + + expect(args.length).toBe(2); + expect(args[0]).toEqual(2); + expect(args[1].type).toBe('mousedown'); }); it('should add a cover slip div to the DOM', function() { @@ -123,7 +155,14 @@ describe('dragElement', function() { }); it('should not add a cover slip div to the DOM when right click', function() { - var options = { element: this.element, gd: this.gd }; + var clickCalls = 0; + var options = { + element: this.element, + gd: this.gd, + clickFn: function() { + clickCalls++; + } + }; dragElement.init(options); var mockObj = { @@ -137,7 +176,8 @@ describe('dragElement', function() { expect(countCoverSlip()).toEqual(0); mouseEvent('mouseup', this.x, this.y); - expect(mockObj.handleClick).toHaveBeenCalled(); + expect(mockObj.handleClick).not.toHaveBeenCalled(); + expect(clickCalls).toBe(1); }); it('should fire off click event when down/up without dragging', function() { diff --git a/test/jasmine/tests/geo_test.js b/test/jasmine/tests/geo_test.js index db0b9c71253..1f3e1829dbc 100644 --- a/test/jasmine/tests/geo_test.js +++ b/test/jasmine/tests/geo_test.js @@ -1287,6 +1287,7 @@ describe('Test event property of interactions on a geo plot:', function() { beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; gd.on('plotly_click', function(data) { futureData = data; }); @@ -1336,6 +1337,7 @@ describe('Test event property of interactions on a geo plot:', function() { beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; gd.on('plotly_click', function(data) { futureData = data; }); @@ -1346,33 +1348,38 @@ describe('Test event property of interactions on a geo plot:', function() { expect(futureData).toBe(undefined); }); - it('should contain the correct fields', function() { + it('does not support right-click', function() { click(pointPos[0], pointPos[1], clickOpts); + expect(futureData).toBe(undefined); - var pt = futureData.points[0], - evt = futureData.event; - - expect(Object.keys(pt)).toEqual([ - 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', - 'lon', 'lat', - 'location', 'text', 'marker.size' - ]); - - expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); - expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); - expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); - expect(pt.lat).toEqual(57.75, 'points[0].lat'); - expect(pt.lon).toEqual(-101.57, 'points[0].lon'); - expect(pt.location).toEqual('CAN', 'points[0].location'); - expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); - expect(pt.text).toEqual(20, 'points[0].text'); - expect(pt['marker.size']).toEqual(20, 'points[0][\'marker.size\']'); - - expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); - expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); - Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { - expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); - }); + // TODO: 'should contain the correct fields' + // This test passed previously, but only because assets/click + // incorrectly generated a click event for right click. It never + // worked in reality. + // var pt = futureData.points[0], + // evt = futureData.event; + + // expect(Object.keys(pt)).toEqual([ + // 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', + // 'lon', 'lat', + // 'location', 'text', 'marker.size' + // ]); + + // expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); + // expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); + // expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); + // expect(pt.lat).toEqual(57.75, 'points[0].lat'); + // expect(pt.lon).toEqual(-101.57, 'points[0].lon'); + // expect(pt.location).toEqual('CAN', 'points[0].location'); + // expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); + // expect(pt.text).toEqual(20, 'points[0].text'); + // expect(pt['marker.size']).toEqual(20, 'points[0][\'marker.size\']'); + + // expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); + // expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); + // Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { + // expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); + // }); }); }); diff --git a/test/jasmine/tests/pie_test.js b/test/jasmine/tests/pie_test.js index a1e688e2467..91aeab8422d 100644 --- a/test/jasmine/tests/pie_test.js +++ b/test/jasmine/tests/pie_test.js @@ -477,16 +477,22 @@ describe('Test event data of interactions on a pie plot:', function() { expect(futureData).toBe(undefined); }); - it('should contain the correct fields', function() { + it('does not respond to right-click', function() { click(pointPos[0], pointPos[1], clickOpts); - expect(futureData.points.length).toEqual(1); + expect(futureData).toBe(undefined); - checkEventData(futureData); + // TODO: 'should contain the correct fields' + // This test passed previously, but only because assets/click + // incorrectly generated a click event for right click. It never + // worked in reality. + // expect(futureData.points.length).toEqual(1); - var evt = futureData.event; - Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { - expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); - }); + // checkEventData(futureData); + + // var evt = futureData.event; + // Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { + // expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); + // }); }); }); diff --git a/test/jasmine/tests/scattermapbox_test.js b/test/jasmine/tests/scattermapbox_test.js index bc355973a2e..ccc6dbef587 100644 --- a/test/jasmine/tests/scattermapbox_test.js +++ b/test/jasmine/tests/scattermapbox_test.js @@ -681,6 +681,7 @@ describe('@noCI Test plotly events on a scattermapbox plot:', function() { beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; gd.on('plotly_click', function(data) { futureData = data; }); @@ -725,6 +726,7 @@ describe('@noCI Test plotly events on a scattermapbox plot:', function() { beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; gd.on('plotly_click', function(data) { futureData = data; }); @@ -735,28 +737,33 @@ describe('@noCI Test plotly events on a scattermapbox plot:', function() { expect(futureData).toBe(undefined); }); - it('should contain the correct fields', function() { + it('does not register right-clicks', function() { click(pointPos[0], pointPos[1], clickOpts); + expect(futureData).toBe(undefined); - var pt = futureData.points[0], - evt = futureData.event; - - expect(Object.keys(pt)).toEqual([ - 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', 'lon', 'lat' - ]); - - expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); - expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); - expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); - expect(pt.lat).toEqual(10, 'points[0].lat'); - expect(pt.lon).toEqual(10, 'points[0].lon'); - expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); - - expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); - expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); - Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { - expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); - }); + // TODO: 'should contain the correct fields' + // This test passed previously, but only because assets/click + // incorrectly generated a click event for right click. It never + // worked in reality. + // var pt = futureData.points[0], + // evt = futureData.event; + + // expect(Object.keys(pt)).toEqual([ + // 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', 'lon', 'lat' + // ]); + + // expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); + // expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); + // expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); + // expect(pt.lat).toEqual(10, 'points[0].lat'); + // expect(pt.lon).toEqual(10, 'points[0].lon'); + // expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); + + // expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); + // expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); + // Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { + // expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); + // }); }); }); diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 0010264a62e..2e314debda0 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -98,11 +98,11 @@ function assertEventCounts(selecting, selected, deselect, msg) { // events for box or lasso select mouse moves then a doubleclick var NOEVENTS = [0, 0, 0]; -// deselect gives an extra plotly_selected event on the first click -// event data is undefined -var BOXEVENTS = [1, 2, 1]; +// deselect used to give an extra plotly_selected event on the first click +// with undefined event data - but now that's gone, since `clickFn` handles this. +var BOXEVENTS = [1, 1, 1]; // assumes 5 points in the lasso path -var LASSOEVENTS = [4, 2, 1]; +var LASSOEVENTS = [4, 1, 1]; describe('Test select box and lasso in general:', function() { var mock = require('@mocks/14.json'); @@ -643,15 +643,12 @@ describe('Test select box and lasso per trace:', function() { return (eventCounts[0] ? selectedPromise : Promise.resolve()) .then(afterDragFn) .then(function() { - // before dblclick we also have one less plotly_selected event - // because the first click of the dblclick emits plotly_selected - // even though it does so with undefined event data. - assertEventCounts(eventCounts[0], Math.max(0, eventCounts[1] - 1), 0, msg); + assertEventCounts(eventCounts[0], eventCounts[1], 0, msg + ' (before dblclick)'); return doubleClick(dblClickPos[0], dblClickPos[1]); }) .then(eventCounts[2] ? deselectPromise : Promise.resolve()) .then(function() { - assertEventCounts(eventCounts[0], eventCounts[1], eventCounts[2], msg); + assertEventCounts(eventCounts[0], eventCounts[1], eventCounts[2], msg + ' (after dblclick)'); }); } diff --git a/test/jasmine/tests/ternary_test.js b/test/jasmine/tests/ternary_test.js index ebed92f9dd0..561d94c530f 100644 --- a/test/jasmine/tests/ternary_test.js +++ b/test/jasmine/tests/ternary_test.js @@ -475,6 +475,8 @@ describe('Test event property of interactions on a ternary plot:', function() { beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; + gd.on('plotly_click', function(data) { futureData = data; }); @@ -496,68 +498,90 @@ describe('Test event property of interactions on a ternary plot:', function() { 'xaxis', 'yaxis', 'a', 'b', 'c' ]); - expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); - expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); - expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); - expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); - expect(typeof pt.xaxis).toEqual(typeof {}, 'points[0].xaxis'); - expect(typeof pt.yaxis).toEqual(typeof {}, 'points[0].yaxis'); - expect(pt.a).toEqual(0.5, 'points[0].a'); - expect(pt.b).toEqual(0.25, 'points[0].b'); - expect(pt.c).toEqual(0.25, 'points[0].c'); - - expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); - expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); + expect(pt.curveNumber).toBe(0, 'points[0].curveNumber'); + expect(typeof pt.data).toBe(typeof {}, 'points[0].data'); + expect(typeof pt.fullData).toBe(typeof {}, 'points[0].fullData'); + expect(pt.pointNumber).toBe(0, 'points[0].pointNumber'); + expect(typeof pt.xaxis).toBe(typeof {}, 'points[0].xaxis'); + expect(typeof pt.yaxis).toBe(typeof {}, 'points[0].yaxis'); + expect(pt.a).toBe(0.5, 'points[0].a'); + expect(pt.b).toBe(0.25, 'points[0].b'); + expect(pt.c).toBe(0.25, 'points[0].c'); + + expect(evt.clientX).toBe(pointPos[0], 'event.clientX'); + expect(evt.clientY).toBe(pointPos[1], 'event.clientY'); }); }); describe('modified click events', function() { - var clickOpts = { - altKey: true, - ctrlKey: true, - metaKey: true, - shiftKey: true - }, - futureData; + var futureData; beforeEach(function(done) { Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + futureData = undefined; + gd.on('plotly_click', function(data) { futureData = data; }); }); - it('should not be trigged when not on data points', function() { - click(blankPos[0], blankPos[1], clickOpts); - expect(futureData).toBe(undefined); - }); + var modClickOpts = { + altKey: true, + ctrlKey: true, // this makes it effectively into a right-click + metaKey: true, + shiftKey: true, + button: 0, + cancelContext: true + }; + var rightClickOpts = { + altKey: false, + ctrlKey: false, + metaKey: false, + shiftKey: false, + button: 2, + cancelContext: true + }; - it('should contain the correct fields', function() { - click(pointPos[0], pointPos[1], clickOpts); + [modClickOpts, rightClickOpts].forEach(function(clickOpts, i) { + it('should not be triggered when not on data points', function() { + click(blankPos[0], blankPos[1], clickOpts); + expect(futureData === undefined).toBe(true, i); + }); - var pt = futureData.points[0], - evt = futureData.event; + it('should not be triggered when not canceling context', function() { + click(pointPos[0], pointPos[1], Lib.extendFlat({}, clickOpts, {cancelContext: false})); + expect(futureData === undefined).toBe(true, i); + }); - expect(Object.keys(pt)).toEqual([ - 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', - 'xaxis', 'yaxis', 'a', 'b', 'c' - ]); + it('should contain the correct fields', function() { + click(pointPos[0], pointPos[1], clickOpts); - expect(pt.curveNumber).toEqual(0, 'points[0].curveNumber'); - expect(typeof pt.data).toEqual(typeof {}, 'points[0].data'); - expect(typeof pt.fullData).toEqual(typeof {}, 'points[0].fullData'); - expect(pt.pointNumber).toEqual(0, 'points[0].pointNumber'); - expect(typeof pt.xaxis).toEqual(typeof {}, 'points[0].xaxis'); - expect(typeof pt.yaxis).toEqual(typeof {}, 'points[0].yaxis'); - expect(pt.a).toEqual(0.5, 'points[0].a'); - expect(pt.b).toEqual(0.25, 'points[0].b'); - expect(pt.c).toEqual(0.25, 'points[0].c'); + var pt = futureData.points[0]; + var evt = futureData.event; - expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); - expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); - Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { - expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); + expect(Object.keys(pt)).toEqual([ + 'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', + 'xaxis', 'yaxis', 'a', 'b', 'c' + ]); + + expect(pt.curveNumber).toBe(0, 'points[0].curveNumber: ' + i); + expect(typeof pt.data).toBe(typeof {}, 'points[0].data: ' + i); + expect(typeof pt.fullData).toBe(typeof {}, 'points[0].fullData: ' + i); + expect(pt.pointNumber).toBe(0, 'points[0].pointNumber: ' + i); + expect(typeof pt.xaxis).toBe(typeof {}, 'points[0].xaxis: ' + i); + expect(typeof pt.yaxis).toBe(typeof {}, 'points[0].yaxis: ' + i); + expect(pt.a).toBe(0.5, 'points[0].a: ' + i); + expect(pt.b).toBe(0.25, 'points[0].b: ' + i); + expect(pt.c).toBe(0.25, 'points[0].c: ' + i); + + expect(evt.clientX).toBe(pointPos[0], 'event.clientX: ' + i); + expect(evt.clientY).toBe(pointPos[1], 'event.clientY: ' + i); + Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { + if(opt !== 'cancelContext') { + expect(evt[opt]).toBe(clickOpts[opt], 'event.' + opt + ': ' + i); + } + }); }); }); }); From 149dd742b04311d2057b48e39ca84564213091ac Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 10 Jan 2018 09:52:40 -0700 Subject: [PATCH 4/5] tyop --- src/traces/pie/plot.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/traces/pie/plot.js b/src/traces/pie/plot.js index c0c895324d0..dc755080378 100644 --- a/src/traces/pie/plot.js +++ b/src/traces/pie/plot.js @@ -171,7 +171,7 @@ module.exports = function plot(gd, cdpie) { function handleClick() { // TODO: this does not support right-click. If we want to support it, we - // would likely need to change mapbox to use dragElement instead of straight + // would likely need to change pie to use dragElement instead of straight // mapbox event binding. Or perhaps better, make a simple wrapper with the // right mousedown, mousemove, and mouseup handlers just for a left/right click // mapbox would use this too. From 974cebf4e6f596d4fde8c84053aef43100b2ea10 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 10 Jan 2018 10:48:08 -0700 Subject: [PATCH 5/5] put back in the plotly_selected->undefined event to be removed for good in v2 --- src/plots/cartesian/select.js | 5 +++++ test/jasmine/tests/select_test.js | 13 ++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 49619e41719..55bcd1a958a 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -273,6 +273,11 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { updateSelectedState(gd, searchTraces); gd.emit('plotly_deselect', null); } + else { + // TODO: remove in v2 - this was probably never intended to work as it does, + // but in case anyone depends on it we don't want to break it now. + gd.emit('plotly_selected', undefined); + } }); }; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 2e314debda0..9c1e33975df 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -96,13 +96,16 @@ function assertEventCounts(selecting, selected, deselect, msg) { expect(deselectCnt).toBe(deselect, 'plotly_deselect call count: ' + msg); } +// TODO: in v2, when we get rid of the `plotly_selected->undefined` event, these will +// change to BOXEVENTS = [1, 1, 1], LASSOEVENTS = [4, 1, 1]. See also _run down below +// // events for box or lasso select mouse moves then a doubleclick var NOEVENTS = [0, 0, 0]; // deselect used to give an extra plotly_selected event on the first click // with undefined event data - but now that's gone, since `clickFn` handles this. -var BOXEVENTS = [1, 1, 1]; +var BOXEVENTS = [1, 2, 1]; // assumes 5 points in the lasso path -var LASSOEVENTS = [4, 1, 1]; +var LASSOEVENTS = [4, 2, 1]; describe('Test select box and lasso in general:', function() { var mock = require('@mocks/14.json'); @@ -643,7 +646,11 @@ describe('Test select box and lasso per trace:', function() { return (eventCounts[0] ? selectedPromise : Promise.resolve()) .then(afterDragFn) .then(function() { - assertEventCounts(eventCounts[0], eventCounts[1], 0, msg + ' (before dblclick)'); + // TODO: in v2 when we remove the `plotly_selecting->undefined` the Math.max(...) + // in the middle here will turn into just eventCounts[1]. + // It's just here because one of the selected events is generated during + // doubleclick so hasn't happened yet when we're testing this. + assertEventCounts(eventCounts[0], Math.max(0, eventCounts[1] - 1), 0, msg + ' (before dblclick)'); return doubleClick(dblClickPos[0], dblClickPos[1]); }) .then(eventCounts[2] ? deselectPromise : Promise.resolve())