From 2fbaaaa5c48e9215a264234ae683142353e17968 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 10 Aug 2018 07:44:03 +0200 Subject: [PATCH 01/54] Tracer-bullet impl of fast click-to-select [1852] Reason: to be reviewable from a perf standpoint. --- src/lib/polygon.js | 26 +++-- src/plots/cartesian/select.js | 188 ++++++++++++++++++++++++++++++++-- src/traces/bar/select.js | 2 +- src/traces/scatter/select.js | 2 +- src/traces/scattergl/index.js | 2 +- 5 files changed, 199 insertions(+), 21 deletions(-) diff --git a/src/lib/polygon.js b/src/lib/polygon.js index d84583e696e..7033e7988e3 100644 --- a/src/lib/polygon.js +++ b/src/lib/polygon.js @@ -179,25 +179,29 @@ polygon.tester = function tester(ptsIn) { */ polygon.multitester = function multitester(list) { var testers = [], - xmin = list[0][0][0], + xmin = list[0].contains ? 0 : list[0][0][0], xmax = xmin, - ymin = list[0][0][1], + ymin = list[0].contains ? 0 : list[0][0][1], ymax = ymin; for(var i = 0; i < list.length; i++) { - var tester = polygon.tester(list[i]); - tester.subtract = list[i].subtract; - testers.push(tester); - xmin = Math.min(xmin, tester.xmin); - xmax = Math.max(xmax, tester.xmax); - ymin = Math.min(ymin, tester.ymin); - ymax = Math.max(ymax, tester.ymax); + if(list[i].contains) { + testers.push(list[i]); + } else { + var tester = polygon.tester(list[i]); + tester.subtract = list[i].subtract; + testers.push(tester); + xmin = Math.min(xmin, tester.xmin); + xmax = Math.max(xmax, tester.xmax); + ymin = Math.min(ymin, tester.ymin); + ymax = Math.max(ymax, tester.ymax); + } } - function contains(pt, arg) { + function contains(pt, arg, index, expandedTraceIndex) { var yes = false; for(var i = 0; i < testers.length; i++) { - if(testers[i].contains(pt, arg)) { + if(testers[i].contains(pt, arg, index, expandedTraceIndex)) { // if contained by subtract polygon - exclude the point yes = testers[i].subtract === false; } diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 498dfff60d2..7eaeee1f722 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -264,13 +264,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { } // draw selection - var paths = []; - for(i = 0; i < mergedPolygons.length; i++) { - var ppts = mergedPolygons[i]; - paths.push(ppts.join('L') + 'L' + ppts[0]); - } - outlines - .attr('d', 'M' + paths.join('M') + 'Z'); + drawSelection(mergedPolygons, outlines); + throttle.throttle( throttleID, @@ -320,6 +315,65 @@ function prepSelect(e, startX, startY, dragOptions, mode) { gd.emit('plotly_deselect', null); } else { + + + + var hoverData = gd._hoverdata; + var selection = []; + var traceSelection; + var thisTracesSelection; + var pointSelected; + var subtract; + + if(isHoverDataSet(hoverData)) { + var clickedPtInfo = extractClickedPtInfo(hoverData, searchTraces); + + // TODO perf: call potentially costly operation (see impl comment) only when needed + pointSelected = isPointSelected(clickedPtInfo.searchInfo.cd[0].trace, + clickedPtInfo.pointNumber); + + if(pointSelected && isOnlyOnePointSelected(searchTraces)) { + // TODO DRY see doubleClick handling above + outlines.remove(); + for(i = 0; i < searchTraces.length; i++) { + searchInfo = searchTraces[i]; + searchInfo._module.selectPoints(searchInfo, false); + } + + updateSelectedState(gd, searchTraces); + gd.emit('plotly_deselect', null); + } else { + subtract = evt.shiftKey && pointSelected; + currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, + clickedPtInfo.searchInfo.cd[0].trace._expandedIndex, subtract); + + var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); + testPoly = multipolygonTester(concatenatedPolygons); + + for(i = 0; i < searchTraces.length; i++) { + traceSelection = searchTraces[i]._module.selectPoints(searchTraces[i], testPoly); + thisTracesSelection = fillSelectionItem(traceSelection, searchTraces[i]); + + if(selection.length) { + for(var j = 0; j < thisTracesSelection.length; j++) { + selection.push(thisTracesSelection[j]); + } + } + else selection = thisTracesSelection; + } + + eventData = {points: selection}; + updateSelectedState(gd, searchTraces, eventData); + + if(currentPolygon && dragOptions.polygons) { + dragOptions.polygons.push(currentPolygon); + } + } + + } + + drawSelection(dragOptions.mergedPolygons, outlines); + // 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); @@ -349,6 +403,126 @@ function prepSelect(e, startX, startY, dragOptions, mode) { }; } +function drawSelection(polygons, outlines) { + var paths = []; + var i; + var d; + + for(i = 0; i < polygons.length; i++) { + var ppts = polygons[i]; + paths.push(ppts.join('L') + 'L' + ppts[0]); + } + + d = polygons.length > 0 ? + 'M' + paths.join('M') + 'Z' : + ''; // TODO empty d attribute works in Chrome, but is it valid / can we rely on it? + outlines.attr('d', d); +} + +function isHoverDataSet(hoverData) { + return hoverData && + Array.isArray(hoverData) && + hoverData[0].hoverOnBox !== true; +} + +function extractClickedPtInfo(hoverData, searchTraces) { + var hoverDatum = hoverData[0]; + var pointNumber = -1; + var pointNumbers = []; + var searchInfo; + var i; + + for(i = 0; i < searchTraces.length; i++) { + searchInfo = searchTraces[i]; + if(hoverDatum.fullData._expandedIndex === searchInfo.cd[0].trace._expandedIndex) { + + // Special case for box (and violin) + if(hoverDatum.hoverOnBox === true) { + break; + } + + // TODO hoverDatum not having a pointNumber but a binNumber seems to be an oddity of histogram only + // Not deleting .pointNumber in histogram/event_data.js would simplify code here and in addition + // would not break the hover event structure + // documented at https://plot.ly/javascript/hover-events/ + if(hoverDatum.pointNumber !== undefined) { + pointNumber = hoverDatum.pointNumber; + } else if(hoverDatum.binNumber !== undefined) { + pointNumber = hoverDatum.binNumber; + pointNumbers = hoverDatum.pointNumbers; + } + + break; + } + } + + return { + pointNumber: pointNumber, + pointNumbers: pointNumbers, + searchInfo: searchInfo + }; +} + +// TODO What about passing a searchInfo instead of wantedExpandedTraceIndex? +function createPtNumTester(wantedPointNumber, wantedExpandedTraceIndex, subtract) { + return { + xmin: 0, + xmax: 0, + ymin: 0, + ymax: 0, + pts: [], + // TODO Consider making signature of contains more lean + contains: function(pt, omitFirstEdge, pointNumber, expandedTraceIndex) { + return expandedTraceIndex === wantedExpandedTraceIndex && pointNumber === wantedPointNumber; + }, + isRect: false, + degenerate: false, + subtract: subtract + }; +} + +function isPointSelected(trace, pointNumber) { + // TODO improve perf + // Primarily we need this function to determine if a click adds or subtracts from a selection. + // + // IME best user experience would be + // - that Shift+Click an unselected points adds to selection + // - and Shift+Click a selected point subtracts from selection. + // + // Several options: + // 1. Avoid problem at all by binding subtract-selection-by-click operation to Shift+Alt-Click. + // Slightly less intuitive. A lot of programs deselect an already selected element when you + // Shift+Click it. + // 2. Delegate decision to the traces module through an additional + // isSelected(searchInfo, pointNumber) function. Traces like scatter or bar have + // a selected flag attached to each calcData element, thus access to that information + // would be fast. However, scattergl only maintains selectBatch and unselectBatch arrays. + // So simply searching through those arrays in scattegl would be slow. Just imagine + // a user selecting all data points with one lasso polygon. So scattergl would require some + // work. + return trace.selectedpoints ? trace.selectedpoints.indexOf(pointNumber) > -1 : false; +} + +function isOnlyOnePointSelected(searchTraces) { + var len = 0; + var searchInfo; + var trace; + var i; + + for(i = 0; i < searchTraces.length; i++) { + searchInfo = searchTraces[i]; + trace = searchInfo.cd[0].trace; + if(trace.selectedpoints) { + if(trace.selectedpoints.length > 1) return false; + + len += trace.selectedpoints.length; + if(len > 1) return false; + } + } + + return len === 1; +} + function updateSelectedState(gd, searchTraces, eventData) { var i, j, searchInfo, trace; diff --git a/src/traces/bar/select.js b/src/traces/bar/select.js index 04ede09356c..c184a024586 100644 --- a/src/traces/bar/select.js +++ b/src/traces/bar/select.js @@ -24,7 +24,7 @@ module.exports = function selectPoints(searchInfo, polygon) { for(i = 0; i < cd.length; i++) { var di = cd[i]; - if(polygon.contains(di.ct)) { + if(polygon.contains(di.ct, false, i, searchInfo.cd[0].trace._expandedIndex)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/scatter/select.js b/src/traces/scatter/select.js index 5d10050b494..524f6993d49 100644 --- a/src/traces/scatter/select.js +++ b/src/traces/scatter/select.js @@ -36,7 +36,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(di.x); y = ya.c2p(di.y); - if(polygon.contains([x, y])) { + if(polygon.contains([x, y], false, i, searchInfo.cd[0].trace._expandedIndex)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index e4cf251a984..56357555470 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -840,7 +840,7 @@ function selectPoints(searchInfo, polygon) { if(polygon !== false && !polygon.degenerate) { els = [], unels = []; for(i = 0; i < stash.count; i++) { - if(polygon.contains([stash.xpx[i], stash.ypx[i]])) { + if(polygon.contains([stash.xpx[i], stash.ypx[i]], false, i, searchInfo.cd[0].trace._expandedIndex)) { els.push(i); selection.push({ pointNumber: i, From 346bad92fdeb6d62aba2242f262c673fc140c8d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 16 Aug 2018 10:13:10 +0200 Subject: [PATCH 02/54] Set correct blank outlines path in case of zero select polygons [1852] --- src/plots/cartesian/select.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 7eaeee1f722..ad16bfec7a4 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -415,7 +415,7 @@ function drawSelection(polygons, outlines) { d = polygons.length > 0 ? 'M' + paths.join('M') + 'Z' : - ''; // TODO empty d attribute works in Chrome, but is it valid / can we rely on it? + 'M0,0Z'; outlines.attr('d', d); } From 4f8dd929fde0ec044812da57b2a64001eea42869 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 16 Aug 2018 14:16:16 +0200 Subject: [PATCH 03/54] Fix select_test.js Jasmine tests [1852] - Set hoverMode to 'closest' where needed. --- src/plots/cartesian/select.js | 6 +----- test/jasmine/tests/select_test.js | 6 +++++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index ad16bfec7a4..88f94c37f32 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -313,11 +313,7 @@ function prepSelect(e, startX, startY, dragOptions, mode) { updateSelectedState(gd, searchTraces); gd.emit('plotly_deselect', null); - } - else { - - - + } else { var hoverData = gd._hoverdata; var selection = []; var traceSelection; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index a801c36ddf8..02d7eb7e829 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -143,6 +143,7 @@ describe('@flaky Test select box and lasso in general:', function() { describe('select events', function() { var mockCopy = Lib.extendDeep({}, mock); mockCopy.layout.dragmode = 'select'; + mockCopy.layout.hovermode = 'closest'; mockCopy.data[0].ids = mockCopy.data[0].x .map(function(v) { return 'id-' + v; }); mockCopy.data[0].customdata = mockCopy.data[0].y @@ -293,6 +294,7 @@ describe('@flaky Test select box and lasso in general:', function() { describe('lasso events', function() { var mockCopy = Lib.extendDeep({}, mock); mockCopy.layout.dragmode = 'lasso'; + mockCopy.layout.hovermode = 'closest'; addInvisible(mockCopy); var gd; @@ -635,6 +637,7 @@ describe('@flaky Test select box and lasso in general:', function() { fig.layout.xaxis.range = [2, 8]; fig.layout.yaxis.autorange = false; fig.layout.yaxis.range = [0, 3]; + fig.layout.hovermode = 'closest'; function _assert(msg, exp) { expect(gd.layout.xaxis.range) @@ -1801,6 +1804,7 @@ describe('@flaky Test select box and lasso per trace:', function() { textposition: 'outside' }], { dragmode: 'select', + hovermode: 'closest', showlegend: false, width: 400, height: 400, @@ -1813,7 +1817,7 @@ describe('@flaky Test select box and lasso per trace:', function() { assertSelectedPoints({0: [0], 1: [0]}); assertFillOpacity([1, 0.2, 0.2, 1, 0.2, 0.2]); }, - null, BOXEVENTS, 'selecting first scatter/bar text nodes' + [10, 10], BOXEVENTS, 'selecting first scatter/bar text nodes' ); }) .then(function() { From 6a8776995a9d3e03442811eb47e3cc624fa1597e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 17 Aug 2018 10:48:11 +0200 Subject: [PATCH 04/54] Add click-to-select specific tests to select_test.js [1852] --- src/plots/cartesian/select.js | 5 + test/jasmine/tests/select_test.js | 265 +++++++++++++++++++++++++++++- 2 files changed, 269 insertions(+), 1 deletion(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 88f94c37f32..0e8393eb214 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -370,6 +370,11 @@ function prepSelect(e, startX, startY, dragOptions, mode) { drawSelection(dragOptions.mergedPolygons, outlines); + // TODO with click-to-select arriving before v2, consider emitting + // plotly_selected only if a point has been selected. Also consider the + // actual clickmode and adapt Jasmine tests that are aware of this current + // behavior. + // 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 02d7eb7e829..f76c6486b8d 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -2,7 +2,9 @@ var d3 = require('d3'); var Plotly = require('@lib/index'); var Lib = require('@src/lib'); +var click = require('../assets/click'); var doubleClick = require('../assets/double_click'); +var DBLCLICKDELAY = require('../../../src/constants/interactions').DBLCLICKDELAY; var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); @@ -75,7 +77,9 @@ function resetEvents(gd) { }); gd.on('plotly_selected', function(data) { - assertSelectionNodes(0, 2); + // TODO reconsider this if(data) condition when it's clear when + // and how plotly_selected event is emitted in context of click-to-select. + if(data) assertSelectionNodes(0, 2); selectedCnt++; selectedData = data; resolve(); @@ -109,6 +113,265 @@ var BOXEVENTS = [1, 2, 1]; // assumes 5 points in the lasso path var LASSOEVENTS = [4, 2, 1]; +var SELECT_PATH = [[93, 193], [143, 193]]; +var LASSO_PATH = [[316, 171], [318, 239], [335, 243], [328, 169]]; + +describe('Click-to-select', function() { + var mock14Pts = { + '1': { x: 134, y: 116 }, + '7': { x: 270, y: 160 }, + '10': { x: 324, y: 198 }, + '35': { x: 685, y: 341 } + }; + var gd; + + beforeEach(function() { + gd = createGraphDiv(); + }); + + afterEach(destroyGraphDiv); + + function plotMock14() { + var mock = require('@mocks/14.json'); + var mockCopy = Lib.extendDeep({}, mock); + + mockCopy.layout.dragmode = 'select'; + mockCopy.layout.hovermode = 'closest'; + + return Plotly.plot(gd, mockCopy.data, mockCopy.layout); + } + + /** + * Executes a click and before resets selection event handlers. + * By default, click is executed with a delay to prevent unwanted double clicks. + * Returns the `selectedPromise` promise for convenience. + */ + function _click(x, y, clickOpts, immediate) { + resetEvents(gd); + + // Too fast subsequent calls of `click` would + // produce an unwanted double click, thus we need + // to delay the click. + if(immediate) { + click(x, y, clickOpts); + } else { + setTimeout(function() { + click(x, y, clickOpts); + }, DBLCLICKDELAY * 1.01); + } + + return selectedPromise; + } + + function _clickPt(coords, clickOpts, immediate) { + expect(coords).toBeDefined('coords needs to be defined'); + expect(coords.x).toBeDefined('coords.x needs to be defined'); + expect(coords.y).toBeDefined('coords.y needs to be defined'); + + return _click(coords.x, coords.y, clickOpts, immediate); + } + + /** + * Convenient helper to execute a click immediately. + */ + function _immediateClickPt(coords, clickOpts) { + return _clickPt(coords, clickOpts, true); + } + + /** + * Asserting selected points. + * + * @param expected can be a point number, an array + * of point numbers (for a single trace) or an array of point number + * arrays in case of multiple traces. + */ + function assertSelectedPoints(expected) { + var expectedPtsPerTrace; + var expectedPts; + var traceNum; + + if(Array.isArray(expected)) { + if(Array.isArray(expected[0])) { + expectedPtsPerTrace = expected; + } else { + expectedPtsPerTrace = [expected]; + } + } else { + expectedPtsPerTrace = [[expected]]; + } + + for(traceNum = 0; traceNum < expectedPtsPerTrace.length; traceNum++) { + expectedPts = expectedPtsPerTrace[traceNum]; + expect(gd._fullData[traceNum].selectedpoints).toEqual(expectedPts); + expect(gd.data[traceNum].selectedpoints).toEqual(expectedPts); + } + } + + function assertSelectionCleared() { + gd._fullData.forEach(function(fullDataItem) { + expect(fullDataItem.selectedpoints).toBeUndefined(); + }); + } + + it('selects a single data point when being clicked', function(done) { + plotMock14() + .then(function() { return _immediateClickPt(mock14Pts[7]); }) + .then(function() { assertSelectedPoints(7); }) + .catch(failTest) + .then(done); + }); + + describe('clears entire selection when the last selected data point', function() { + [{ + desc: 'is clicked', + clickOpts: {} + }, { + desc: 'is clicked while add/subtract modifier keys are active', + clickOpts: { shiftKey: true } + }].forEach(function(testData) { + it(testData.desc, function(done) { + plotMock14() + .then(function() { return _immediateClickPt(mock14Pts[7]); }) + .then(function() { + assertSelectedPoints(7); + _clickPt(mock14Pts[7], testData.opts); + return deselectPromise; + }) + .then(assertSelectionCleared) + .catch(failTest) + .then(done); + }); + }); + }); + + it('supports adding to an existing selection', function(done) { + plotMock14() + .then(function() { return _immediateClickPt(mock14Pts[7]); }) + .then(function() { + assertSelectedPoints(7); + return _clickPt(mock14Pts[35], { shiftKey: true }); + }) + .then(function() { assertSelectedPoints([7, 35]); }) + .catch(failTest) + .then(done); + }); + + it('supports subtracting from an existing selection', function(done) { + plotMock14() + .then(function() { return _immediateClickPt(mock14Pts[7]); }) + .then(function() { + assertSelectedPoints(7); + return _clickPt(mock14Pts[35], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([7, 35]); + return _clickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { assertSelectedPoints(35); }) + .catch(failTest) + .then(done); + }); + + it('can be used interchangeably with lasso/box select', function(done) { + plotMock14() + .then(function() { + return _immediateClickPt(mock14Pts[35]); + }) + .then(function() { + assertSelectedPoints(35); + drag(SELECT_PATH, { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 1, 35]); + return _immediateClickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 1, 7, 35]); + return _clickPt(mock14Pts[1], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 7, 35]); + return Plotly.relayout(gd, 'dragmode', 'lasso'); + }) + .then(function() { + assertSelectedPoints([0, 7, 35]); + drag(LASSO_PATH, { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 7, 10, 35]); + return _clickPt(mock14Pts[10], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 7, 35]); + drag([[670, 330], [695, 330], [695, 350], [670, 350]], + { shiftKey: true, altKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 7]); + return _clickPt(mock14Pts[35], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([0, 7, 35]); + return _clickPt(mock14Pts[7]); + }) + .then(function() { + assertSelectedPoints([7]); + return doubleClick(650, 100); + }) + .then(function() { + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + }); + + it('works in a multi-trace plot', function(done) { + Plotly.plot(gd, [ + { + x: [1, 3, 5, 4, 10, 12, 12, 7], + y: [2, 7, 6, 1, 0, 13, 6, 12], + type: 'scatter', + mode: 'markers', + marker: { size: 20 } + }, { + x: [1, 7, 6, 2], + y: [2, 3, 5, 4], + type: 'bar' + }, { + x: [7, 8, 9, 10], + y: [7, 9, 13, 21], + type: 'scattergl', + mode: 'markers', + marker: { size: 20 } + } + ], { width: 400, height: 600, hovermode: 'closest', dragmode: 'select' }) + .then(function() { + return _click(136, 369, {}, true); }) + .then(function() { + assertSelectedPoints([[1], [], []]); + return _click(245, 136, { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([[1], [], [3]]); + return _click(183, 470, { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([[1], [2], [3]]); + }) + .catch(failTest) + .then(done); + }); + + // it('is supported in pan/zoom mode', function() { + // }); + + // describe('is disabled when clickmode does not include select', function() { + // }) + + // describe('is supported by', function() { + // }) +}); + describe('@flaky Test select box and lasso in general:', function() { var mock = require('@mocks/14.json'); var selectPath = [[93, 193], [143, 193]]; From 2d593658510647b06e48cc79969952836671d837 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Tue, 21 Aug 2018 08:34:39 +0200 Subject: [PATCH 05/54] Make click-to-select work in pan/zoom dragmodes [1852] --- src/plots/cartesian/dragbox.js | 20 +- src/plots/cartesian/select.js | 306 ++++++++++++++++-------------- test/jasmine/tests/select_test.js | 74 +++++++- 3 files changed, 252 insertions(+), 148 deletions(-) diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index 8d66a4cb4a5..8cf800e21b1 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -30,6 +30,7 @@ var doTicksSingle = require('./axes').doTicksSingle; var getFromId = require('./axis_ids').getFromId; var prepSelect = require('./select').prepSelect; var clearSelect = require('./select').clearSelect; +var selectOnClick = require('./select').selectOnClick; var scaleZoom = require('./scale_zoom'); var constants = require('./constants'); @@ -148,7 +149,11 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { }; dragOptions.prepFn = function(e, startX, startY) { + var dragModePrev = dragOptions.dragmode; var dragModeNow = gd._fullLayout.dragmode; + if(dragModeNow !== dragModePrev) { + dragOptions.dragmode = dragModeNow; + } recomputeAxisLists(); @@ -178,7 +183,18 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { prepSelect(e, startX, startY, dragOptions, dragModeNow); } else { dragOptions.clickFn = clickFn; - clearAndResetSelect(); + if(isSelectOrLasso(dragModePrev)) { + // TODO Fix potential bug + // Note: clearing / resetting selection state only happens, when user + // triggers at least one interaction in pan/zoom mode. Otherwise, the + // select/lasso outlines are deleted (in plots.js.cleanPlot) but the selection + // cache isn't cleared. So when the user switches back to select/lasso and + // 'add to a selection' with Shift, the "old", seemingly removed outlines + // are redrawn again because the selection cache still holds their points. + // However, this isn't easily solved, since plots.js would need + // to have a reference to the dragOptions object. + clearAndResetSelect(); + } if(!allFixedRanges) { if(dragModeNow === 'zoom') { @@ -213,6 +229,8 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { if(isMainDrag) { Fx.click(gd, evt, plotinfo.id); + // TODO Throttle needed here? + selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions); } else if(numClicks === 1 && singleEnd) { var ax = ns ? ya0 : xa0, diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 0e8393eb214..3c5fb417d2f 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -45,43 +45,15 @@ function prepSelect(e, startX, startY, dragOptions, mode) { var path0 = 'M' + x0 + ',' + y0; var pw = dragOptions.xaxes[0]._length; var ph = dragOptions.yaxes[0]._length; - var xAxisIds = dragOptions.xaxes.map(getAxId); - var yAxisIds = dragOptions.yaxes.map(getAxId); + // var xAxisIds = dragOptions.xaxes.map(getAxId); + // var yAxisIds = dragOptions.yaxes.map(getAxId); var allAxes = dragOptions.xaxes.concat(dragOptions.yaxes); var subtract = e.altKey; var filterPoly, testPoly, mergedPolygons, currentPolygon; - var i, cd, trace, searchInfo, eventData; + var i, searchInfo, eventData; - var selectingOnSameSubplot = ( - fullLayout._lastSelectedSubplot && - fullLayout._lastSelectedSubplot === plotinfo.id - ); - - if( - selectingOnSameSubplot && - (e.shiftKey || e.altKey) && - (plotinfo.selection && plotinfo.selection.polygons) && - !dragOptions.polygons - ) { - // take over selection polygons from prev mode, if any - dragOptions.polygons = plotinfo.selection.polygons; - dragOptions.mergedPolygons = plotinfo.selection.mergedPolygons; - } else if( - (!e.shiftKey && !e.altKey) || - ((e.shiftKey || e.altKey) && !plotinfo.selection) - ) { - // create new polygons, if shift mode or selecting across different subplots - plotinfo.selection = {}; - plotinfo.selection.polygons = dragOptions.polygons = []; - plotinfo.selection.mergedPolygons = dragOptions.mergedPolygons = []; - } - - // clear selection outline when selecting a different subplot - if(!selectingOnSameSubplot) { - clearSelect(zoomLayer); - fullLayout._lastSelectedSubplot = plotinfo.id; - } + coerceSelectionsCache(e, gd, dragOptions); if(mode === 'lasso') { filterPoly = filteredPolygon([[x0, y0]], constants.BENDPX); @@ -106,52 +78,12 @@ function prepSelect(e, startX, startY, dragOptions, mode) { .attr('d', 'M0,0Z'); - // find the traces to search for selection points - var searchTraces = []; var throttleID = fullLayout._uid + constants.SELECTID; var selection = []; - for(i = 0; i < gd.calcdata.length; i++) { - cd = gd.calcdata[i]; - trace = cd[0].trace; - - if(trace.visible !== true || !trace._module || !trace._module.selectPoints) continue; - - if(dragOptions.subplot) { - if( - trace.subplot === dragOptions.subplot || - trace.geo === dragOptions.subplot - ) { - searchTraces.push({ - _module: trace._module, - cd: cd, - xaxis: dragOptions.xaxes[0], - yaxis: dragOptions.yaxes[0] - }); - } - } else if( - trace.type === 'splom' && - // FIXME: make sure we don't have more than single axis for splom - trace._xaxes[xAxisIds[0]] && trace._yaxes[yAxisIds[0]] - ) { - searchTraces.push({ - _module: trace._module, - cd: cd, - xaxis: dragOptions.xaxes[0], - yaxis: dragOptions.yaxes[0] - }); - } else { - if(xAxisIds.indexOf(trace.xaxis) === -1) continue; - if(yAxisIds.indexOf(trace.yaxis) === -1) continue; - - searchTraces.push({ - _module: trace._module, - cd: cd, - xaxis: getFromId(gd, trace.xaxis), - yaxis: getFromId(gd, trace.yaxis) - }); - } - } + // find the traces to search for selection points + var searchTraces = determineSearchTraces(gd, dragOptions.xaxes, + dragOptions.yaxes, dragOptions.subplot); function axValue(ax) { var index = (ax._id.charAt(0) === 'y') ? 1 : 0; @@ -314,70 +246,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { updateSelectedState(gd, searchTraces); gd.emit('plotly_deselect', null); } else { - var hoverData = gd._hoverdata; - var selection = []; - var traceSelection; - var thisTracesSelection; - var pointSelected; - var subtract; - - if(isHoverDataSet(hoverData)) { - var clickedPtInfo = extractClickedPtInfo(hoverData, searchTraces); - - // TODO perf: call potentially costly operation (see impl comment) only when needed - pointSelected = isPointSelected(clickedPtInfo.searchInfo.cd[0].trace, - clickedPtInfo.pointNumber); - - if(pointSelected && isOnlyOnePointSelected(searchTraces)) { - // TODO DRY see doubleClick handling above - outlines.remove(); - for(i = 0; i < searchTraces.length; i++) { - searchInfo = searchTraces[i]; - searchInfo._module.selectPoints(searchInfo, false); - } - - updateSelectedState(gd, searchTraces); - gd.emit('plotly_deselect', null); - } else { - subtract = evt.shiftKey && pointSelected; - currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, - clickedPtInfo.searchInfo.cd[0].trace._expandedIndex, subtract); - - var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); - testPoly = multipolygonTester(concatenatedPolygons); - - for(i = 0; i < searchTraces.length; i++) { - traceSelection = searchTraces[i]._module.selectPoints(searchTraces[i], testPoly); - thisTracesSelection = fillSelectionItem(traceSelection, searchTraces[i]); - - if(selection.length) { - for(var j = 0; j < thisTracesSelection.length; j++) { - selection.push(thisTracesSelection[j]); - } - } - else selection = thisTracesSelection; - } - - eventData = {points: selection}; - updateSelectedState(gd, searchTraces, eventData); - - if(currentPolygon && dragOptions.polygons) { - dragOptions.polygons.push(currentPolygon); - } - } - - } - - drawSelection(dragOptions.mergedPolygons, outlines); - - // TODO with click-to-select arriving before v2, consider emitting - // plotly_selected only if a point has been selected. Also consider the - // actual clickmode and adapt Jasmine tests that are aware of this current - // behavior. - - // 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); + selectOnClick(evt, gd, dragOptions.xaxes, dragOptions.yaxes, + dragOptions.subplot, dragOptions, outlines); } Fx.click(gd, evt); @@ -404,6 +274,161 @@ function prepSelect(e, startX, startY, dragOptions, mode) { }; } +function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutlines) { + var hoverData = gd._hoverdata; + var selection = []; + var searchTraces; + var searchInfo; + var currentPolygon; + var testPoly; + var traceSelection; + var thisTracesSelection; + var pointSelected; + var subtract; + var eventData; + var i; + + if(isHoverDataSet(hoverData)) { + coerceSelectionsCache(evt, gd, dragOptions); + searchTraces = determineSearchTraces(gd, xAxes, yAxes, subplot); + var clickedPtInfo = extractClickedPtInfo(hoverData, searchTraces); + + // TODO perf: call potentially costly operation (see impl comment) only when needed + pointSelected = isPointSelected(clickedPtInfo.searchInfo.cd[0].trace, + clickedPtInfo.pointNumber); + + if(pointSelected && isOnlyOnePointSelected(searchTraces)) { + // TODO DRY see doubleClick handling above + if(polygonOutlines) polygonOutlines.remove(); + for(i = 0; i < searchTraces.length; i++) { + searchInfo = searchTraces[i]; + searchInfo._module.selectPoints(searchInfo, false); + } + + updateSelectedState(gd, searchTraces); + gd.emit('plotly_deselect', null); + } else { + subtract = evt.shiftKey && pointSelected; + currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, + clickedPtInfo.searchInfo.cd[0].trace._expandedIndex, subtract); + + // var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); + var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); + testPoly = multipolygonTester(concatenatedPolygons); + + for(i = 0; i < searchTraces.length; i++) { + traceSelection = searchTraces[i]._module.selectPoints(searchTraces[i], testPoly); + thisTracesSelection = fillSelectionItem(traceSelection, searchTraces[i]); + + if(selection.length) { + for(var j = 0; j < thisTracesSelection.length; j++) { + selection.push(thisTracesSelection[j]); + } + } + else selection = thisTracesSelection; + } + + eventData = {points: selection}; + updateSelectedState(gd, searchTraces, eventData); + + if(currentPolygon && dragOptions) { + dragOptions.polygons.push(currentPolygon); + } + } + + } + + if(polygonOutlines) drawSelection(dragOptions.mergedPolygons, polygonOutlines); + + // TODO with click-to-select arriving before v2, consider emitting + // plotly_selected only if a point has been selected. Also consider the + // actual clickmode and adapt Jasmine tests that are aware of this current + // behavior. + + // 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); +} + +function coerceSelectionsCache(evt, gd, dragOptions) { + var fullLayout = gd._fullLayout; + var zoomLayer = fullLayout._zoomlayer; + var plotinfo = dragOptions.plotinfo; + + var selectingOnSameSubplot = ( + fullLayout._lastSelectedSubplot && + fullLayout._lastSelectedSubplot === plotinfo.id + ); + + if( + selectingOnSameSubplot && + (evt.shiftKey || evt.altKey) && + (plotinfo.selection && plotinfo.selection.polygons) && + !dragOptions.polygons + ) { + // take over selection polygons from prev mode, if any + dragOptions.polygons = plotinfo.selection.polygons; + dragOptions.mergedPolygons = plotinfo.selection.mergedPolygons; + } else if( + (!evt.shiftKey && !evt.altKey) || + ((evt.shiftKey || evt.altKey) && !plotinfo.selection) + ) { + // create new polygons, if shift mode or selecting across different subplots + plotinfo.selection = {}; + plotinfo.selection.polygons = dragOptions.polygons = []; + plotinfo.selection.mergedPolygons = dragOptions.mergedPolygons = []; + } + + // clear selection outline when selecting a different subplot + if(!selectingOnSameSubplot) { + clearSelect(zoomLayer); + fullLayout._lastSelectedSubplot = plotinfo.id; + } +} + +function determineSearchTraces(gd, xAxes, yAxes, subplot) { + var searchTraces = []; + var xAxisIds = xAxes.map(getAxId); + var yAxisIds = yAxes.map(getAxId); + var cd; + var trace; + var i; + + for(i = 0; i < gd.calcdata.length; i++) { + cd = gd.calcdata[i]; + trace = cd[0].trace; + + if(trace.visible !== true || !trace._module || !trace._module.selectPoints) continue; + + if(subplot && (trace.subplot === subplot || trace.geo === subplot)) { + searchTraces.push(createSearchInfo(trace._module, cd, xAxes[0], yAxes[0])); + } else if( + trace.type === 'splom' && + // FIXME: make sure we don't have more than single axis for splom + trace._xaxes[xAxisIds[0]] && trace._yaxes[yAxisIds[0]] + ) { + searchTraces.push(createSearchInfo(trace._module, cd, xAxes[0], yAxes[0])); + } else { + if(xAxisIds.indexOf(trace.xaxis) === -1) continue; + if(yAxisIds.indexOf(trace.yaxis) === -1) continue; + + searchTraces.push(createSearchInfo(trace._module, cd, + getFromId(gd, trace.xaxis), getFromId(gd, trace.yaxis))); + } + } + + return searchTraces; + + function createSearchInfo(module, calcData, xaxis, yaxis) { + return { + _module: module, + cd: calcData, + xaxis: xaxis, + yaxis: yaxis + }; + } +} + function drawSelection(polygons, outlines) { var paths = []; var i; @@ -646,5 +671,6 @@ function clearSelect(zoomlayer) { module.exports = { prepSelect: prepSelect, - clearSelect: clearSelect + clearSelect: clearSelect, + selectOnClick: selectOnClick }; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index f76c6486b8d..4353589a834 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -131,12 +131,19 @@ describe('Click-to-select', function() { afterEach(destroyGraphDiv); - function plotMock14() { + function plotMock14(layoutOpts) { var mock = require('@mocks/14.json'); - var mockCopy = Lib.extendDeep({}, mock); - - mockCopy.layout.dragmode = 'select'; - mockCopy.layout.hovermode = 'closest'; + var defaultLayoutOpts = { + layout: { + dragmode: 'select', + hovermode: 'closest' + } + }; + var mockCopy = Lib.extendDeep( + {}, + mock, + defaultLayoutOpts, + { layout: layoutOpts }); return Plotly.plot(gd, mockCopy.data, mockCopy.layout); } @@ -362,8 +369,61 @@ describe('Click-to-select', function() { .then(done); }); - // it('is supported in pan/zoom mode', function() { - // }); + it('is supported in pan/zoom mode', function(done) { + plotMock14({ dragmode: 'zoom' }) + .then(function() { + return _immediateClickPt(mock14Pts[35]); + }) + .then(function() { + assertSelectedPoints(35); + return _clickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([7, 35]); + return _clickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints(35); + drag(LASSO_PATH); + }) + .then(function() { + assertSelectedPoints(35); + return _clickPt(mock14Pts[35], { shiftKey: true }); + }) + .then(function() { + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + }); + + it('retains selected points when switching between pan and zoom mode', function(done) { + plotMock14({ dragmode: 'zoom' }) + .then(function() { + return _immediateClickPt(mock14Pts[35]); + }) + .then(function() { + assertSelectedPoints(35); + return Plotly.relayout(gd, 'dragmode', 'pan'); + }) + .then(function() { + assertSelectedPoints(35); + return _clickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints([7, 35]); + return Plotly.relayout(gd, 'dragmode', 'zoom'); + }) + .then(function() { + assertSelectedPoints([7, 35]); + return _clickPt(mock14Pts[7], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints(35); + }) + .catch(failTest) + .then(done); + }); // describe('is disabled when clickmode does not include select', function() { // }) From 5a13573de7b7979767ee8afeddf3209eda627a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Tue, 21 Aug 2018 14:38:33 +0200 Subject: [PATCH 06/54] Introduce layout attribute 'clickmode' [1852] --- src/components/fx/layout_attributes.js | 21 +++++++++++ src/components/fx/layout_defaults.js | 2 ++ src/plots/cartesian/dragbox.js | 12 +++++-- src/plots/cartesian/select.js | 39 ++++++++++++-------- test/jasmine/tests/select_test.js | 49 ++++++++++++++++++++++---- 5 files changed, 98 insertions(+), 25 deletions(-) diff --git a/src/components/fx/layout_attributes.js b/src/components/fx/layout_attributes.js index 4c35c2b4086..246d3e5df72 100644 --- a/src/components/fx/layout_attributes.js +++ b/src/components/fx/layout_attributes.js @@ -18,6 +18,27 @@ fontAttrs.family.dflt = constants.HOVERFONT; fontAttrs.size.dflt = constants.HOVERFONTSIZE; module.exports = { + clickmode: { + valType: 'flaglist', + role: 'info', + flags: ['event', 'select'], + dflt: 'event', + editType: 'plot', + description: [ + 'Determines the mode of single click interactions.', + '*event* is the default value and only emits *plotly_selected*', + 'events with no event data (kept for compatibility reasons) in dragmodes', + '*lasso* and *select*. The *select* flag enables selecting single', + 'data points by click. Its behavior is closely tied to *hovermode*. The', + 'data point that is being currently hovered on, will be the data point', + 'to be selected. So setting *hovermode* to *closest* may be the best fit', + 'for most applications. Click-to-select also supports persistent selections,', + 'meaning that pressing Shift while clicking, adds to / subtracts from an', + 'existing selection.', + 'When *clickmode* is being set to *select+event*, click-to-select is enabled', + 'and select events are sent with corresponding eventData attached.' + ].join(' ') + }, dragmode: { valType: 'enumerated', role: 'info', diff --git a/src/components/fx/layout_defaults.js b/src/components/fx/layout_defaults.js index 742c6eb1621..ed34ce47b8d 100644 --- a/src/components/fx/layout_defaults.js +++ b/src/components/fx/layout_defaults.js @@ -16,6 +16,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt); } + coerce('clickmode'); + var dragMode = coerce('dragmode'); if(dragMode === 'select') coerce('selectdirection'); diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index 8cf800e21b1..884d50c4a6e 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -223,14 +223,20 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { } function clickFn(numClicks, evt) { + var clickmode = gd._fullLayout.clickmode; + removeZoombox(gd); if(numClicks === 2 && !singleEnd) doubleClick(); if(isMainDrag) { - Fx.click(gd, evt, plotinfo.id); - // TODO Throttle needed here? - selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions); + if(clickmode.indexOf('event') > -1) { + Fx.click(gd, evt, plotinfo.id); + } + + if(clickmode.indexOf('select') > -1) { + selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions); + } } else if(numClicks === 1 && singleEnd) { var ax = ns ? ya0 : xa0, diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 3c5fb417d2f..61fa87a9fe9 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -231,6 +231,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { }; dragOptions.clickFn = function(numClicks, evt) { + var clickmode = fullLayout.clickmode; + corners.remove(); throttle.done(throttleID).then(function() { @@ -246,8 +248,16 @@ function prepSelect(e, startX, startY, dragOptions, mode) { updateSelectedState(gd, searchTraces); gd.emit('plotly_deselect', null); } else { - selectOnClick(evt, gd, dragOptions.xaxes, dragOptions.yaxes, - dragOptions.subplot, dragOptions, outlines); + if(clickmode.indexOf('select') > -1) { + selectOnClick(evt, gd, dragOptions.xaxes, dragOptions.yaxes, + dragOptions.subplot, dragOptions, outlines); + } + + if(clickmode === 'event') { + // 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); + } } Fx.click(gd, evt); @@ -276,6 +286,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutlines) { var hoverData = gd._hoverdata; + var clickmode = gd._fullLayout.clickmode; + var sendEvents = clickmode.indexOf('event') > -1; var selection = []; var searchTraces; var searchInfo; @@ -306,7 +318,10 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli } updateSelectedState(gd, searchTraces); - gd.emit('plotly_deselect', null); + + if(sendEvents) { + gd.emit('plotly_deselect', null); + } } else { subtract = evt.shiftKey && pointSelected; currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, @@ -334,20 +349,14 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli if(currentPolygon && dragOptions) { dragOptions.polygons.push(currentPolygon); } - } - - } - - if(polygonOutlines) drawSelection(dragOptions.mergedPolygons, polygonOutlines); - // TODO with click-to-select arriving before v2, consider emitting - // plotly_selected only if a point has been selected. Also consider the - // actual clickmode and adapt Jasmine tests that are aware of this current - // behavior. + if(polygonOutlines) drawSelection(dragOptions.mergedPolygons, polygonOutlines); - // 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); + if(sendEvents) { + gd.emit('plotly_selected', eventData); + } + } + } } function coerceSelectionsCache(evt, gd, dragOptions) { diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 4353589a834..c7bd3630120 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -77,9 +77,13 @@ function resetEvents(gd) { }); gd.on('plotly_selected', function(data) { - // TODO reconsider this if(data) condition when it's clear when - // and how plotly_selected event is emitted in context of click-to-select. - if(data) assertSelectionNodes(0, 2); + // With click-to-select supported, selection nodes are only + // in the DOM in certain circumstances. + if(data && + gd._fullLayout.dragmode.indexOf('select') > -1 && + gd._fullLayout.dragmode.indexOf('lasso') > -1) { + assertSelectionNodes(0, 2); + } selectedCnt++; selectedData = data; resolve(); @@ -135,6 +139,7 @@ describe('Click-to-select', function() { var mock = require('@mocks/14.json'); var defaultLayoutOpts = { layout: { + clickmode: 'event+select', dragmode: 'select', hovermode: 'closest' } @@ -351,7 +356,13 @@ describe('Click-to-select', function() { mode: 'markers', marker: { size: 20 } } - ], { width: 400, height: 600, hovermode: 'closest', dragmode: 'select' }) + ], { + width: 400, + height: 600, + hovermode: 'closest', + dragmode: 'select', + clickmode: 'event+select' + }) .then(function() { return _click(136, 369, {}, true); }) .then(function() { @@ -388,7 +399,8 @@ describe('Click-to-select', function() { }) .then(function() { assertSelectedPoints(35); - return _clickPt(mock14Pts[35], { shiftKey: true }); + _clickPt(mock14Pts[35], { shiftKey: true }); + return deselectPromise; }) .then(function() { assertSelectionCleared(); @@ -425,8 +437,31 @@ describe('Click-to-select', function() { .then(done); }); - // describe('is disabled when clickmode does not include select', function() { - // }) + describe('is disabled when clickmode does not include \'select\'', function() { + // TODO How to test for pan and zoom mode as well? Note, that + // in lasso and select mode, plotly_selected was emitted upon a single + // click although select-on-click wasn't supported. This behavior is kept + // for compatibility reasons and as a side affect allows to write this test + // for lasso and select. But in pan and zoom, how to be sure a click has been + // processed by plotly.js? + // ['pan', 'zoom', 'select', 'lasso'] + ['select', 'lasso'] + .forEach(function(dragmode) { + it('and dragmode is ' + dragmode, function(done) { + plotMock14({ clickmode: 'event', dragmode: dragmode }) + .then(function() { + // Still, the plotly_selected event should be thrown, + // so return promise here + return _immediateClickPt(mock14Pts[1]); + }) + .then(function() { + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + }); + }); + }); // describe('is supported by', function() { // }) From d3b4a0f07b9e941648adf3759bf9fc4e8eeb7445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Tue, 21 Aug 2018 15:23:27 +0200 Subject: [PATCH 07/54] Simplify signature of selection testers' contains function [1852] - Instead of an expandingIndex to identify the actual trace wanted, expecting a searchInfo object. --- src/lib/polygon.js | 4 ++-- src/plots/cartesian/select.js | 12 +++++------- src/traces/bar/select.js | 2 +- src/traces/scatter/select.js | 2 +- src/traces/scattergl/index.js | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/lib/polygon.js b/src/lib/polygon.js index 7033e7988e3..29744665b96 100644 --- a/src/lib/polygon.js +++ b/src/lib/polygon.js @@ -198,10 +198,10 @@ polygon.multitester = function multitester(list) { } } - function contains(pt, arg, index, expandedTraceIndex) { + function contains(pt, arg, pointNumber, searchInfo) { var yes = false; for(var i = 0; i < testers.length; i++) { - if(testers[i].contains(pt, arg, index, expandedTraceIndex)) { + if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { // if contained by subtract polygon - exclude the point yes = testers[i].subtract === false; } diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 61fa87a9fe9..45bf50860f3 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -324,10 +324,8 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli } } else { subtract = evt.shiftKey && pointSelected; - currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, - clickedPtInfo.searchInfo.cd[0].trace._expandedIndex, subtract); + currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); - // var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); testPoly = multipolygonTester(concatenatedPolygons); @@ -498,8 +496,7 @@ function extractClickedPtInfo(hoverData, searchTraces) { }; } -// TODO What about passing a searchInfo instead of wantedExpandedTraceIndex? -function createPtNumTester(wantedPointNumber, wantedExpandedTraceIndex, subtract) { +function createPtNumTester(wantedPointNumber, wantedSearchInfo, subtract) { return { xmin: 0, xmax: 0, @@ -507,8 +504,9 @@ function createPtNumTester(wantedPointNumber, wantedExpandedTraceIndex, subtract ymax: 0, pts: [], // TODO Consider making signature of contains more lean - contains: function(pt, omitFirstEdge, pointNumber, expandedTraceIndex) { - return expandedTraceIndex === wantedExpandedTraceIndex && pointNumber === wantedPointNumber; + contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { + return searchInfo.cd[0].trace._expandedIndex === wantedSearchInfo.cd[0].trace._expandedIndex && + pointNumber === wantedPointNumber; }, isRect: false, degenerate: false, diff --git a/src/traces/bar/select.js b/src/traces/bar/select.js index c184a024586..bedc76a8a3a 100644 --- a/src/traces/bar/select.js +++ b/src/traces/bar/select.js @@ -24,7 +24,7 @@ module.exports = function selectPoints(searchInfo, polygon) { for(i = 0; i < cd.length; i++) { var di = cd[i]; - if(polygon.contains(di.ct, false, i, searchInfo.cd[0].trace._expandedIndex)) { + if(polygon.contains(di.ct, false, i, searchInfo)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/scatter/select.js b/src/traces/scatter/select.js index 524f6993d49..6d1e4160c34 100644 --- a/src/traces/scatter/select.js +++ b/src/traces/scatter/select.js @@ -36,7 +36,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(di.x); y = ya.c2p(di.y); - if(polygon.contains([x, y], false, i, searchInfo.cd[0].trace._expandedIndex)) { + if(polygon.contains([x, y], false, i, searchInfo)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 56357555470..26dc0cb18be 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -840,7 +840,7 @@ function selectPoints(searchInfo, polygon) { if(polygon !== false && !polygon.degenerate) { els = [], unels = []; for(i = 0; i < stash.count; i++) { - if(polygon.contains([stash.xpx[i], stash.ypx[i]], false, i, searchInfo.cd[0].trace._expandedIndex)) { + if(polygon.contains([stash.xpx[i], stash.ypx[i]], false, i, searchInfo)) { els.push(i); selection.push({ pointNumber: i, From 36145df2fbfb15308d598af26f3569c35e1cd9ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 22 Aug 2018 10:45:24 +0200 Subject: [PATCH 08/54] Support click-to-select for histograms [1852] --- src/plots/cartesian/select.js | 60 ++++++++++++++++++--- test/jasmine/tests/select_test.js | 87 ++++++++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 9 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 45bf50860f3..093108e4fb6 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -295,7 +295,7 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli var testPoly; var traceSelection; var thisTracesSelection; - var pointSelected; + var pointOrBinSelected; var subtract; var eventData; var i; @@ -304,12 +304,16 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli coerceSelectionsCache(evt, gd, dragOptions); searchTraces = determineSearchTraces(gd, xAxes, yAxes, subplot); var clickedPtInfo = extractClickedPtInfo(hoverData, searchTraces); + var isBinnedTrace = clickedPtInfo.pointNumbers.length > 0; // TODO perf: call potentially costly operation (see impl comment) only when needed - pointSelected = isPointSelected(clickedPtInfo.searchInfo.cd[0].trace, - clickedPtInfo.pointNumber); + pointOrBinSelected = isPointOrBinSelected(clickedPtInfo); - if(pointSelected && isOnlyOnePointSelected(searchTraces)) { + if(pointOrBinSelected && + (isBinnedTrace ? + isOnlyThisBinSelected(searchTraces, clickedPtInfo) : + isOnlyOnePointSelected(searchTraces))) + { // TODO DRY see doubleClick handling above if(polygonOutlines) polygonOutlines.remove(); for(i = 0; i < searchTraces.length; i++) { @@ -323,7 +327,7 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli gd.emit('plotly_deselect', null); } } else { - subtract = evt.shiftKey && pointSelected; + subtract = evt.shiftKey && pointOrBinSelected; currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); @@ -514,7 +518,7 @@ function createPtNumTester(wantedPointNumber, wantedSearchInfo, subtract) { }; } -function isPointSelected(trace, pointNumber) { +function isPointOrBinSelected(clickedPtInfo) { // TODO improve perf // Primarily we need this function to determine if a click adds or subtracts from a selection. // @@ -533,7 +537,49 @@ function isPointSelected(trace, pointNumber) { // So simply searching through those arrays in scattegl would be slow. Just imagine // a user selecting all data points with one lasso polygon. So scattergl would require some // work. - return trace.selectedpoints ? trace.selectedpoints.indexOf(pointNumber) > -1 : false; + var trace = clickedPtInfo.searchInfo.cd[0].trace; + var ptNum = clickedPtInfo.pointNumber; + var ptNums = clickedPtInfo.pointNumbers; + var ptNumsSet = ptNums.length > 0; + + // When pointsNumbers is set (e.g. histogram's binning), + // it is assumed that when the first point of + // a bin is selected, all others are as well + var ptNumToTest = ptNumsSet ? ptNums[0] : ptNum; + + return trace.selectedpoints ? trace.selectedpoints.indexOf(ptNumToTest) > -1 : false; +} + +function isOnlyThisBinSelected(searchTraces, clickedPtInfo) { + var tracesWithSelectedPts = []; + var searchInfo; + var trace; + var isSameTrace; + var i; + + for(i = 0; i < searchTraces.length; i++) { + searchInfo = searchTraces[i]; + if(searchInfo.cd[0].trace.selectedpoints && searchInfo.cd[0].trace.selectedpoints.length > 0) { + tracesWithSelectedPts.push(searchInfo); + } + } + + if(tracesWithSelectedPts.length === 1) { + isSameTrace = tracesWithSelectedPts[0] === clickedPtInfo.searchInfo; + if(isSameTrace) { + trace = clickedPtInfo.searchInfo.cd[0].trace; + if(trace.selectedpoints.length === clickedPtInfo.pointNumbers.length) { + for(i = 0; i > clickedPtInfo.pointNumbers.length; i++) { + if(trace.selectedpoints.indexOf(clickedPtInfo.pointNumbers[i]) < 0) { + return false; + } + } + return true; + } + } + } + + return false; } function isOnlyOnePointSelected(searchTraces) { diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index c7bd3630120..e8d7fcac80c 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -437,6 +437,50 @@ describe('Click-to-select', function() { .then(done); }); + // it('is supported by scattergl in pan/zoom mode', function() { + // failTest('Not implemented'); + // }); + + it('deals correctly with histogram\'s binning in the persistent selection case', function(done) { + var mock = require('@mocks/histogram_colorscale.json'); + var firstBinPts = [0]; + var secondBinPts = [1, 2]; + var thirdBinPts = [3, 4, 5]; + + mock.layout.clickmode = 'event+select'; + Plotly.plot(gd, mock.data, mock.layout) + .then(function() { + return clickFirstBinImmediately(); + }) + .then(function() { + assertSelectedPoints(firstBinPts); + return shiftClickSecondBin(); + }) + .then(function() { + assertSelectedPoints([].concat(firstBinPts, secondBinPts)); + return shiftClickThirdBin(); + }) + .then(function() { + assertSelectedPoints([].concat(firstBinPts, secondBinPts, thirdBinPts)); + return clickFirstBin(); + }) + .then(function() { + assertSelectedPoints([].concat(firstBinPts)); + clickFirstBin(); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + + function clickFirstBinImmediately() { return _immediateClickPt({ x: 141, y: 358 }); } + function clickFirstBin() { return _click(141, 358); } + function shiftClickSecondBin() { return _click(239, 330, { shiftKey: true }); } + function shiftClickThirdBin() { return _click(351, 347, { shiftKey: true }); } + }); + describe('is disabled when clickmode does not include \'select\'', function() { // TODO How to test for pan and zoom mode as well? Note, that // in lasso and select mode, plotly_selected was emitted upon a single @@ -463,8 +507,47 @@ describe('Click-to-select', function() { }); }); - // describe('is supported by', function() { - // }) + describe('is supported by', function() { + + // On loading mocks: note, that require functions are resolved at compile time + // and thus dynamically concatenated mock paths wont't work. + [testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5])] + .forEach(function(testCase) { + it('trace type ' + testCase.traceType, function(done) { + var defaultLayoutOpts = { + layout: { + clickmode: 'event+select', + dragmode: 'pan', + hovermode: 'closest' + } + }; + var mockCopy = Lib.extendDeep( + {}, + testCase.mock, + defaultLayoutOpts); + + Plotly.plot(gd, mockCopy.data, mockCopy.layout) + .then(function() { + return _immediateClickPt(testCase); + }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + }) + .catch(failTest) + .then(done); + }); + }); + + function testCase(traceType, mock, x, y, expectedPts) { + return { + traceType: traceType, + mock: mock, + x: x, + y: y, + expectedPts: expectedPts + }; + } + }); }); describe('@flaky Test select box and lasso in general:', function() { From 73e5085b22ab7f359b9b3239bd3699c0a8e2b599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 22 Aug 2018 11:38:19 +0200 Subject: [PATCH 09/54] Fix click-to-select bug when clearing selection with Shift [1852] - Reason: the selection cache wasn't cleared when a user clicked the last selected point with Shift and thus a new selection started with Shift would add to the still cached selection testers from previous selection. --- src/plots/cartesian/select.js | 5 ++++- test/jasmine/tests/select_test.js | 10 ++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 093108e4fb6..88d378c1bb6 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -314,7 +314,6 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli isOnlyThisBinSelected(searchTraces, clickedPtInfo) : isOnlyOnePointSelected(searchTraces))) { - // TODO DRY see doubleClick handling above if(polygonOutlines) polygonOutlines.remove(); for(i = 0; i < searchTraces.length; i++) { searchInfo = searchTraces[i]; @@ -323,6 +322,10 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli updateSelectedState(gd, searchTraces); + // Clear selection cache to ensure a possible next selection + // starts from a clean slate. + dragOptions.polygons = []; + if(sendEvents) { gd.emit('plotly_deselect', null); } diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index e8d7fcac80c..5a0247c6ce2 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -246,10 +246,16 @@ describe('Click-to-select', function() { .then(function() { return _immediateClickPt(mock14Pts[7]); }) .then(function() { assertSelectedPoints(7); - _clickPt(mock14Pts[7], testData.opts); + _clickPt(mock14Pts[7], testData.clickOpts); return deselectPromise; }) - .then(assertSelectionCleared) + .then(function() { + assertSelectionCleared(); + return _clickPt(mock14Pts[35], testData.clickOpts); + }) + .then(function() { + assertSelectedPoints(35); + }) .catch(failTest) .then(done); }); From 1a89040aa54df4bac58b505ab52b019ffe96642f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 22 Aug 2018 20:40:36 +0200 Subject: [PATCH 10/54] Enable click-to-select for scattergl in pan/zoom mode [1852] --- src/traces/scattergl/index.js | 3 ++- test/jasmine/tests/select_test.js | 28 +++++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 26dc0cb18be..fc0d862bfbe 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -523,6 +523,7 @@ function plot(gd, subplot, cdata) { scene.unselectBatch = null; var dragmode = fullLayout.dragmode; var selectMode = dragmode === 'lasso' || dragmode === 'select'; + var clickSelectEnabled = fullLayout.clickmode.indexOf('select') > -1; for(i = 0; i < cdata.length; i++) { var cd0 = cdata[i][0]; @@ -532,7 +533,7 @@ function plot(gd, subplot, cdata) { var x = stash.x; var y = stash.y; - if(trace.selectedpoints || selectMode) { + if(trace.selectedpoints || selectMode || clickSelectEnabled) { if(!selectMode) selectMode = true; if(!scene.selectBatch) { diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 5a0247c6ce2..2d58945c48f 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -443,9 +443,31 @@ describe('Click-to-select', function() { .then(done); }); - // it('is supported by scattergl in pan/zoom mode', function() { - // failTest('Not implemented'); - // }); + it('is supported by scattergl in pan/zoom mode', function(done) { + Plotly.plot(gd, [ + { + x: [7, 8, 9, 10], + y: [7, 9, 13, 21], + type: 'scattergl', + mode: 'markers', + marker: { size: 20 } + } + ], { + width: 400, + height: 600, + hovermode: 'closest', + dragmode: 'zoom', + clickmode: 'event+select' + }) + .then(function() { + return _click(230, 340, {}, true); + }) + .then(function() { + assertSelectedPoints(2); + }) + .catch(failTest) + .then(done); + }); it('deals correctly with histogram\'s binning in the persistent selection case', function(done) { var mock = require('@mocks/histogram_colorscale.json'); From a6e49fde61fffca2be2cf5ba3d7690f90904b48f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 22 Aug 2018 22:00:59 +0200 Subject: [PATCH 11/54] Add click-to-select support to box plots [1852] --- src/traces/box/event_data.js | 26 +++++++++++++++++++++ src/traces/box/hover.js | 4 ++++ src/traces/box/index.js | 1 + src/traces/box/select.js | 2 +- test/jasmine/tests/select_test.js | 38 ++++++++++++++++++++++++++++++- 5 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 src/traces/box/event_data.js diff --git a/src/traces/box/event_data.js b/src/traces/box/event_data.js new file mode 100644 index 00000000000..bdc1cc88817 --- /dev/null +++ b/src/traces/box/event_data.js @@ -0,0 +1,26 @@ +/** +* Copyright 2012-2018, Plotly, Inc. +* All rights reserved. +* +* This source code is licensed under the MIT license found in the +* LICENSE file in the root directory of this source tree. +*/ + +'use strict'; + +module.exports = function eventData(out, pt) { + if(pt.hoverOnBox) out.hoverOnBox = pt.hoverOnBox; + + // TODO Clean up + if('xVal' in pt) out.x = pt.xVal; + else if('x' in pt) out.x = pt.x; + + if('yVal' in pt) out.y = pt.yVal; + else if('y' in pt) out.y = pt.y; + + if(pt.xa) out.xaxis = pt.xa; + if(pt.ya) out.yaxis = pt.ya; + if(pt.zLabelVal !== undefined) out.z = pt.zLabelVal; + + return out; +}; diff --git a/src/traces/box/hover.js b/src/traces/box/hover.js index 79e24509360..b4729279e1c 100644 --- a/src/traces/box/hover.js +++ b/src/traces/box/hover.js @@ -169,6 +169,10 @@ function hoverOnBoxes(pointData, xval, yval, hovermode) { pointData2[vLetter + 'LabelVal'] = val; pointData2[vLetter + 'Label'] = (t.labels ? t.labels[attr] + ' ' : '') + Axes.hoverLabelText(vAxis, val); + // Note: introduced to be able to distinguish a + // clicked point from a box during click-to-select + pointData2.hoverOnBox = true; + if(attr === 'mean' && ('sd' in di) && trace.boxmean === 'sd') { pointData2[vLetter + 'err'] = di.sd; } diff --git a/src/traces/box/index.js b/src/traces/box/index.js index 3ad049e1701..17931ec782d 100644 --- a/src/traces/box/index.js +++ b/src/traces/box/index.js @@ -20,6 +20,7 @@ Box.plot = require('./plot').plot; Box.style = require('./style').style; Box.styleOnSelect = require('./style').styleOnSelect; Box.hoverPoints = require('./hover').hoverPoints; +Box.eventData = require('./event_data'); Box.selectPoints = require('./select'); Box.moduleType = 'trace'; diff --git a/src/traces/box/select.js b/src/traces/box/select.js index 9ec9ed03e3f..66ab44db849 100644 --- a/src/traces/box/select.js +++ b/src/traces/box/select.js @@ -29,7 +29,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var x = xa.c2p(pt.x); var y = ya.c2p(pt.y); - if(polygon.contains([x, y])) { + if(polygon.contains([x, y], null, pt.i, searchInfo)) { selection.push({ pointNumber: pt.i, x: xa.c2d(pt.x), diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 2d58945c48f..dcccd9d1f1e 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -509,6 +509,39 @@ describe('Click-to-select', function() { function shiftClickThirdBin() { return _click(351, 347, { shiftKey: true }); } }); + it('ignores clicks on boxes in a box trace type', function(done) { + var mock = require('@mocks/box_grouped_horz.json'); + + mock.layout.clickmode = 'event+select'; + mock.layout.width = 1100; + mock.layout.height = 450; + + Plotly.plot(gd, mock.data, mock.layout) + .then(function() { + return clickPtImmediately(); + }) + .then(function() { + assertSelectedPoints(2); + clickPt(); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + clickBox(); + }) + .then(function() { + // TODO Be sure this is called "late enough" after clicking on box has been processed + // Maybe plotly_click event would get fired after any selection events? + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + + function clickPtImmediately() { return _immediateClickPt({ x: 610, y: 342 }); } + function clickPt() { return _clickPt({ x: 610, y: 342 }); } + function clickBox() { return _clickPt({ x: 565, y: 329 }); } + }); + describe('is disabled when clickmode does not include \'select\'', function() { // TODO How to test for pan and zoom mode as well? Note, that // in lasso and select mode, plotly_selected was emitted upon a single @@ -539,7 +572,10 @@ describe('Click-to-select', function() { // On loading mocks: note, that require functions are resolved at compile time // and thus dynamically concatenated mock paths wont't work. - [testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5])] + [ + testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5]), + testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [2]) + ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { var defaultLayoutOpts = { From bb44b6d16263c694daf5945f1ef83afd8aee1bcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 23 Aug 2018 11:00:48 +0200 Subject: [PATCH 12/54] Fix bug when clearing selections and add/subtract mode on [1852] - Ensure click-to-select cleanly clears and starts selections although add/subtract mode on. - Ensure double click in lasso and select mode cleanly clears and restarts selections when add/subtract mode on. --- src/plots/cartesian/select.js | 20 +++++++---- test/jasmine/assets/double_click.js | 15 ++++---- test/jasmine/tests/select_test.js | 53 +++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 88d378c1bb6..96c2c61de9f 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -246,6 +246,9 @@ function prepSelect(e, startX, startY, dragOptions, mode) { } updateSelectedState(gd, searchTraces); + + clearSelectionsCache(dragOptions); + gd.emit('plotly_deselect', null); } else { if(clickmode.indexOf('select') > -1) { @@ -322,9 +325,7 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli updateSelectedState(gd, searchTraces); - // Clear selection cache to ensure a possible next selection - // starts from a clean slate. - dragOptions.polygons = []; + clearSelectionsCache(dragOptions); if(sendEvents) { gd.emit('plotly_deselect', null); @@ -387,10 +388,7 @@ function coerceSelectionsCache(evt, gd, dragOptions) { (!evt.shiftKey && !evt.altKey) || ((evt.shiftKey || evt.altKey) && !plotinfo.selection) ) { - // create new polygons, if shift mode or selecting across different subplots - plotinfo.selection = {}; - plotinfo.selection.polygons = dragOptions.polygons = []; - plotinfo.selection.mergedPolygons = dragOptions.mergedPolygons = []; + clearSelectionsCache(dragOptions); } // clear selection outline when selecting a different subplot @@ -400,6 +398,14 @@ function coerceSelectionsCache(evt, gd, dragOptions) { } } +function clearSelectionsCache(dragOptions) { + var plotinfo = dragOptions.plotinfo; + + plotinfo.selection = {}; + plotinfo.selection.polygons = dragOptions.polygons = []; + plotinfo.selection.mergedPolygons = dragOptions.mergedPolygons = []; +} + function determineSearchTraces(gd, xAxes, yAxes, subplot) { var searchTraces = []; var xAxisIds = xAxes.map(getAxId); diff --git a/test/jasmine/assets/double_click.js b/test/jasmine/assets/double_click.js index 73d444d0782..222ecd7e5a4 100644 --- a/test/jasmine/assets/double_click.js +++ b/test/jasmine/assets/double_click.js @@ -3,22 +3,25 @@ var getNodeCoords = require('./get_node_coords'); var DBLCLICKDELAY = require('../../../src/constants/interactions').DBLCLICKDELAY; /* - * double click on a point. - * you can either specify x,y as pixels, or + * Double click on a point. + * You can either specify x,y as pixels, or * you can specify node and optionally an edge ('n', 'se', 'w' etc) - * to grab it by an edge or corner (otherwise the middle is used) + * to grab it by an edge or corner (otherwise the middle is used). + * You can also pass options for the underlying click, e.g. + * to specify modifier keys. See `click` function + * for more info. */ -module.exports = function doubleClick(x, y) { +module.exports = function doubleClick(x, y, clickOpts) { if(typeof x === 'object') { var coords = getNodeCoords(x, y); x = coords.x; y = coords.y; } return new Promise(function(resolve) { - click(x, y); + click(x, y, clickOpts); setTimeout(function() { - click(x, y); + click(x, y, clickOpts); setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); }, DBLCLICKDELAY / 2); }); diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index dcccd9d1f1e..4453bbb5349 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -262,6 +262,27 @@ describe('Click-to-select', function() { }); }); + it('cleanly clears and starts selections although add/subtract mode on', function(done) { + plotMock14() + .then(function() { + return _immediateClickPt(mock14Pts[7]); + }) + .then(function() { + assertSelectedPoints(7); + _clickPt(mock14Pts[7], { shiftKey: true }); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + return _clickPt(mock14Pts[35], { shiftKey: true }); + }) + .then(function() { + assertSelectedPoints(35); + }) + .catch(failTest) + .then(done); + }); + it('supports adding to an existing selection', function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[7]); }) @@ -1134,6 +1155,38 @@ describe('@flaky Test select box and lasso in general:', function() { .then(done); }); + it('should cleanly clear and restart selections on double click when add/subtract mode on', function(done) { + var gd = createGraphDiv(); + var fig = Lib.extendDeep({}, require('@mocks/0.json')); + + fig.layout.dragmode = 'select'; + Plotly.plot(gd, fig) + .then(function() { + return drag([[350, 100], [400, 400]]); + }) + .then(function() { + _assertSelectedPoints([49, 50, 51, 52, 53, 54, 55, 56, 57]); + return doubleClick(500, 200, { shiftKey: true }); + }) + .then(function() { + _assertSelectedPoints(null); + return drag([[450, 100], [500, 400]], { shiftKey: true }); + }) + .then(function() { + _assertSelectedPoints([67, 68, 69, 70, 71, 72, 73, 74]); + }) + .catch(failTest) + .then(done); + + function _assertSelectedPoints(selPts) { + if(selPts) { + expect(gd.data[0].selectedpoints).toEqual(selPts); + } else { + expect('selectedpoints' in gd.data[0]).toBe(false); + } + } + }); + it('should clear selected points on double click only on pan/lasso modes', function(done) { var gd = createGraphDiv(); var fig = Lib.extendDeep({}, require('@mocks/0.json')); From 48a0a3651f43191a084345653caad444bff754f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 23 Aug 2018 11:58:09 +0200 Subject: [PATCH 13/54] Test violin trace type's support for click-to-test [1852] --- test/jasmine/tests/select_test.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 4453bbb5349..ffa4cd8583e 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -531,7 +531,7 @@ describe('Click-to-select', function() { }); it('ignores clicks on boxes in a box trace type', function(done) { - var mock = require('@mocks/box_grouped_horz.json'); + var mock = Lib.extendDeep({}, require('@mocks/box_grouped_horz.json')); mock.layout.clickmode = 'event+select'; mock.layout.width = 1100; @@ -595,7 +595,10 @@ describe('Click-to-select', function() { // and thus dynamically concatenated mock paths wont't work. [ testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5]), - testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [2]) + testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [[2], [], []], + { width: 1100, height: 450 }), + testCase('violin', require('@mocks/violin_grouped.json'), 166, 187, [[3], [], []], + { width: 1100, height: 450 }) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { @@ -606,10 +609,14 @@ describe('Click-to-select', function() { hovermode: 'closest' } }; + var customLayoutOptions = { + layout: testCase.layoutOptions + }; var mockCopy = Lib.extendDeep( {}, testCase.mock, - defaultLayoutOpts); + defaultLayoutOpts, + customLayoutOptions); Plotly.plot(gd, mockCopy.data, mockCopy.layout) .then(function() { @@ -623,10 +630,11 @@ describe('Click-to-select', function() { }); }); - function testCase(traceType, mock, x, y, expectedPts) { + function testCase(traceType, mock, x, y, expectedPts, layoutOptions) { return { traceType: traceType, mock: mock, + layoutOptions: layoutOptions, x: x, y: y, expectedPts: expectedPts From 463971837e06a54f317538994f94898391107899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 23 Aug 2018 12:30:43 +0200 Subject: [PATCH 14/54] Add click-to-select support to ohlc and candlestick [1852] --- src/traces/ohlc/select.js | 2 +- test/jasmine/tests/select_test.js | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/traces/ohlc/select.js b/src/traces/ohlc/select.js index 29bed35028f..e5555211190 100644 --- a/src/traces/ohlc/select.js +++ b/src/traces/ohlc/select.js @@ -26,7 +26,7 @@ module.exports = function selectPoints(searchInfo, polygon) { for(i = 0; i < cd.length; i++) { var di = cd[i]; - if(polygon.contains([xa.c2p(di.pos + posOffset), ya.c2p(di.yc)])) { + if(polygon.contains([xa.c2p(di.pos + posOffset), ya.c2p(di.yc)], null, di.i, searchInfo)) { selection.push({ pointNumber: di.i, x: xa.c2d(di.pos), diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index ffa4cd8583e..84fb88b05bb 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -598,7 +598,9 @@ describe('Click-to-select', function() { testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [[2], [], []], { width: 1100, height: 450 }), testCase('violin', require('@mocks/violin_grouped.json'), 166, 187, [[3], [], []], - { width: 1100, height: 450 }) + { width: 1100, height: 450 }), + testCase('ohlc', require('@mocks/ohlc_first.json'), 669, 165, [9]), + testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From 497747b2f8510dc794bf1603bac6795edeabedd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 23 Aug 2018 17:19:59 +0200 Subject: [PATCH 15/54] Add click-to-select support to geo base plot [1852] - TODO: deactivated a mouseout listener in geo.js cause with it, click-to-select won't work. To be reviewed. - Include additional for choropleth that's based on geo. - One double click location in an existing choropleth test needed to change though to be on an empty area that is not a world country. --- src/plots/geo/geo.js | 99 ++++++++++++++++++------------- src/traces/choropleth/select.js | 2 +- test/jasmine/tests/select_test.js | 5 +- 3 files changed, 62 insertions(+), 44 deletions(-) diff --git a/src/plots/geo/geo.js b/src/plots/geo/geo.js index e6d419503b5..a6d9feef042 100644 --- a/src/plots/geo/geo.js +++ b/src/plots/geo/geo.js @@ -21,6 +21,7 @@ var Plots = require('../plots'); var Axes = require('../cartesian/axes'); var dragElement = require('../../components/dragelement'); var prepSelect = require('../cartesian/select').prepSelect; +var selectOnClick = require('../cartesian/select').selectOnClick; var createGeoZoom = require('./zoom'); var constants = require('./constants'); @@ -354,6 +355,7 @@ proto.updateFx = function(fullLayout, geoLayout) { var gd = _this.graphDiv; var bgRect = _this.bgRect; var dragMode = fullLayout.dragmode; + var clickMode = fullLayout.clickmode; if(_this.isStatic) return; @@ -376,6 +378,43 @@ proto.updateFx = function(fullLayout, geoLayout) { ]); } + var fillRangeItems; + + if(dragMode === 'select') { + fillRangeItems = function(eventData, poly) { + var ranges = eventData.range = {}; + ranges[_this.id] = [ + invert([poly.xmin, poly.ymin]), + invert([poly.xmax, poly.ymax]) + ]; + }; + } else if(dragMode === 'lasso') { + fillRangeItems = function(eventData, poly, pts) { + var dataPts = eventData.lassoPoints = {}; + dataPts[_this.id] = pts.filtered.map(invert); + }; + } + + // Note: dragOptions is needed to be declared for all dragmodes because + // it's the object that holds persistent selection state. + var dragOptions = { + element: _this.bgRect.node(), + gd: gd, + plotinfo: { + xaxis: _this.xaxis, + yaxis: _this.yaxis, + fillRangeItems: fillRangeItems + }, + xaxes: [_this.xaxis], + yaxes: [_this.yaxis], + subplot: _this.id, + clickFn: function(numClicks) { + if(numClicks === 2) { + fullLayout._zoomlayer.selectAll('.select-outline').remove(); + } + } + }; + if(dragMode === 'pan') { bgRect.node().onmousedown = null; bgRect.call(createGeoZoom(_this, geoLayout)); @@ -384,41 +423,6 @@ proto.updateFx = function(fullLayout, geoLayout) { else if(dragMode === 'select' || dragMode === 'lasso') { bgRect.on('.zoom', null); - var fillRangeItems; - - if(dragMode === 'select') { - fillRangeItems = function(eventData, poly) { - var ranges = eventData.range = {}; - ranges[_this.id] = [ - invert([poly.xmin, poly.ymin]), - invert([poly.xmax, poly.ymax]) - ]; - }; - } else if(dragMode === 'lasso') { - fillRangeItems = function(eventData, poly, pts) { - var dataPts = eventData.lassoPoints = {}; - dataPts[_this.id] = pts.filtered.map(invert); - }; - } - - var dragOptions = { - element: _this.bgRect.node(), - gd: gd, - plotinfo: { - xaxis: _this.xaxis, - yaxis: _this.yaxis, - fillRangeItems: fillRangeItems - }, - xaxes: [_this.xaxis], - yaxes: [_this.yaxis], - 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); }; @@ -440,15 +444,28 @@ proto.updateFx = function(fullLayout, geoLayout) { }); bgRect.on('mouseout', function() { - dragElement.unhover(gd, d3.event); + // TODO Find a way to reactivate this, but with it enabled + // gd._hoverdata would get purged an click-to-select won't + // work. + // dragElement.unhover(gd, d3.event); }); 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); + // For select and lasso the dragElement is handling clicks + if(dragMode !== 'select' && dragMode !== 'lasso') { + if(clickMode.indexOf('event') > -1) { + // 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); + } + + if(clickMode.indexOf('select') > -1) { + selectOnClick(d3.event, gd, [_this.xaxis], [_this.yaxis], + _this.id, dragOptions); + } + } }); }; diff --git a/src/traces/choropleth/select.js b/src/traces/choropleth/select.js index c3a8f332c4d..dfdd9e65a3c 100644 --- a/src/traces/choropleth/select.js +++ b/src/traces/choropleth/select.js @@ -30,7 +30,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(ct); y = ya.c2p(ct); - if(polygon.contains([x, y])) { + if(polygon.contains([x, y], null, i, searchInfo)) { selection.push({ pointNumber: i, lon: ct[0], diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 84fb88b05bb..e7eb9fac8f0 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -600,7 +600,8 @@ describe('Click-to-select', function() { testCase('violin', require('@mocks/violin_grouped.json'), 166, 187, [[3], [], []], { width: 1100, height: 450 }), testCase('ohlc', require('@mocks/ohlc_first.json'), 669, 165, [9]), - testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]) + testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]), + testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { @@ -1909,7 +1910,7 @@ describe('@flaky Test select box and lasso per trace:', function() { }) .then(function() { return _run( - [[370, 120], [500, 200]], null, [280, 190], NOEVENTS, 'choropleth pan' + [[370, 120], [500, 200]], null, [200, 180], NOEVENTS, 'choropleth pan' ); }) .catch(failTest) From 744ab5688b19905596669a274560b28e9e7e271b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 09:02:17 +0200 Subject: [PATCH 16/54] Add click-to-select support to mapbox and scattermapbox [1852] --- src/plots/mapbox/mapbox.js | 68 +++++++++++++++++++++++------- src/traces/scattermapbox/select.js | 2 +- test/jasmine/tests/select_test.js | 29 ++++++++++--- 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index c5332d05fd8..80d3ab89ab1 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -15,6 +15,7 @@ var Fx = require('../../components/fx'); var Lib = require('../../lib'); var dragElement = require('../../components/dragelement'); var prepSelect = require('../cartesian/select').prepSelect; +var selectOnClick = require('../cartesian/select').selectOnClick; var constants = require('./constants'); var layoutAttributes = require('./layout_attributes'); var createMapboxLayer = require('./layers'); @@ -221,11 +222,29 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { gd.emit('plotly_relayout', evtData); } - // define clear select on map creation, to keep one ref per map, + // define event handlers on map creation, to keep one ref per map, // so that map.on / map.off in updateFx works as expected self.clearSelect = function() { gd._fullLayout._zoomlayer.selectAll('.select-outline').remove(); }; + + /** + * Returns a click handler function that is supposed + * to handle clicks in pan mode. + */ + self.onClickInPanFn = function(dragOptions) { + return function(evt) { + var clickMode = gd._fullLayout.clickmode; + + if(clickMode.indexOf('event') > -1) { + Fx.click(gd, evt.originalEvent); + } + + if(clickMode.indexOf('select') > -1) { + selectOnClick(evt.originalEvent, gd, [self.xaxis], [self.yaxis], self.id, dragOptions); + } + }; + }; }; proto.updateMap = function(calcData, fullLayout, resolve, reject) { @@ -382,32 +401,49 @@ proto.updateFx = function(fullLayout) { }; } + // Note: dragOptions is needed to be declared for all dragmodes because + // it's the object that holds persistent selection state. + // Merge old dragOptions with new to keep possibly initialized + // persistent selection state. + var oldDragOptions = self.dragOptions; + self.dragOptions = Lib.extendDeep(oldDragOptions || {}, { + element: self.div, + gd: gd, + plotinfo: { + id: self.id, + xaxis: self.xaxis, + yaxis: self.yaxis, + fillRangeItems: fillRangeItems + }, + xaxes: [self.xaxis], + yaxes: [self.yaxis], + subplot: self.id + }); + if(dragMode === 'select' || dragMode === 'lasso') { map.dragPan.disable(); map.on('zoomstart', self.clearSelect); - var dragOptions = { - element: self.div, - gd: gd, - plotinfo: { - xaxis: self.xaxis, - yaxis: self.yaxis, - fillRangeItems: fillRangeItems - }, - xaxes: [self.xaxis], - yaxes: [self.yaxis], - subplot: self.id - }; - dragOptions.prepFn = function(e, startX, startY) { - prepSelect(e, startX, startY, dragOptions, dragMode); + self.dragOptions.prepFn = function(e, startX, startY) { + prepSelect(e, startX, startY, self.dragOptions, dragMode); }; - dragElement.init(dragOptions); + dragElement.init(self.dragOptions); + + map.off('click', self.onClickInPanHandler); } else { map.dragPan.enable(); map.off('zoomstart', self.clearSelect); self.div.onmousedown = null; + + // 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. + self.onClickInPanHandler = self.onClickInPanFn(self.dragOptions); + map.on('click', self.onClickInPanHandler); } }; diff --git a/src/traces/scattermapbox/select.js b/src/traces/scattermapbox/select.js index ade2ea5ceeb..3b799c718b2 100644 --- a/src/traces/scattermapbox/select.js +++ b/src/traces/scattermapbox/select.js @@ -35,7 +35,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var lonlat2 = [Lib.wrap180(lonlat[0]), lonlat[1]]; var xy = [xa.c2p(lonlat2), ya.c2p(lonlat2)]; - if(polygon.contains(xy)) { + if(polygon.contains(xy, null, i, searchInfo)) { selection.push({ pointNumber: i, lon: lonlat[0], diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index e7eb9fac8f0..d4c1e69b708 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -601,7 +601,9 @@ describe('Click-to-select', function() { { width: 1100, height: 450 }), testCase('ohlc', require('@mocks/ohlc_first.json'), 669, 165, [9]), testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]), - testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]) + testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]), + testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, + { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { @@ -615,16 +617,32 @@ describe('Click-to-select', function() { var customLayoutOptions = { layout: testCase.layoutOptions }; + var customConfigOptions = { + config: testCase.configOptions + }; var mockCopy = Lib.extendDeep( {}, testCase.mock, defaultLayoutOpts, - customLayoutOptions); + customLayoutOptions, + customConfigOptions); - Plotly.plot(gd, mockCopy.data, mockCopy.layout) + Plotly.plot(gd, mockCopy.data, mockCopy.layout, mockCopy.config) .then(function() { return _immediateClickPt(testCase); }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + return Plotly.relayout(gd, 'dragmode', 'lasso'); + }) + .then(function() { + _clickPt(testCase); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + return _clickPt(testCase); + }) .then(function() { assertSelectedPoints(testCase.expectedPts); }) @@ -633,14 +651,15 @@ describe('Click-to-select', function() { }); }); - function testCase(traceType, mock, x, y, expectedPts, layoutOptions) { + function testCase(traceType, mock, x, y, expectedPts, layoutOptions, configOptions) { return { traceType: traceType, mock: mock, layoutOptions: layoutOptions, x: x, y: y, - expectedPts: expectedPts + expectedPts: expectedPts, + configOptions: configOptions }; } }); From 7da973b6c5e219fe6def91bf587c63fc1bba56ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 09:17:22 +0200 Subject: [PATCH 17/54] Add @flaky flag to click-to-select tests [1852] - Reason: some tests ran into async timeout issues on CI. --- test/jasmine/tests/select_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index d4c1e69b708..c7ee6bdc06a 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -120,7 +120,7 @@ var LASSOEVENTS = [4, 2, 1]; var SELECT_PATH = [[93, 193], [143, 193]]; var LASSO_PATH = [[316, 171], [318, 239], [335, 243], [328, 169]]; -describe('Click-to-select', function() { +describe('@flaky Click-to-select', function() { var mock14Pts = { '1': { x: 134, y: 116 }, '7': { x: 270, y: 160 }, From ee5f990bbad803a5a4f2325e2d2ef58e30fbcaec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 09:28:26 +0200 Subject: [PATCH 18/54] Reactivate unhover on mouseout event in geo base plot [1852] --- src/plots/geo/geo.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/plots/geo/geo.js b/src/plots/geo/geo.js index a6d9feef042..f049f6b3e97 100644 --- a/src/plots/geo/geo.js +++ b/src/plots/geo/geo.js @@ -444,10 +444,8 @@ proto.updateFx = function(fullLayout, geoLayout) { }); bgRect.on('mouseout', function() { - // TODO Find a way to reactivate this, but with it enabled - // gd._hoverdata would get purged an click-to-select won't - // work. - // dragElement.unhover(gd, d3.event); + if(gd._dragging) return; + dragElement.unhover(gd, d3.event); }); bgRect.on('click', function() { From 6ac1a087b0cc9243ce54601dfbba0d6c102e22cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 10:17:24 +0200 Subject: [PATCH 19/54] Add click-to-select support to scattergeo [1852] - Thereby fixed bug in geo base plot: set a subplot id in dragOptions so that selection cache isn't coerced each time an interaction happens. --- src/plots/geo/geo.js | 1 + src/traces/scattergeo/select.js | 2 +- test/jasmine/tests/select_test.js | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plots/geo/geo.js b/src/plots/geo/geo.js index f049f6b3e97..80a2f03c04a 100644 --- a/src/plots/geo/geo.js +++ b/src/plots/geo/geo.js @@ -401,6 +401,7 @@ proto.updateFx = function(fullLayout, geoLayout) { element: _this.bgRect.node(), gd: gd, plotinfo: { + id: _this.id, xaxis: _this.xaxis, yaxis: _this.yaxis, fillRangeItems: fillRangeItems diff --git a/src/traces/scattergeo/select.js b/src/traces/scattergeo/select.js index 4c11e6c3196..1d1c656913d 100644 --- a/src/traces/scattergeo/select.js +++ b/src/traces/scattergeo/select.js @@ -38,7 +38,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(lonlat); y = ya.c2p(lonlat); - if(polygon.contains([x, y])) { + if(polygon.contains([x, y], null, i, searchInfo)) { selection.push({ pointNumber: i, lon: lonlat[0], diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index c7ee6bdc06a..c43e2485826 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -603,7 +603,8 @@ describe('@flaky Click-to-select', function() { testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]), testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]), testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, - { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }) + { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), + testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From e11faf54a76fb2d461bb30e9c930843d383132f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 11:30:32 +0200 Subject: [PATCH 20/54] Add click-to-select support to ternary base plot [1852] --- src/plots/ternary/ternary.js | 40 +++++++++++++++++++++---------- test/jasmine/tests/select_test.js | 3 ++- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/plots/ternary/ternary.js b/src/plots/ternary/ternary.js index 436397bc5aa..048bcdd9309 100644 --- a/src/plots/ternary/ternary.js +++ b/src/plots/ternary/ternary.js @@ -25,6 +25,7 @@ var dragElement = require('../../components/dragelement'); var Fx = require('../../components/fx'); var Titles = require('../../components/titles'); var prepSelect = require('../cartesian/select').prepSelect; +var selectOnClick = require('../cartesian/select').selectOnClick; var clearSelect = require('../cartesian/select').clearSelect; var constants = require('../cartesian/constants'); @@ -452,6 +453,7 @@ proto.initInteractions = function() { element: dragger, gd: gd, plotinfo: { + id: _this.id, xaxis: _this.xaxis, yaxis: _this.yaxis }, @@ -472,11 +474,13 @@ proto.initInteractions = function() { if(dragModeNow === 'zoom') { dragOptions.moveFn = zoomMove; + dragOptions.clickFn = clickZoomPan; dragOptions.doneFn = zoomDone; zoomPrep(e, startX, startY); } else if(dragModeNow === 'pan') { dragOptions.moveFn = plotDrag; + dragOptions.clickFn = clickZoomPan; dragOptions.doneFn = dragDone; panPrep(); clearSelect(zoomContainer); @@ -484,24 +488,34 @@ 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); - Registry.call('relayout', gd, attrs); - } - Fx.click(gd, evt, _this.id); } }; var x0, y0, mins0, span0, mins, lum, path0, dimmed, zb, corners; + function clickZoomPan(numClicks, evt) { + var clickMode = gd._fullLayout.clickmode; + + 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); + Registry.call('relayout', gd, attrs); + } + + if(clickMode.indexOf('event') > -1) { + Fx.click(gd, evt, _this.id); + } + + if(clickMode.indexOf('select') > -1 && numClicks === 1) { + selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOptions); + } + } + function zoomPrep(e, startX, startY) { var dragBBox = dragger.getBoundingClientRect(); x0 = startX - dragBBox.left; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index c43e2485826..d9e6b1869d4 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -604,7 +604,8 @@ describe('@flaky Click-to-select', function() { testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]), testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), - testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]) + testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), + testCase('scatterternary', require('@mocks/ternary_markers.json'), 485, 335, [7]) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From bef47a3cac6ada4134ca448ec9cca8470942172f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 13:53:26 +0200 Subject: [PATCH 21/54] Add test of scattercarpet's click-to-select support [1852] --- test/jasmine/tests/select_test.js | 45 ++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index d9e6b1869d4..c0995042687 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -195,28 +195,38 @@ describe('@flaky Click-to-select', function() { * * @param expected can be a point number, an array * of point numbers (for a single trace) or an array of point number - * arrays in case of multiple traces. + * arrays in case of multiple traces. undefined in an array of arrays + * is also allowed, e.g. useful when not all traces support selection. */ function assertSelectedPoints(expected) { - var expectedPtsPerTrace; + var expectedPtsPerTrace = toArrayOfArrays(expected); var expectedPts; var traceNum; - if(Array.isArray(expected)) { - if(Array.isArray(expected[0])) { - expectedPtsPerTrace = expected; - } else { - expectedPtsPerTrace = [expected]; - } - } else { - expectedPtsPerTrace = [[expected]]; - } - for(traceNum = 0; traceNum < expectedPtsPerTrace.length; traceNum++) { expectedPts = expectedPtsPerTrace[traceNum]; expect(gd._fullData[traceNum].selectedpoints).toEqual(expectedPts); expect(gd.data[traceNum].selectedpoints).toEqual(expectedPts); } + + function toArrayOfArrays(expected) { + var isArrayInArray; + var i; + + if(Array.isArray(expected)) { + isArrayInArray = false; + for(i = 0; i < expected.length; i++) { + if(Array.isArray(expected[i])) { + isArrayInArray = true; + break; + } + } + + return isArrayInArray ? expected : [expected]; + } else { + return [[expected]]; + } + } } function assertSelectionCleared() { @@ -591,8 +601,11 @@ describe('@flaky Click-to-select', function() { describe('is supported by', function() { - // On loading mocks: note, that require functions are resolved at compile time - // and thus dynamically concatenated mock paths wont't work. + // On loading mocks: + // - Note, that `require` function calls are resolved at compile time + // and thus dynamically concatenated mock paths won't work. + // - Some mocks don't specify a width and height, so this needs + // to be set explicitly to ensure click coordinates fit. [ testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5]), testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [[2], [], []], @@ -605,7 +618,9 @@ describe('@flaky Click-to-select', function() { testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), - testCase('scatterternary', require('@mocks/ternary_markers.json'), 485, 335, [7]) + testCase('scatterternary', require('@mocks/ternary_markers.json'), 485, 335, [7]), + testCase('scattercarpet', require('@mocks/scattercarpet.json'), 532, 178, + [undefined, [], [], [], [], [], [2]], { width: 1100, height: 450 }) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From c394a8b0dc09ae9a54c9ec9107498c235ce201aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 14:39:20 +0200 Subject: [PATCH 22/54] Add click-to-select support to polar base plot [1852] --- src/plots/polar/polar.js | 45 +++++++++++++++++++------------ test/jasmine/tests/select_test.js | 9 ++++++- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/plots/polar/polar.js b/src/plots/polar/polar.js index e300807ed1c..814b7e5f252 100644 --- a/src/plots/polar/polar.js +++ b/src/plots/polar/polar.js @@ -23,6 +23,7 @@ var dragBox = require('../cartesian/dragbox'); var Fx = require('../../components/fx'); var Titles = require('../../components/titles'); var prepSelect = require('../cartesian/select').prepSelect; +var selectOnClick = require('../cartesian/select').selectOnClick; var clearSelect = require('../cartesian/select').clearSelect; var setCursor = require('../../lib/setcursor'); var polygonTester = require('../../lib/polygon').tester; @@ -592,6 +593,7 @@ proto.updateMainDrag = function(fullLayout, polarLayout) { gd: gd, subplot: _this.id, plotinfo: { + id: _this.id, xaxis: _this.xaxis, yaxis: _this.yaxis }, @@ -811,6 +813,31 @@ proto.updateMainDrag = function(fullLayout, polarLayout) { Registry.call('relayout', gd, updateObj); } + function zoomClick(numClicks, evt) { + var clickMode = gd._fullLayout.clickmode; + + dragBox.removeZoombox(gd); + + // TODO double once vs twice logic (autorange vs fixed range) + if(numClicks === 2) { + var updateObj = {}; + for(var k in _this.viewInitial) { + updateObj[_this.id + '.' + k] = _this.viewInitial[k]; + } + + gd.emit('plotly_doubleclick', null); + Registry.call('relayout', gd, updateObj); + } + + if(clickMode.indexOf('event') > -1) { + Fx.click(gd, evt, _this.id); + } + + if(clickMode.indexOf('select') > -1 && numClicks === 1) { + selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOpts); + } + } + dragOpts.prepFn = function(evt, startX, startY) { var dragModeNow = gd._fullLayout.dragmode; @@ -833,6 +860,7 @@ proto.updateMainDrag = function(fullLayout, polarLayout) { } else { dragOpts.moveFn = zoomMove; } + dragOpts.clickFn = zoomClick; dragOpts.doneFn = zoomDone; zoomPrep(evt, startX, startY); break; @@ -843,23 +871,6 @@ proto.updateMainDrag = function(fullLayout, polarLayout) { } }; - dragOpts.clickFn = function(numClicks, evt) { - dragBox.removeZoombox(gd); - - // TODO double once vs twice logic (autorange vs fixed range) - if(numClicks === 2) { - var updateObj = {}; - for(var k in _this.viewInitial) { - updateObj[_this.id + '.' + k] = _this.viewInitial[k]; - } - - gd.emit('plotly_doubleclick', null); - Registry.call('relayout', gd, updateObj); - } - - Fx.click(gd, evt, _this.id); - }; - mainDrag.onmousemove = function(evt) { Fx.hover(gd, evt, _this.id); gd._fullLayout._lasthover = mainDrag; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index c0995042687..3af3f1b573e 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -619,8 +619,15 @@ describe('@flaky Click-to-select', function() { { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), testCase('scatterternary', require('@mocks/ternary_markers.json'), 485, 335, [7]), + + // Note that first trace (carpet) in mock doesn't support selection, + // thus undefined is expected testCase('scattercarpet', require('@mocks/scattercarpet.json'), 532, 178, - [undefined, [], [], [], [], [], [2]], { width: 1100, height: 450 }) + [undefined, [], [], [], [], [], [2]], { width: 1100, height: 450 }), + + // scatterpolar does not support pan (the default), so set dragmode to zoom + testCase('scatterpolar', require('@mocks/polar_scatter.json'), 130, 290, + [[], [], [], [19], [], []], { dragmode: 'zoom' }) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From 5b08a2c39efc7c8b0949b735da644a0a5b740be8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 14:57:26 +0200 Subject: [PATCH 23/54] Add testing scatterpolargl's click-to-select support [1852] --- test/jasmine/tests/select_test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 3af3f1b573e..19e81aed90d 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -625,8 +625,11 @@ describe('@flaky Click-to-select', function() { testCase('scattercarpet', require('@mocks/scattercarpet.json'), 532, 178, [undefined, [], [], [], [], [], [2]], { width: 1100, height: 450 }), - // scatterpolar does not support pan (the default), so set dragmode to zoom + // scatterpolar and scatterpolargl do not support pan (the default), + // so set dragmode to zoom testCase('scatterpolar', require('@mocks/polar_scatter.json'), 130, 290, + [[], [], [], [19], [], []], { dragmode: 'zoom' }), + testCase('scatterpolargl', require('@mocks/glpolar_scatter.json'), 130, 290, [[], [], [], [19], [], []], { dragmode: 'zoom' }) ] .forEach(function(testCase) { From e76396456692e83ed8769e38a7603183d89b2451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 24 Aug 2018 15:42:28 +0200 Subject: [PATCH 24/54] Add click-to-select support to splom [1852] --- src/traces/splom/index.js | 6 ++++-- test/jasmine/tests/select_test.js | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/traces/splom/index.js b/src/traces/splom/index.js index c5613e5844f..43e44df2ed1 100644 --- a/src/traces/splom/index.js +++ b/src/traces/splom/index.js @@ -221,7 +221,9 @@ function plotOne(gd, cd0) { scene.matrix = createMatrix(regl); } - var selectMode = dragmode === 'lasso' || dragmode === 'select' || !!trace.selectedpoints; + var clickSelectEnabled = fullLayout.clickmode.indexOf('select') > -1; + var selectMode = dragmode === 'lasso' || dragmode === 'select' || + !!trace.selectedpoints || clickSelectEnabled; scene.selectBatch = null; scene.unselectBatch = null; @@ -372,7 +374,7 @@ function selectPoints(searchInfo, polygon) { if(polygon !== false && !polygon.degenerate) { els = [], unels = []; for(i = 0; i < x.length; i++) { - if(polygon.contains([xpx[i], ypx[i]])) { + if(polygon.contains([xpx[i], ypx[i]], null, i, searchInfo)) { els.push(i); selection.push({ pointNumber: i, diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 19e81aed90d..3202657e70f 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -630,7 +630,8 @@ describe('@flaky Click-to-select', function() { testCase('scatterpolar', require('@mocks/polar_scatter.json'), 130, 290, [[], [], [], [19], [], []], { dragmode: 'zoom' }), testCase('scatterpolargl', require('@mocks/glpolar_scatter.json'), 130, 290, - [[], [], [], [19], [], []], { dragmode: 'zoom' }) + [[], [], [], [19], [], []], { dragmode: 'zoom' }), + testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]) ] .forEach(function(testCase) { it('trace type ' + testCase.traceType, function(done) { From 41e3ba5ad032848ce15b9cb45ac3d4151ef26267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 08:42:58 +0200 Subject: [PATCH 25/54] Activate persistent selection for ternary plots [1852] - Ternary was the only base plot with inconsistent behavior in terms of holding the Shift key. It allowed to temporarily pan (zoom when in pan) in zoom, lasso and select drag modes. --- src/plots/ternary/ternary.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/plots/ternary/ternary.js b/src/plots/ternary/ternary.js index 048bcdd9309..d10fc985094 100644 --- a/src/plots/ternary/ternary.js +++ b/src/plots/ternary/ternary.js @@ -464,10 +464,6 @@ proto.initInteractions = function() { dragOptions.xaxes = [_this.xaxis]; dragOptions.yaxes = [_this.yaxis]; var dragModeNow = gd._fullLayout.dragmode; - if(e.shiftKey) { - if(dragModeNow === 'pan') dragModeNow = 'zoom'; - else dragModeNow = 'pan'; - } if(dragModeNow === 'lasso') dragOptions.minDrag = 1; else dragOptions.minDrag = undefined; From 6a2ae3bf0e69cbaed2ed7095e5dbb97f99e9b2eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 09:13:05 +0200 Subject: [PATCH 26/54] Refine click-to-select TODO comments that'll be solved later [1852] - Updates of TODO comments are based on discussions on Github and Slack. --- src/plots/cartesian/dragbox.js | 7 ++++--- src/plots/cartesian/select.js | 25 +++++++------------------ 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index 884d50c4a6e..e9ca4d65fad 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -189,10 +189,11 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { // triggers at least one interaction in pan/zoom mode. Otherwise, the // select/lasso outlines are deleted (in plots.js.cleanPlot) but the selection // cache isn't cleared. So when the user switches back to select/lasso and - // 'add to a selection' with Shift, the "old", seemingly removed outlines - // are redrawn again because the selection cache still holds their points. + // 'adds to a selection' with Shift, the "old", seemingly removed outlines + // are redrawn again because the selection cache still holds their coordinates. // However, this isn't easily solved, since plots.js would need - // to have a reference to the dragOptions object. + // to have a reference to the dragOptions object (which holds the + // selection cache). clearAndResetSelect(); } diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 96c2c61de9f..f86304cdcc1 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -259,6 +259,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { if(clickmode === 'event') { // 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. + // Note that click-to-select introduced pre v2 also emitts proper + // event data when clickmode is having 'select' in its flag list. gd.emit('plotly_selected', undefined); } } @@ -528,24 +530,6 @@ function createPtNumTester(wantedPointNumber, wantedSearchInfo, subtract) { } function isPointOrBinSelected(clickedPtInfo) { - // TODO improve perf - // Primarily we need this function to determine if a click adds or subtracts from a selection. - // - // IME best user experience would be - // - that Shift+Click an unselected points adds to selection - // - and Shift+Click a selected point subtracts from selection. - // - // Several options: - // 1. Avoid problem at all by binding subtract-selection-by-click operation to Shift+Alt-Click. - // Slightly less intuitive. A lot of programs deselect an already selected element when you - // Shift+Click it. - // 2. Delegate decision to the traces module through an additional - // isSelected(searchInfo, pointNumber) function. Traces like scatter or bar have - // a selected flag attached to each calcData element, thus access to that information - // would be fast. However, scattergl only maintains selectBatch and unselectBatch arrays. - // So simply searching through those arrays in scattegl would be slow. Just imagine - // a user selecting all data points with one lasso polygon. So scattergl would require some - // work. var trace = clickedPtInfo.searchInfo.cd[0].trace; var ptNum = clickedPtInfo.pointNumber; var ptNums = clickedPtInfo.pointNumbers; @@ -556,6 +540,11 @@ function isPointOrBinSelected(clickedPtInfo) { // a bin is selected, all others are as well var ptNumToTest = ptNumsSet ? ptNums[0] : ptNum; + // TODO potential performance improvement + // Primarily we need this function to determine if a click adds + // or subtracts from a selection. + // In cases `trace.selectedpoints` is a huge array, indexOf + // might be slow. One remedy would be to introduce a hash somewhere. return trace.selectedpoints ? trace.selectedpoints.indexOf(ptNumToTest) > -1 : false; } From e7c2240bc563dece9296354d505a0e17b5d1bafc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 09:23:53 +0200 Subject: [PATCH 27/54] Clean up box' custom eventData function [1852] --- src/traces/box/event_data.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/traces/box/event_data.js b/src/traces/box/event_data.js index bdc1cc88817..a12ee8eb67a 100644 --- a/src/traces/box/event_data.js +++ b/src/traces/box/event_data.js @@ -9,18 +9,16 @@ 'use strict'; module.exports = function eventData(out, pt) { + + // Note: hoverOnBox property is needed for click-to-select + // to ignore when a box was clicked. This is the reason box + // implements this custom eventData function. if(pt.hoverOnBox) out.hoverOnBox = pt.hoverOnBox; - // TODO Clean up if('xVal' in pt) out.x = pt.xVal; - else if('x' in pt) out.x = pt.x; - if('yVal' in pt) out.y = pt.yVal; - else if('y' in pt) out.y = pt.y; - if(pt.xa) out.xaxis = pt.xa; if(pt.ya) out.yaxis = pt.ya; - if(pt.zLabelVal !== undefined) out.z = pt.zLabelVal; return out; }; From 23024b2f045f186fbd002cde737ee15dd2092864 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 09:54:58 +0200 Subject: [PATCH 28/54] Call click-to-select's isPointOrBinSelected as late as possible [1852] - Reason: isPointOrBinSelected is a potentially costly operation and thus leverage an assignment in an if condition trick to avoid executing isPointOrBinSelected unnecessarily. --- src/plots/cartesian/select.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index f86304cdcc1..ffb1394688f 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -311,13 +311,14 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli var clickedPtInfo = extractClickedPtInfo(hoverData, searchTraces); var isBinnedTrace = clickedPtInfo.pointNumbers.length > 0; - // TODO perf: call potentially costly operation (see impl comment) only when needed - pointOrBinSelected = isPointOrBinSelected(clickedPtInfo); - if(pointOrBinSelected && - (isBinnedTrace ? + // Note: potentially costly operation isPointOrBinSelected is + // called as late as possible through the use of an assignment + // in an if condition. + if((isBinnedTrace ? isOnlyThisBinSelected(searchTraces, clickedPtInfo) : - isOnlyOnePointSelected(searchTraces))) + isOnlyOnePointSelected(searchTraces)) && + (pointOrBinSelected = isPointOrBinSelected(clickedPtInfo))) { if(polygonOutlines) polygonOutlines.remove(); for(i = 0; i < searchTraces.length; i++) { @@ -333,7 +334,10 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli gd.emit('plotly_deselect', null); } } else { - subtract = evt.shiftKey && pointOrBinSelected; + subtract = evt.shiftKey && + (pointOrBinSelected !== undefined ? + pointOrBinSelected : + isPointOrBinSelected(clickedPtInfo)); currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); From cbd9f0769e69fc0a09ef5415d94eaa167bcf46fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 10:40:32 +0200 Subject: [PATCH 29/54] Refactor selection testing namings and structure [1852] - Renaming because since click-to-select, selections not only can be defined by polygons, but also by point numbers. Up until now code was centered around polygons being the sole means to define a selection. Renamed identifiers to be more general. - Extract `multitester` to new module `lib/select.js` because `multitester` is no longer to been seen in the sole context of polygons. --- src/lib/polygon.js | 27 +++---- src/lib/select.js | 116 +++++++++++++++++++++++++++++ src/plots/cartesian/select.js | 62 ++++++--------- src/traces/bar/select.js | 6 +- src/traces/box/select.js | 6 +- src/traces/choropleth/select.js | 6 +- src/traces/ohlc/select.js | 6 +- src/traces/scatter/select.js | 6 +- src/traces/scattergeo/select.js | 6 +- src/traces/scattergl/index.js | 6 +- src/traces/scattermapbox/select.js | 6 +- src/traces/splom/index.js | 6 +- 12 files changed, 178 insertions(+), 81 deletions(-) create mode 100644 src/lib/select.js diff --git a/src/lib/polygon.js b/src/lib/polygon.js index 29744665b96..a282d232209 100644 --- a/src/lib/polygon.js +++ b/src/lib/polygon.js @@ -174,34 +174,31 @@ polygon.tester = function tester(ptsIn) { }; }; +// TODO Somewhat redundant to multiTester in 'lib/select.js' /** * Test multiple polygons */ polygon.multitester = function multitester(list) { var testers = [], - xmin = list[0].contains ? 0 : list[0][0][0], + xmin = list[0][0][0], xmax = xmin, - ymin = list[0].contains ? 0 : list[0][0][1], + ymin = list[0][0][1], ymax = ymin; for(var i = 0; i < list.length; i++) { - if(list[i].contains) { - testers.push(list[i]); - } else { - var tester = polygon.tester(list[i]); - tester.subtract = list[i].subtract; - testers.push(tester); - xmin = Math.min(xmin, tester.xmin); - xmax = Math.max(xmax, tester.xmax); - ymin = Math.min(ymin, tester.ymin); - ymax = Math.max(ymax, tester.ymax); - } + var tester = polygon.tester(list[i]); + tester.subtract = list[i].subtract; + testers.push(tester); + xmin = Math.min(xmin, tester.xmin); + xmax = Math.max(xmax, tester.xmax); + ymin = Math.min(ymin, tester.ymin); + ymax = Math.max(ymax, tester.ymax); } - function contains(pt, arg, pointNumber, searchInfo) { + function contains(pt, arg) { var yes = false; for(var i = 0; i < testers.length; i++) { - if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { + if(testers[i].contains(pt, arg)) { // if contained by subtract polygon - exclude the point yes = testers[i].subtract === false; } diff --git a/src/lib/select.js b/src/lib/select.js new file mode 100644 index 00000000000..14d6143fb83 --- /dev/null +++ b/src/lib/select.js @@ -0,0 +1,116 @@ +/** +* Copyright 2012-2018, Plotly, Inc. +* All rights reserved. +* +* This source code is licensed under the MIT license found in the +* LICENSE file in the root directory of this source tree. +*/ + + +'use strict'; + +var polygon = require('./polygon'); + +var select = module.exports = {}; + +/** + * Constructs a new point selection definition. + * + * @param pointNumber the point number of the point as in `data` + * @param searchInfo identifies the trace the point is contained in + * @param subtract true if the selection definition should mark a deselection + * @return {{pointNumber: Number, searchInfo: Object, subtract: boolean}} + */ +select.pointSelectionDef = function(pointNumber, searchInfo, subtract) { + return { + pointNumber: pointNumber, + searchInfo: searchInfo, + subtract: subtract + }; +}; + +function isPointSelectionDef(o) { + return 'pointNumber' in o && 'searchInfo' in o; +} + +function pointNumberTester(pointSelectionDef) { + return { + xmin: 0, + xmax: 0, + ymin: 0, + ymax: 0, + pts: [], + contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { + return searchInfo.cd[0].trace._expandedIndex === pointSelectionDef.searchInfo.cd[0].trace._expandedIndex && + pointNumber === pointSelectionDef.pointNumber; + }, + isRect: false, + degenerate: false, + subtract: pointSelectionDef.subtract + }; +} + +/** + * Wraps multiple selection testers. + * + * @param list an array of selection testers + * + * @return a selection tester object with a contains function + * that can be called to evaluate a point against all wrapped + * selection testers that were passed in list. + */ +select.multiTester = function multiTester(list) { + var testers = []; + var xmin = isPointSelectionDef(list[0]) ? 0 : list[0][0][0]; + var xmax = xmin; + var ymin = isPointSelectionDef(list[0]) ? 0 : list[0][0][1]; + var ymax = ymin; + + for(var i = 0; i < list.length; i++) { + if(isPointSelectionDef(list[i])) { + testers.push(pointNumberTester(list[i])); + } else { + var tester = polygon.tester(list[i]); + tester.subtract = list[i].subtract; + testers.push(tester); + xmin = Math.min(xmin, tester.xmin); + xmax = Math.max(xmax, tester.xmax); + ymin = Math.min(ymin, tester.ymin); + ymax = Math.max(ymax, tester.ymax); + } + } + + // TODO Consider making signature of contains more lean + /** + * Tests if the given point is within this tester. + * + * @param pt an object having an `x` and a `y` property defining the location + * of the point + * @param arg parameter to pass additional arguments down to wrapped testers + * @param pointNumber the point number of the point + * @param searchInfo identifies the trace the point is contained in + * @return {boolean} + */ + function contains(pt, arg, pointNumber, searchInfo) { + var yes = false; + for(var i = 0; i < testers.length; i++) { + if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { + // if contained by subtract tester - exclude the point + yes = testers[i].subtract === false; + } + } + + return yes; + } + + return { + xmin: xmin, + xmax: xmax, + ymin: ymin, + ymax: ymax, + pts: [], + contains: contains, + isRect: false, + degenerate: false + }; +}; diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index ffb1394688f..23beb9da25c 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -16,6 +16,7 @@ var Color = require('../../components/color'); var Fx = require('../../components/fx'); var polygon = require('../../lib/polygon'); +var select = require('../../lib/select'); var throttle = require('../../lib/throttle'); var makeEventData = require('../../components/fx/helpers').makeEventData; var getFromId = require('./axis_ids').getFromId; @@ -26,7 +27,8 @@ var MINSELECT = constants.MINSELECT; var filteredPolygon = polygon.filter; var polygonTester = polygon.tester; -var multipolygonTester = polygon.multitester; +var pointSelectionDef = select.pointSelectionDef; +var multiTester = select.multiTester; function getAxId(ax) { return ax._id; } @@ -50,7 +52,7 @@ function prepSelect(e, startX, startY, dragOptions, mode) { var allAxes = dragOptions.xaxes.concat(dragOptions.yaxes); var subtract = e.altKey; - var filterPoly, testPoly, mergedPolygons, currentPolygon; + var filterPoly, selectionTester, mergedPolygons, currentPolygon; var i, searchInfo, eventData; coerceSelectionsCache(e, gd, dragOptions); @@ -185,14 +187,14 @@ function prepSelect(e, startX, startY, dragOptions, mode) { } // create outline & tester - if(dragOptions.polygons && dragOptions.polygons.length) { + if(dragOptions.selectionDefs && dragOptions.selectionDefs.length) { mergedPolygons = mergePolygons(dragOptions.mergedPolygons, currentPolygon, subtract); currentPolygon.subtract = subtract; - testPoly = multipolygonTester(dragOptions.polygons.concat([currentPolygon])); + selectionTester = multiTester(dragOptions.selectionDefs.concat([currentPolygon])); } else { mergedPolygons = [currentPolygon]; - testPoly = polygonTester(currentPolygon); + selectionTester = polygonTester(currentPolygon); } // draw selection @@ -209,7 +211,7 @@ function prepSelect(e, startX, startY, dragOptions, mode) { for(i = 0; i < searchTraces.length; i++) { searchInfo = searchTraces[i]; - traceSelection = searchInfo._module.selectPoints(searchInfo, testPoly); + traceSelection = searchInfo._module.selectPoints(searchInfo, selectionTester); traceSelections.push(traceSelection); thisSelection = fillSelectionItem(traceSelection, searchInfo); @@ -276,10 +278,10 @@ function prepSelect(e, startX, startY, dragOptions, mode) { throttle.clear(throttleID); dragOptions.gd.emit('plotly_selected', eventData); - if(currentPolygon && dragOptions.polygons) { + if(currentPolygon && dragOptions.selectionDefs) { // save last polygons currentPolygon.subtract = subtract; - dragOptions.polygons.push(currentPolygon); + dragOptions.selectionDefs.push(currentPolygon); // we have to keep reference to arrays container dragOptions.mergedPolygons.length = 0; @@ -296,8 +298,8 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli var selection = []; var searchTraces; var searchInfo; - var currentPolygon; - var testPoly; + var currentSelectionDef; + var selectionTester; var traceSelection; var thisTracesSelection; var pointOrBinSelected; @@ -338,13 +340,13 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli (pointOrBinSelected !== undefined ? pointOrBinSelected : isPointOrBinSelected(clickedPtInfo)); - currentPolygon = createPtNumTester(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); + currentSelectionDef = pointSelectionDef(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); - var concatenatedPolygons = dragOptions.polygons.concat([currentPolygon]); - testPoly = multipolygonTester(concatenatedPolygons); + var allSelectionDefs = dragOptions.selectionDefs.concat([currentSelectionDef]); + selectionTester = multiTester(allSelectionDefs); for(i = 0; i < searchTraces.length; i++) { - traceSelection = searchTraces[i]._module.selectPoints(searchTraces[i], testPoly); + traceSelection = searchTraces[i]._module.selectPoints(searchTraces[i], selectionTester); thisTracesSelection = fillSelectionItem(traceSelection, searchTraces[i]); if(selection.length) { @@ -358,8 +360,8 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli eventData = {points: selection}; updateSelectedState(gd, searchTraces, eventData); - if(currentPolygon && dragOptions) { - dragOptions.polygons.push(currentPolygon); + if(currentSelectionDef && dragOptions) { + dragOptions.selectionDefs.push(currentSelectionDef); } if(polygonOutlines) drawSelection(dragOptions.mergedPolygons, polygonOutlines); @@ -384,11 +386,11 @@ function coerceSelectionsCache(evt, gd, dragOptions) { if( selectingOnSameSubplot && (evt.shiftKey || evt.altKey) && - (plotinfo.selection && plotinfo.selection.polygons) && - !dragOptions.polygons + (plotinfo.selection && plotinfo.selection.selectionDefs) && + !dragOptions.selectionDefs ) { - // take over selection polygons from prev mode, if any - dragOptions.polygons = plotinfo.selection.polygons; + // take over selection definitions from prev mode, if any + dragOptions.selectionDefs = plotinfo.selection.selectionDefs; dragOptions.mergedPolygons = plotinfo.selection.mergedPolygons; } else if( (!evt.shiftKey && !evt.altKey) || @@ -408,7 +410,7 @@ function clearSelectionsCache(dragOptions) { var plotinfo = dragOptions.plotinfo; plotinfo.selection = {}; - plotinfo.selection.polygons = dragOptions.polygons = []; + plotinfo.selection.selectionDefs = dragOptions.selectionDefs = []; plotinfo.selection.mergedPolygons = dragOptions.mergedPolygons = []; } @@ -515,24 +517,6 @@ function extractClickedPtInfo(hoverData, searchTraces) { }; } -function createPtNumTester(wantedPointNumber, wantedSearchInfo, subtract) { - return { - xmin: 0, - xmax: 0, - ymin: 0, - ymax: 0, - pts: [], - // TODO Consider making signature of contains more lean - contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { - return searchInfo.cd[0].trace._expandedIndex === wantedSearchInfo.cd[0].trace._expandedIndex && - pointNumber === wantedPointNumber; - }, - isRect: false, - degenerate: false, - subtract: subtract - }; -} - function isPointOrBinSelected(clickedPtInfo) { var trace = clickedPtInfo.searchInfo.cd[0].trace; var ptNum = clickedPtInfo.pointNumber; diff --git a/src/traces/bar/select.js b/src/traces/bar/select.js index bedc76a8a3a..4d80b7b4836 100644 --- a/src/traces/bar/select.js +++ b/src/traces/bar/select.js @@ -8,14 +8,14 @@ 'use strict'; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; var selection = []; var i; - if(polygon === false) { + if(selectionTester === false) { // clear selection for(i = 0; i < cd.length; i++) { cd[i].selected = 0; @@ -24,7 +24,7 @@ module.exports = function selectPoints(searchInfo, polygon) { for(i = 0; i < cd.length; i++) { var di = cd[i]; - if(polygon.contains(di.ct, false, i, searchInfo)) { + if(selectionTester.contains(di.ct, false, i, searchInfo)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/box/select.js b/src/traces/box/select.js index 66ab44db849..069b9b1896f 100644 --- a/src/traces/box/select.js +++ b/src/traces/box/select.js @@ -8,14 +8,14 @@ 'use strict'; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; var selection = []; var i, j; - if(polygon === false) { + if(selectionTester === false) { for(i = 0; i < cd.length; i++) { for(j = 0; j < (cd[i].pts || []).length; j++) { // clear selection @@ -29,7 +29,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var x = xa.c2p(pt.x); var y = ya.c2p(pt.y); - if(polygon.contains([x, y], null, pt.i, searchInfo)) { + if(selectionTester.contains([x, y], null, pt.i, searchInfo)) { selection.push({ pointNumber: pt.i, x: xa.c2d(pt.x), diff --git a/src/traces/choropleth/select.js b/src/traces/choropleth/select.js index dfdd9e65a3c..9052c06a74e 100644 --- a/src/traces/choropleth/select.js +++ b/src/traces/choropleth/select.js @@ -8,7 +8,7 @@ 'use strict'; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; @@ -16,7 +16,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var i, di, ct, x, y; - if(polygon === false) { + if(selectionTester === false) { for(i = 0; i < cd.length; i++) { cd[i].selected = 0; } @@ -30,7 +30,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(ct); y = ya.c2p(ct); - if(polygon.contains([x, y], null, i, searchInfo)) { + if(selectionTester.contains([x, y], null, i, searchInfo)) { selection.push({ pointNumber: i, lon: ct[0], diff --git a/src/traces/ohlc/select.js b/src/traces/ohlc/select.js index e5555211190..a588e2ac164 100644 --- a/src/traces/ohlc/select.js +++ b/src/traces/ohlc/select.js @@ -8,7 +8,7 @@ 'use strict'; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; @@ -17,7 +17,7 @@ module.exports = function selectPoints(searchInfo, polygon) { // for (potentially grouped) candlesticks var posOffset = cd[0].t.bPos || 0; - if(polygon === false) { + if(selectionTester === false) { // clear selection for(i = 0; i < cd.length; i++) { cd[i].selected = 0; @@ -26,7 +26,7 @@ module.exports = function selectPoints(searchInfo, polygon) { for(i = 0; i < cd.length; i++) { var di = cd[i]; - if(polygon.contains([xa.c2p(di.pos + posOffset), ya.c2p(di.yc)], null, di.i, searchInfo)) { + if(selectionTester.contains([xa.c2p(di.pos + posOffset), ya.c2p(di.yc)], null, di.i, searchInfo)) { selection.push({ pointNumber: di.i, x: xa.c2d(di.pos), diff --git a/src/traces/scatter/select.js b/src/traces/scatter/select.js index 6d1e4160c34..42ce03bac87 100644 --- a/src/traces/scatter/select.js +++ b/src/traces/scatter/select.js @@ -11,7 +11,7 @@ var subtypes = require('./subtypes'); -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd, xa = searchInfo.xaxis, ya = searchInfo.yaxis, @@ -25,7 +25,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var hasOnlyLines = (!subtypes.hasMarkers(trace) && !subtypes.hasText(trace)); if(hasOnlyLines) return []; - if(polygon === false) { // clear selection + if(selectionTester === false) { // clear selection for(i = 0; i < cd.length; i++) { cd[i].selected = 0; } @@ -36,7 +36,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(di.x); y = ya.c2p(di.y); - if(polygon.contains([x, y], false, i, searchInfo)) { + if(selectionTester.contains([x, y], false, i, searchInfo)) { selection.push({ pointNumber: i, x: xa.c2d(di.x), diff --git a/src/traces/scattergeo/select.js b/src/traces/scattergeo/select.js index 1d1c656913d..b6b9fe2b212 100644 --- a/src/traces/scattergeo/select.js +++ b/src/traces/scattergeo/select.js @@ -11,7 +11,7 @@ var subtypes = require('../scatter/subtypes'); var BADNUM = require('../../constants/numerical').BADNUM; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; @@ -23,7 +23,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var hasOnlyLines = (!subtypes.hasMarkers(trace) && !subtypes.hasText(trace)); if(hasOnlyLines) return []; - if(polygon === false) { + if(selectionTester === false) { for(i = 0; i < cd.length; i++) { cd[i].selected = 0; } @@ -38,7 +38,7 @@ module.exports = function selectPoints(searchInfo, polygon) { x = xa.c2p(lonlat); y = ya.c2p(lonlat); - if(polygon.contains([x, y], null, i, searchInfo)) { + if(selectionTester.contains([x, y], null, i, searchInfo)) { selection.push({ pointNumber: i, lon: lonlat[0], diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index fc0d862bfbe..fc43c95a57c 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -816,7 +816,7 @@ function calcHover(pointData, x, y, trace) { } -function selectPoints(searchInfo, polygon) { +function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var selection = []; var trace = cd[0].trace; @@ -838,10 +838,10 @@ function selectPoints(searchInfo, polygon) { var unels = null; // FIXME: clearing selection does not work here var i; - if(polygon !== false && !polygon.degenerate) { + if(selectionTester !== false && !selectionTester.degenerate) { els = [], unels = []; for(i = 0; i < stash.count; i++) { - if(polygon.contains([stash.xpx[i], stash.ypx[i]], false, i, searchInfo)) { + if(selectionTester.contains([stash.xpx[i], stash.ypx[i]], false, i, searchInfo)) { els.push(i); selection.push({ pointNumber: i, diff --git a/src/traces/scattermapbox/select.js b/src/traces/scattermapbox/select.js index 3b799c718b2..298c76419e3 100644 --- a/src/traces/scattermapbox/select.js +++ b/src/traces/scattermapbox/select.js @@ -12,7 +12,7 @@ var Lib = require('../../lib'); var subtypes = require('../scatter/subtypes'); var BADNUM = require('../../constants/numerical').BADNUM; -module.exports = function selectPoints(searchInfo, polygon) { +module.exports = function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var xa = searchInfo.xaxis; var ya = searchInfo.yaxis; @@ -22,7 +22,7 @@ module.exports = function selectPoints(searchInfo, polygon) { if(!subtypes.hasMarkers(trace)) return []; - if(polygon === false) { + if(selectionTester === false) { for(i = 0; i < cd.length; i++) { cd[i].selected = 0; } @@ -35,7 +35,7 @@ module.exports = function selectPoints(searchInfo, polygon) { var lonlat2 = [Lib.wrap180(lonlat[0]), lonlat[1]]; var xy = [xa.c2p(lonlat2), ya.c2p(lonlat2)]; - if(polygon.contains(xy, null, i, searchInfo)) { + if(selectionTester.contains(xy, null, i, searchInfo)) { selection.push({ pointNumber: i, lon: lonlat[0], diff --git a/src/traces/splom/index.js b/src/traces/splom/index.js index 43e44df2ed1..27c8bc532ba 100644 --- a/src/traces/splom/index.js +++ b/src/traces/splom/index.js @@ -342,7 +342,7 @@ function hoverPoints(pointData, xval, yval) { return [pointData]; } -function selectPoints(searchInfo, polygon) { +function selectPoints(searchInfo, selectionTester) { var cd = searchInfo.cd; var trace = cd[0].trace; var stash = cd[0].t; @@ -371,10 +371,10 @@ function selectPoints(searchInfo, polygon) { // filter out points by visible scatter ones var els = null; var unels = null; - if(polygon !== false && !polygon.degenerate) { + if(selectionTester !== false && !selectionTester.degenerate) { els = [], unels = []; for(i = 0; i < x.length; i++) { - if(polygon.contains([xpx[i], ypx[i]], null, i, searchInfo)) { + if(selectionTester.contains([xpx[i], ypx[i]], null, i, searchInfo)) { els.push(i); selection.push({ pointNumber: i, From 7eba6402e62a30d15d75c0a87c85f15733d07774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 14:59:37 +0200 Subject: [PATCH 30/54] Add comment for multiTester's contains function [1852] --- src/lib/select.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/lib/select.js b/src/lib/select.js index 14d6143fb83..40abcd720fa 100644 --- a/src/lib/select.js +++ b/src/lib/select.js @@ -80,16 +80,15 @@ select.multiTester = function multiTester(list) { } } - // TODO Consider making signature of contains more lean /** * Tests if the given point is within this tester. * - * @param pt an object having an `x` and a `y` property defining the location - * of the point - * @param arg parameter to pass additional arguments down to wrapped testers - * @param pointNumber the point number of the point - * @param searchInfo identifies the trace the point is contained in - * @return {boolean} + * @param {Array} pt - [0] is the x coordinate, [1] is the y coordinate of the point. + * @param {*} arg - An optional parameter to pass down to wrapped testers. + * @param {number} pointNumber - The point number of the point within the underlying data array. + * @param {number} searchInfo - An object identifying the trace the point is contained in. + * + * @return {boolean} true if point is considered to be selected, false otherwise. */ function contains(pt, arg, pointNumber, searchInfo) { var yes = false; From 92ee0facc0392499733d36d79c052cd3998202b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 15:57:05 +0200 Subject: [PATCH 31/54] Remove two obsolete comments from cartesian/select.js [1852] --- src/plots/cartesian/select.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 23beb9da25c..e54e980dfff 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -47,8 +47,6 @@ function prepSelect(e, startX, startY, dragOptions, mode) { var path0 = 'M' + x0 + ',' + y0; var pw = dragOptions.xaxes[0]._length; var ph = dragOptions.yaxes[0]._length; - // var xAxisIds = dragOptions.xaxes.map(getAxId); - // var yAxisIds = dragOptions.yaxes.map(getAxId); var allAxes = dragOptions.xaxes.concat(dragOptions.yaxes); var subtract = e.altKey; From 3767ce2bbf2fc1d76210edca427752d0057245ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 16:28:08 +0200 Subject: [PATCH 32/54] Fix test-syntax linting errors in select_test.js [1852] - @flaky annotation is no longer allowed in calls of Jasmine `describe`. --- test/jasmine/tests/select_test.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 6fa683a52e7..1fd59539b0f 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -120,7 +120,7 @@ var LASSOEVENTS = [4, 2, 1]; var SELECT_PATH = [[93, 193], [143, 193]]; var LASSO_PATH = [[316, 171], [318, 239], [335, 243], [328, 169]]; -describe('@flaky Click-to-select', function() { +describe('Click-to-select', function() { var mock14Pts = { '1': { x: 134, y: 116 }, '7': { x: 270, y: 160 }, @@ -243,7 +243,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - describe('clears entire selection when the last selected data point', function() { + describe('@flaky clears entire selection when the last selected data point', function() { [{ desc: 'is clicked', clickOpts: {} @@ -272,7 +272,7 @@ describe('@flaky Click-to-select', function() { }); }); - it('cleanly clears and starts selections although add/subtract mode on', function(done) { + it('@flaky cleanly clears and starts selections although add/subtract mode on', function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[7]); @@ -293,7 +293,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('supports adding to an existing selection', function(done) { + it('@flaky supports adding to an existing selection', function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[7]); }) .then(function() { @@ -305,7 +305,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('supports subtracting from an existing selection', function(done) { + it('@flaky supports subtracting from an existing selection', function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[7]); }) .then(function() { @@ -321,7 +321,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('can be used interchangeably with lasso/box select', function(done) { + it('@flaky can be used interchangeably with lasso/box select', function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[35]); @@ -374,7 +374,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('works in a multi-trace plot', function(done) { + it('@flaky works in a multi-trace plot', function(done) { Plotly.plot(gd, [ { x: [1, 3, 5, 4, 10, 12, 12, 7], @@ -417,7 +417,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('is supported in pan/zoom mode', function(done) { + it('@flaky is supported in pan/zoom mode', function(done) { plotMock14({ dragmode: 'zoom' }) .then(function() { return _immediateClickPt(mock14Pts[35]); @@ -446,7 +446,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('retains selected points when switching between pan and zoom mode', function(done) { + it('@flaky retains selected points when switching between pan and zoom mode', function(done) { plotMock14({ dragmode: 'zoom' }) .then(function() { return _immediateClickPt(mock14Pts[35]); @@ -474,7 +474,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('is supported by scattergl in pan/zoom mode', function(done) { + it('@flaky is supported by scattergl in pan/zoom mode', function(done) { Plotly.plot(gd, [ { x: [7, 8, 9, 10], @@ -500,7 +500,7 @@ describe('@flaky Click-to-select', function() { .then(done); }); - it('deals correctly with histogram\'s binning in the persistent selection case', function(done) { + it('@flaky deals correctly with histogram\'s binning in the persistent selection case', function(done) { var mock = require('@mocks/histogram_colorscale.json'); var firstBinPts = [0]; var secondBinPts = [1, 2]; @@ -540,7 +540,7 @@ describe('@flaky Click-to-select', function() { function shiftClickThirdBin() { return _click(351, 347, { shiftKey: true }); } }); - it('ignores clicks on boxes in a box trace type', function(done) { + it('@flaky ignores clicks on boxes in a box trace type', function(done) { var mock = Lib.extendDeep({}, require('@mocks/box_grouped_horz.json')); mock.layout.clickmode = 'event+select'; @@ -573,7 +573,7 @@ describe('@flaky Click-to-select', function() { function clickBox() { return _clickPt({ x: 565, y: 329 }); } }); - describe('is disabled when clickmode does not include \'select\'', function() { + describe('@flaky is disabled when clickmode does not include \'select\'', function() { // TODO How to test for pan and zoom mode as well? Note, that // in lasso and select mode, plotly_selected was emitted upon a single // click although select-on-click wasn't supported. This behavior is kept @@ -634,7 +634,7 @@ describe('@flaky Click-to-select', function() { testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]) ] .forEach(function(testCase) { - it('trace type ' + testCase.traceType, function(done) { + it('@flaky trace type ' + testCase.traceType, function(done) { var defaultLayoutOpts = { layout: { clickmode: 'event+select', From 8a4fc9988d3c081f0841095b27bad79442769f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 16:50:54 +0200 Subject: [PATCH 33/54] Fix further test-syntax linting errors in select_test.js [1852] --- test/jasmine/tests/select_test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 1fd59539b0f..544bc45b470 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -243,7 +243,7 @@ describe('Click-to-select', function() { .then(done); }); - describe('@flaky clears entire selection when the last selected data point', function() { + describe('clears entire selection when the last selected data point', function() { [{ desc: 'is clicked', clickOpts: {} @@ -251,7 +251,7 @@ describe('Click-to-select', function() { desc: 'is clicked while add/subtract modifier keys are active', clickOpts: { shiftKey: true } }].forEach(function(testData) { - it(testData.desc, function(done) { + it('@flaky ' + testData.desc, function(done) { plotMock14() .then(function() { return _immediateClickPt(mock14Pts[7]); }) .then(function() { @@ -573,7 +573,7 @@ describe('Click-to-select', function() { function clickBox() { return _clickPt({ x: 565, y: 329 }); } }); - describe('@flaky is disabled when clickmode does not include \'select\'', function() { + describe('is disabled when clickmode does not include \'select\'', function() { // TODO How to test for pan and zoom mode as well? Note, that // in lasso and select mode, plotly_selected was emitted upon a single // click although select-on-click wasn't supported. This behavior is kept @@ -583,7 +583,7 @@ describe('Click-to-select', function() { // ['pan', 'zoom', 'select', 'lasso'] ['select', 'lasso'] .forEach(function(dragmode) { - it('and dragmode is ' + dragmode, function(done) { + it('@flaky and dragmode is ' + dragmode, function(done) { plotMock14({ clickmode: 'event', dragmode: dragmode }) .then(function() { // Still, the plotly_selected event should be thrown, From db3e1267930e28afbfe9aa0d5f0b6defba57aa9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 27 Aug 2018 17:02:22 +0200 Subject: [PATCH 34/54] Fix for-loop bug in cartesian/select.js caught by ESLint [1852] --- src/plots/cartesian/select.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index e54e980dfff..4c016c2c821 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -553,7 +553,7 @@ function isOnlyThisBinSelected(searchTraces, clickedPtInfo) { if(isSameTrace) { trace = clickedPtInfo.searchInfo.cd[0].trace; if(trace.selectedpoints.length === clickedPtInfo.pointNumbers.length) { - for(i = 0; i > clickedPtInfo.pointNumbers.length; i++) { + for(i = 0; i < clickedPtInfo.pointNumbers.length; i++) { if(trace.selectedpoints.indexOf(clickedPtInfo.pointNumbers[i]) < 0) { return false; } From b3984f7599786273b033bd2723db8ae262a693e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 29 Aug 2018 13:59:36 +0200 Subject: [PATCH 35/54] Adapt description of of `clickmode` attribute [1852] --- src/components/fx/layout_attributes.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/components/fx/layout_attributes.js b/src/components/fx/layout_attributes.js index 246d3e5df72..153c723bafe 100644 --- a/src/components/fx/layout_attributes.js +++ b/src/components/fx/layout_attributes.js @@ -26,17 +26,18 @@ module.exports = { editType: 'plot', description: [ 'Determines the mode of single click interactions.', - '*event* is the default value and only emits *plotly_selected*', - 'events with no event data (kept for compatibility reasons) in dragmodes', - '*lasso* and *select*. The *select* flag enables selecting single', - 'data points by click. Its behavior is closely tied to *hovermode*. The', - 'data point that is being currently hovered on, will be the data point', - 'to be selected. So setting *hovermode* to *closest* may be the best fit', - 'for most applications. Click-to-select also supports persistent selections,', + '*event* is the default value and emits the `plotly_click`', + 'event. In addition this mode emits the `plotly_selected` event', + 'in drag modes *lasso* and *select*, but with no event data attached', + '(kept for compatibility reasons).', + 'The *select* flag enables selecting single', + 'data points via click. This mode also supports persistent selections,', 'meaning that pressing Shift while clicking, adds to / subtracts from an', - 'existing selection.', - 'When *clickmode* is being set to *select+event*, click-to-select is enabled', - 'and select events are sent with corresponding eventData attached.' + 'existing selection. *select* with `hovermode`: *x* can be confusing, consider', + 'explicitly setting `hovermode`: *closest* when using this feature.', + 'Selection events are sent accordingly as long as *event* flag is set as well.', + 'When the *event* flag is missing, `plotly_click` and `plotly_selected`', + 'events are not fired.' ].join(' ') }, dragmode: { From 2f7ad27e3f2d620a449c35569b36dfcd837784d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 11:30:25 +0200 Subject: [PATCH 36/54] Simplify check if clicked point is the sole selected point [1852] - Remove an unnecessary call to `isPointOrBinSelected` in case the clicked point is part of a binned trace (like histogram). --- src/plots/cartesian/select.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 4c016c2c821..d32569aa0b9 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -315,10 +315,10 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli // Note: potentially costly operation isPointOrBinSelected is // called as late as possible through the use of an assignment // in an if condition. - if((isBinnedTrace ? + if(isBinnedTrace ? isOnlyThisBinSelected(searchTraces, clickedPtInfo) : - isOnlyOnePointSelected(searchTraces)) && - (pointOrBinSelected = isPointOrBinSelected(clickedPtInfo))) + isOnlyOnePointSelected(searchTraces) && + (pointOrBinSelected = isPointOrBinSelected(clickedPtInfo))) { if(polygonOutlines) polygonOutlines.remove(); for(i = 0; i < searchTraces.length; i++) { From d7c14341a1ecc97f5da4d160ba195075c1454d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 12:09:08 +0200 Subject: [PATCH 37/54] Flag click-to-select tests of gl-based traces with @gl [1852] --- test/jasmine/tests/select_test.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 544bc45b470..8d4dfd25b95 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -374,7 +374,7 @@ describe('Click-to-select', function() { .then(done); }); - it('@flaky works in a multi-trace plot', function(done) { + it('@gl works in a multi-trace plot', function(done) { Plotly.plot(gd, [ { x: [1, 3, 5, 4, 10, 12, 12, 7], @@ -474,7 +474,7 @@ describe('Click-to-select', function() { .then(done); }); - it('@flaky is supported by scattergl in pan/zoom mode', function(done) { + it('@gl is supported by scattergl in pan/zoom mode', function(done) { Plotly.plot(gd, [ { x: [7, 8, 9, 10], @@ -630,11 +630,12 @@ describe('Click-to-select', function() { testCase('scatterpolar', require('@mocks/polar_scatter.json'), 130, 290, [[], [], [], [19], [], []], { dragmode: 'zoom' }), testCase('scatterpolargl', require('@mocks/glpolar_scatter.json'), 130, 290, - [[], [], [], [19], [], []], { dragmode: 'zoom' }), - testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]) + [[], [], [], [19], [], []], { dragmode: 'zoom' }).enableGl(), + testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]).enableGl() ] .forEach(function(testCase) { - it('@flaky trace type ' + testCase.traceType, function(done) { + var ciAnnotation = testCase.gl ? 'gl' : 'flaky'; + it('@' + ciAnnotation + ' trace type ' + testCase.traceType, function(done) { var defaultLayoutOpts = { layout: { clickmode: 'event+select', @@ -687,7 +688,12 @@ describe('Click-to-select', function() { x: x, y: y, expectedPts: expectedPts, - configOptions: configOptions + configOptions: configOptions, + gl: false, + enableGl: function() { + this.gl = true; + return this; + } }; } }); From e895efd6c0601f362f4aac65f19d5937bf1e2f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 12:28:29 +0200 Subject: [PATCH 38/54] Rename variable in `lib/select.js` [1852] --- src/lib/select.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/select.js b/src/lib/select.js index 40abcd720fa..b8ced28de38 100644 --- a/src/lib/select.js +++ b/src/lib/select.js @@ -91,15 +91,15 @@ select.multiTester = function multiTester(list) { * @return {boolean} true if point is considered to be selected, false otherwise. */ function contains(pt, arg, pointNumber, searchInfo) { - var yes = false; + var contained = false; for(var i = 0; i < testers.length; i++) { if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { // if contained by subtract tester - exclude the point - yes = testers[i].subtract === false; + contained = testers[i].subtract === false; } } - return yes; + return contained; } return { From 60317b17c12b8fc9b3eb25eb347a1bc70712aa17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 14:01:29 +0200 Subject: [PATCH 39/54] Adapt uninitialized var declarations in click-to-select code [1852] - From one per line to many per line. --- src/plots/cartesian/select.js | 31 +++++++------------------------ test/jasmine/tests/select_test.js | 6 ++---- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index d32569aa0b9..0c51bc02f44 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -294,16 +294,8 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli var clickmode = gd._fullLayout.clickmode; var sendEvents = clickmode.indexOf('event') > -1; var selection = []; - var searchTraces; - var searchInfo; - var currentSelectionDef; - var selectionTester; - var traceSelection; - var thisTracesSelection; - var pointOrBinSelected; - var subtract; - var eventData; - var i; + var searchTraces, searchInfo, currentSelectionDef, selectionTester, traceSelection; + var thisTracesSelection, pointOrBinSelected, subtract, eventData, i; if(isHoverDataSet(hoverData)) { coerceSelectionsCache(evt, gd, dragOptions); @@ -416,9 +408,7 @@ function determineSearchTraces(gd, xAxes, yAxes, subplot) { var searchTraces = []; var xAxisIds = xAxes.map(getAxId); var yAxisIds = yAxes.map(getAxId); - var cd; - var trace; - var i; + var cd, trace, i; for(i = 0; i < gd.calcdata.length; i++) { cd = gd.calcdata[i]; @@ -457,8 +447,7 @@ function determineSearchTraces(gd, xAxes, yAxes, subplot) { function drawSelection(polygons, outlines) { var paths = []; - var i; - var d; + var i, d; for(i = 0; i < polygons.length; i++) { var ppts = polygons[i]; @@ -481,8 +470,7 @@ function extractClickedPtInfo(hoverData, searchTraces) { var hoverDatum = hoverData[0]; var pointNumber = -1; var pointNumbers = []; - var searchInfo; - var i; + var searchInfo, i; for(i = 0; i < searchTraces.length; i++) { searchInfo = searchTraces[i]; @@ -536,10 +524,7 @@ function isPointOrBinSelected(clickedPtInfo) { function isOnlyThisBinSelected(searchTraces, clickedPtInfo) { var tracesWithSelectedPts = []; - var searchInfo; - var trace; - var isSameTrace; - var i; + var searchInfo, trace, isSameTrace, i; for(i = 0; i < searchTraces.length; i++) { searchInfo = searchTraces[i]; @@ -568,9 +553,7 @@ function isOnlyThisBinSelected(searchTraces, clickedPtInfo) { function isOnlyOnePointSelected(searchTraces) { var len = 0; - var searchInfo; - var trace; - var i; + var searchInfo, trace, i; for(i = 0; i < searchTraces.length; i++) { searchInfo = searchTraces[i]; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 8d4dfd25b95..7db2a398317 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -200,8 +200,7 @@ describe('Click-to-select', function() { */ function assertSelectedPoints(expected) { var expectedPtsPerTrace = toArrayOfArrays(expected); - var expectedPts; - var traceNum; + var expectedPts, traceNum; for(traceNum = 0; traceNum < expectedPtsPerTrace.length; traceNum++) { expectedPts = expectedPtsPerTrace[traceNum]; @@ -210,8 +209,7 @@ describe('Click-to-select', function() { } function toArrayOfArrays(expected) { - var isArrayInArray; - var i; + var isArrayInArray, i; if(Array.isArray(expected)) { isArrayInArray = false; From 61b1a3259e7026af5853f1eac3588758efb2e9cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 14:05:30 +0200 Subject: [PATCH 40/54] Add 'none' as an extra flag to clickmode [1852] --- src/components/fx/layout_attributes.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/fx/layout_attributes.js b/src/components/fx/layout_attributes.js index 153c723bafe..9fbc6254506 100644 --- a/src/components/fx/layout_attributes.js +++ b/src/components/fx/layout_attributes.js @@ -24,6 +24,7 @@ module.exports = { flags: ['event', 'select'], dflt: 'event', editType: 'plot', + extras: ['none'], description: [ 'Determines the mode of single click interactions.', '*event* is the default value and emits the `plotly_click`', From e460e4626778314fb2e6e81b62b753c486e669d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 14:19:31 +0200 Subject: [PATCH 41/54] Simplify code for coercing selection cache state [1852] --- src/plots/cartesian/select.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 0c51bc02f44..13d59b0a28e 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -372,20 +372,13 @@ function coerceSelectionsCache(evt, gd, dragOptions) { fullLayout._lastSelectedSubplot && fullLayout._lastSelectedSubplot === plotinfo.id ); - - if( - selectingOnSameSubplot && - (evt.shiftKey || evt.altKey) && - (plotinfo.selection && plotinfo.selection.selectionDefs) && - !dragOptions.selectionDefs - ) { + var hasModifierKey = evt.shiftKey || evt.altKey; + if(selectingOnSameSubplot && hasModifierKey && + (plotinfo.selection && plotinfo.selection.selectionDefs) && !dragOptions.selectionDefs) { // take over selection definitions from prev mode, if any dragOptions.selectionDefs = plotinfo.selection.selectionDefs; dragOptions.mergedPolygons = plotinfo.selection.mergedPolygons; - } else if( - (!evt.shiftKey && !evt.altKey) || - ((evt.shiftKey || evt.altKey) && !plotinfo.selection) - ) { + } else if(!hasModifierKey || !plotinfo.selection) { clearSelectionsCache(dragOptions); } From 2ef3fce4938dbe7f3227ca224d96302de0fb6ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 14:32:49 +0200 Subject: [PATCH 42/54] Resolve TODO in cartesian/select.js regarding binned traces [1852] - Replace TODO comment with a hint. --- src/plots/cartesian/select.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 13d59b0a28e..7cce7cce787 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -474,10 +474,10 @@ function extractClickedPtInfo(hoverData, searchTraces) { break; } - // TODO hoverDatum not having a pointNumber but a binNumber seems to be an oddity of histogram only - // Not deleting .pointNumber in histogram/event_data.js would simplify code here and in addition - // would not break the hover event structure - // documented at https://plot.ly/javascript/hover-events/ + // Hint: in some traces like histogram, one graphical element + // doesn't correspond to one particular data point, but to + // bins of data points. Thus, hoverDatum can have a binNumber + // property instead of pointNumber. if(hoverDatum.pointNumber !== undefined) { pointNumber = hoverDatum.pointNumber; } else if(hoverDatum.binNumber !== undefined) { From ac4b909748166cb553bdea888923d77ac28dbe7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 17:31:08 +0200 Subject: [PATCH 43/54] Remove lib/select.js and integrate code in cartesian/select.js [1852] - Reason: cartesian is the only module using it anyways. --- src/lib/select.js | 115 ---------------------------------- src/plots/cartesian/select.js | 106 +++++++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 119 deletions(-) delete mode 100644 src/lib/select.js diff --git a/src/lib/select.js b/src/lib/select.js deleted file mode 100644 index b8ced28de38..00000000000 --- a/src/lib/select.js +++ /dev/null @@ -1,115 +0,0 @@ -/** -* Copyright 2012-2018, Plotly, Inc. -* All rights reserved. -* -* This source code is licensed under the MIT license found in the -* LICENSE file in the root directory of this source tree. -*/ - - -'use strict'; - -var polygon = require('./polygon'); - -var select = module.exports = {}; - -/** - * Constructs a new point selection definition. - * - * @param pointNumber the point number of the point as in `data` - * @param searchInfo identifies the trace the point is contained in - * @param subtract true if the selection definition should mark a deselection - * @return {{pointNumber: Number, searchInfo: Object, subtract: boolean}} - */ -select.pointSelectionDef = function(pointNumber, searchInfo, subtract) { - return { - pointNumber: pointNumber, - searchInfo: searchInfo, - subtract: subtract - }; -}; - -function isPointSelectionDef(o) { - return 'pointNumber' in o && 'searchInfo' in o; -} - -function pointNumberTester(pointSelectionDef) { - return { - xmin: 0, - xmax: 0, - ymin: 0, - ymax: 0, - pts: [], - contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { - return searchInfo.cd[0].trace._expandedIndex === pointSelectionDef.searchInfo.cd[0].trace._expandedIndex && - pointNumber === pointSelectionDef.pointNumber; - }, - isRect: false, - degenerate: false, - subtract: pointSelectionDef.subtract - }; -} - -/** - * Wraps multiple selection testers. - * - * @param list an array of selection testers - * - * @return a selection tester object with a contains function - * that can be called to evaluate a point against all wrapped - * selection testers that were passed in list. - */ -select.multiTester = function multiTester(list) { - var testers = []; - var xmin = isPointSelectionDef(list[0]) ? 0 : list[0][0][0]; - var xmax = xmin; - var ymin = isPointSelectionDef(list[0]) ? 0 : list[0][0][1]; - var ymax = ymin; - - for(var i = 0; i < list.length; i++) { - if(isPointSelectionDef(list[i])) { - testers.push(pointNumberTester(list[i])); - } else { - var tester = polygon.tester(list[i]); - tester.subtract = list[i].subtract; - testers.push(tester); - xmin = Math.min(xmin, tester.xmin); - xmax = Math.max(xmax, tester.xmax); - ymin = Math.min(ymin, tester.ymin); - ymax = Math.max(ymax, tester.ymax); - } - } - - /** - * Tests if the given point is within this tester. - * - * @param {Array} pt - [0] is the x coordinate, [1] is the y coordinate of the point. - * @param {*} arg - An optional parameter to pass down to wrapped testers. - * @param {number} pointNumber - The point number of the point within the underlying data array. - * @param {number} searchInfo - An object identifying the trace the point is contained in. - * - * @return {boolean} true if point is considered to be selected, false otherwise. - */ - function contains(pt, arg, pointNumber, searchInfo) { - var contained = false; - for(var i = 0; i < testers.length; i++) { - if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { - // if contained by subtract tester - exclude the point - contained = testers[i].subtract === false; - } - } - - return contained; - } - - return { - xmin: xmin, - xmax: xmax, - ymin: ymin, - ymax: ymax, - pts: [], - contains: contains, - isRect: false, - degenerate: false - }; -}; diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 7cce7cce787..fdc869f8690 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -16,7 +16,6 @@ var Color = require('../../components/color'); var Fx = require('../../components/fx'); var polygon = require('../../lib/polygon'); -var select = require('../../lib/select'); var throttle = require('../../lib/throttle'); var makeEventData = require('../../components/fx/helpers').makeEventData; var getFromId = require('./axis_ids').getFromId; @@ -27,8 +26,6 @@ var MINSELECT = constants.MINSELECT; var filteredPolygon = polygon.filter; var polygonTester = polygon.tester; -var pointSelectionDef = select.pointSelectionDef; -var multiTester = select.multiTester; function getAxId(ax) { return ax._id; } @@ -330,7 +327,7 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli (pointOrBinSelected !== undefined ? pointOrBinSelected : isPointOrBinSelected(clickedPtInfo)); - currentSelectionDef = pointSelectionDef(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); + currentSelectionDef = newPointSelectionDef(clickedPtInfo.pointNumber, clickedPtInfo.searchInfo, subtract); var allSelectionDefs = dragOptions.selectionDefs.concat([currentSelectionDef]); selectionTester = multiTester(allSelectionDefs); @@ -363,6 +360,107 @@ function selectOnClick(evt, gd, xAxes, yAxes, subplot, dragOptions, polygonOutli } } +/** + * Constructs a new point selection definition object. + */ +function newPointSelectionDef(pointNumber, searchInfo, subtract) { + return { + pointNumber: pointNumber, + searchInfo: searchInfo, + subtract: subtract + }; +} + +function isPointSelectionDef(o) { + return 'pointNumber' in o && 'searchInfo' in o; +} + +/* + * Constructs a new point number tester. + */ +function newPointNumTester(pointSelectionDef) { + return { + xmin: 0, + xmax: 0, + ymin: 0, + ymax: 0, + pts: [], + contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { + var idxWantedTrace = pointSelectionDef.searchInfo.cd[0].trace._expandedIndex; + var idxActualTrace = searchInfo.cd[0].trace._expandedIndex; + return idxActualTrace === idxWantedTrace && + pointNumber === pointSelectionDef.pointNumber; + }, + isRect: false, + degenerate: false, + subtract: pointSelectionDef.subtract + }; +} + +/** + * Wraps multiple selection testers. + * + * @param {Array} list - An array of selection testers. + * + * @return a selection tester object with a contains function + * that can be called to evaluate a point against all wrapped + * selection testers that were passed in list. + */ +function multiTester(list) { + var testers = []; + var xmin = isPointSelectionDef(list[0]) ? 0 : list[0][0][0]; + var xmax = xmin; + var ymin = isPointSelectionDef(list[0]) ? 0 : list[0][0][1]; + var ymax = ymin; + + for(var i = 0; i < list.length; i++) { + if(isPointSelectionDef(list[i])) { + testers.push(newPointNumTester(list[i])); + } else { + var tester = polygon.tester(list[i]); + tester.subtract = list[i].subtract; + testers.push(tester); + xmin = Math.min(xmin, tester.xmin); + xmax = Math.max(xmax, tester.xmax); + ymin = Math.min(ymin, tester.ymin); + ymax = Math.max(ymax, tester.ymax); + } + } + + /** + * Tests if the given point is within this tester. + * + * @param {Array} pt - [0] is the x coordinate, [1] is the y coordinate of the point. + * @param {*} arg - An optional parameter to pass down to wrapped testers. + * @param {number} pointNumber - The point number of the point within the underlying data array. + * @param {number} searchInfo - An object identifying the trace the point is contained in. + * + * @return {boolean} true if point is considered to be selected, false otherwise. + */ + function contains(pt, arg, pointNumber, searchInfo) { + var contained = false; + for(var i = 0; i < testers.length; i++) { + if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { + // if contained by subtract tester - exclude the point + contained = testers[i].subtract === false; + } + } + + return contained; + } + + return { + xmin: xmin, + xmax: xmax, + ymin: ymin, + ymax: ymax, + pts: [], + contains: contains, + isRect: false, + degenerate: false + }; +} + function coerceSelectionsCache(evt, gd, dragOptions) { var fullLayout = gd._fullLayout; var zoomLayer = fullLayout._zoomlayer; From 05b50b94c9f56ac258517a8e2beefa607f0feec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 30 Aug 2018 22:57:24 +0200 Subject: [PATCH 44/54] Fix mapbox click-to-select bug caused by multiple click listeners [1852] --- src/plots/mapbox/mapbox.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index 80d3ab89ab1..ade50e2586a 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -420,6 +420,10 @@ proto.updateFx = function(fullLayout) { subplot: self.id }); + // Unregister the old handler before potentially registering + // a new one. Otherwise multiple click handlers might + // be registered resulting in unwanted behavior. + map.off('click', self.onClickInPanHandler); if(dragMode === 'select' || dragMode === 'lasso') { map.dragPan.disable(); map.on('zoomstart', self.clearSelect); @@ -430,8 +434,6 @@ proto.updateFx = function(fullLayout) { }; dragElement.init(self.dragOptions); - - map.off('click', self.onClickInPanHandler); } else { map.dragPan.enable(); map.off('zoomstart', self.clearSelect); From 88e9993668673029b7310aec0c591967b52f4413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Tue, 4 Sep 2018 15:27:24 +0200 Subject: [PATCH 45/54] Fix testing click-to-select is inactive if clickmode is 'event' [1852] - Wait until 'plotly_click' event is fired before checking that no point has been selected. --- test/jasmine/tests/select_test.js | 36 ++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 7db2a398317..a3850d4da3a 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -54,7 +54,7 @@ function assertSelectionNodes(cornerCnt, outlineCnt, _msg) { } var selectingCnt, selectingData, selectedCnt, selectedData, deselectCnt, doubleClickData; -var selectedPromise, deselectPromise; +var selectedPromise, deselectPromise, clickedPromise; function resetEvents(gd) { selectingCnt = 0; @@ -98,6 +98,12 @@ function resetEvents(gd) { resolve(); }); }); + + clickedPromise = new Promise(function(resolve) { + gd.on('plotly_click', function() { + resolve(); + }); + }); } function assertEventCounts(selecting, selected, deselect, msg) { @@ -557,10 +563,9 @@ describe('Click-to-select', function() { .then(function() { assertSelectionCleared(); clickBox(); + return clickedPromise; }) .then(function() { - // TODO Be sure this is called "late enough" after clicking on box has been processed - // Maybe plotly_click event would get fired after any selection events? assertSelectionCleared(); }) .catch(failTest) @@ -572,13 +577,6 @@ describe('Click-to-select', function() { }); describe('is disabled when clickmode does not include \'select\'', function() { - // TODO How to test for pan and zoom mode as well? Note, that - // in lasso and select mode, plotly_selected was emitted upon a single - // click although select-on-click wasn't supported. This behavior is kept - // for compatibility reasons and as a side affect allows to write this test - // for lasso and select. But in pan and zoom, how to be sure a click has been - // processed by plotly.js? - // ['pan', 'zoom', 'select', 'lasso'] ['select', 'lasso'] .forEach(function(dragmode) { it('@flaky and dragmode is ' + dragmode, function(done) { @@ -597,6 +595,24 @@ describe('Click-to-select', function() { }); }); + describe('is disabled when clickmode does not include \'select\'', function() { + ['pan', 'zoom'] + .forEach(function(dragmode) { + it('@flaky and dragmode is ' + dragmode, function(done) { + plotMock14({ clickmode: 'event', dragmode: dragmode }) + .then(function() { + _immediateClickPt(mock14Pts[1]); + return clickedPromise; + }) + .then(function() { + assertSelectionCleared(); + }) + .catch(failTest) + .then(done); + }); + }); + }); + describe('is supported by', function() { // On loading mocks: From f247d934cfae80a9e8de64aaaa19062d66a64707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Tue, 4 Sep 2018 15:32:48 +0200 Subject: [PATCH 46/54] Ensure 'plotly_click' is fired after 'plotly_selected' [1852] --- src/plots/cartesian/dragbox.js | 8 +-- src/plots/geo/geo.js | 10 +-- src/plots/mapbox/mapbox.js | 22 +++--- src/plots/polar/polar.js | 8 +-- src/plots/ternary/ternary.js | 8 +-- test/jasmine/tests/select_test.js | 110 +++++++++++++++++++----------- 6 files changed, 98 insertions(+), 68 deletions(-) diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index e9ca4d65fad..bdd180b17c4 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -231,13 +231,13 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { if(numClicks === 2 && !singleEnd) doubleClick(); if(isMainDrag) { - if(clickmode.indexOf('event') > -1) { - Fx.click(gd, evt, plotinfo.id); - } - if(clickmode.indexOf('select') > -1) { selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions); } + + if(clickmode.indexOf('event') > -1) { + Fx.click(gd, evt, plotinfo.id); + } } else if(numClicks === 1 && singleEnd) { var ax = ns ? ya0 : xa0, diff --git a/src/plots/geo/geo.js b/src/plots/geo/geo.js index 80a2f03c04a..83153e0dfe3 100644 --- a/src/plots/geo/geo.js +++ b/src/plots/geo/geo.js @@ -452,6 +452,11 @@ proto.updateFx = function(fullLayout, geoLayout) { bgRect.on('click', function() { // For select and lasso the dragElement is handling clicks if(dragMode !== 'select' && dragMode !== 'lasso') { + if(clickMode.indexOf('select') > -1) { + selectOnClick(d3.event, gd, [_this.xaxis], [_this.yaxis], + _this.id, dragOptions); + } + if(clickMode.indexOf('event') > -1) { // TODO: like pie and mapbox, this doesn't support right-click // actually this one is worse, as right-click starts a pan, or leaves @@ -459,11 +464,6 @@ proto.updateFx = function(fullLayout, geoLayout) { // Also, only tangentially related, we should cancel hover during pan Fx.click(gd, d3.event); } - - if(clickMode.indexOf('select') > -1) { - selectOnClick(d3.event, gd, [_this.xaxis], [_this.yaxis], - _this.id, dragOptions); - } } }); }; diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index ade50e2586a..75c91941702 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -177,15 +177,6 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { Fx.hover(gd, evt, self.id); }); - 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); - }); - function unhover() { Fx.loneUnhover(fullLayout._toppaper); } @@ -236,13 +227,18 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { return function(evt) { var clickMode = gd._fullLayout.clickmode; - if(clickMode.indexOf('event') > -1) { - Fx.click(gd, evt.originalEvent); - } - if(clickMode.indexOf('select') > -1) { selectOnClick(evt.originalEvent, gd, [self.xaxis], [self.yaxis], self.id, dragOptions); } + + if(clickMode.indexOf('event') > -1) { + // 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); + } }; }; }; diff --git a/src/plots/polar/polar.js b/src/plots/polar/polar.js index 8d083e7a270..184018ad310 100644 --- a/src/plots/polar/polar.js +++ b/src/plots/polar/polar.js @@ -849,13 +849,13 @@ proto.updateMainDrag = function(fullLayout, polarLayout) { Registry.call('relayout', gd, updateObj); } - if(clickMode.indexOf('event') > -1) { - Fx.click(gd, evt, _this.id); - } - if(clickMode.indexOf('select') > -1 && numClicks === 1) { selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOpts); } + + if(clickMode.indexOf('event') > -1) { + Fx.click(gd, evt, _this.id); + } } dragOpts.prepFn = function(evt, startX, startY) { diff --git a/src/plots/ternary/ternary.js b/src/plots/ternary/ternary.js index d10fc985094..8d0cfd9cc41 100644 --- a/src/plots/ternary/ternary.js +++ b/src/plots/ternary/ternary.js @@ -503,13 +503,13 @@ proto.initInteractions = function() { Registry.call('relayout', gd, attrs); } - if(clickMode.indexOf('event') > -1) { - Fx.click(gd, evt, _this.id); - } - if(clickMode.indexOf('select') > -1 && numClicks === 1) { selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOptions); } + + if(clickMode.indexOf('event') > -1) { + Fx.click(gd, evt, _this.id); + } } function zoomPrep(e, startX, startY) { diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index a3850d4da3a..a954df9ca55 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -649,28 +649,8 @@ describe('Click-to-select', function() { ] .forEach(function(testCase) { var ciAnnotation = testCase.gl ? 'gl' : 'flaky'; - it('@' + ciAnnotation + ' trace type ' + testCase.traceType, function(done) { - var defaultLayoutOpts = { - layout: { - clickmode: 'event+select', - dragmode: 'pan', - hovermode: 'closest' - } - }; - var customLayoutOptions = { - layout: testCase.layoutOptions - }; - var customConfigOptions = { - config: testCase.configOptions - }; - var mockCopy = Lib.extendDeep( - {}, - testCase.mock, - defaultLayoutOpts, - customLayoutOptions, - customConfigOptions); - - Plotly.plot(gd, mockCopy.data, mockCopy.layout, mockCopy.config) + it('@' + ciAnnotation + ' trace type ' + testCase.label, function(done) { + Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) .then(function() { return _immediateClickPt(testCase); }) @@ -693,24 +673,78 @@ describe('Click-to-select', function() { .then(done); }); }); + }); - function testCase(traceType, mock, x, y, expectedPts, layoutOptions, configOptions) { - return { - traceType: traceType, - mock: mock, - layoutOptions: layoutOptions, - x: x, - y: y, - expectedPts: expectedPts, - configOptions: configOptions, - gl: false, - enableGl: function() { - this.gl = true; - return this; - } - }; - } + describe('triggers \'plotly_selected\' before \'plotly_click\'', function() { + [ + testCase('cartesian', require('@mocks/14.json'), 270, 160, [7]), + testCase('geo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), + testCase('ternary', require('@mocks/ternary_markers.json'), 485, 335, [7]), + testCase('mapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, + { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), + testCase('polar', require('@mocks/polar_scatter.json'), 130, 290, + [[], [], [], [19], [], []], { dragmode: 'zoom' }) + ].forEach(function(testCase) { + it('@flaky for base plot ' + testCase.label, function(done) { + Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) + .then(function() { + var clickHandlerCalled = false; + var selectedHandlerCalled = false; + + gd.on('plotly_selected', function() { + expect(clickHandlerCalled).toBe(false); + selectedHandlerCalled = true; + }); + gd.on('plotly_click', function() { + clickHandlerCalled = true; + expect(selectedHandlerCalled).toBe(true); + done(); + }); + + return click(testCase.x, testCase.y); + }) + .catch(failTest) + .then(done); + }); + }); }); + + function testCase(label, mock, x, y, expectedPts, layoutOptions, configOptions) { + var defaultLayoutOpts = { + layout: { + clickmode: 'event+select', + dragmode: 'pan', + hovermode: 'closest' + } + }; + var customLayoutOptions = { + layout: layoutOptions + }; + var customConfigOptions = { + config: configOptions + }; + var mockCopy = Lib.extendDeep( + {}, + mock, + defaultLayoutOpts, + customLayoutOptions, + customConfigOptions); + + return { + label: label, + mock: mockCopy, + layoutOptions: layoutOptions, + x: x, + y: y, + expectedPts: expectedPts, + configOptions: configOptions, + gl: false, + enableGl: function() { + this.gl = true; + return this; + } + }; + } }); describe('Test select box and lasso in general:', function() { From 90ba209b76417cac03cf3bc8c28be4c0e52b52b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 5 Sep 2018 18:00:56 +0200 Subject: [PATCH 47/54] Replace dynamic CI tags with static ones in select_test.js [1852] - Reason: dynamically set CI tags in Jasmine `it` test descriptions are not parsed by CI. --- test/jasmine/tests/select_test.js | 71 +++++++++++++++++-------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index a954df9ca55..59730275821 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -615,11 +615,37 @@ describe('Click-to-select', function() { describe('is supported by', function() { + function _run(testCase, doneFn) { + Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) + .then(function() { + return _immediateClickPt(testCase); + }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + return Plotly.relayout(gd, 'dragmode', 'lasso'); + }) + .then(function() { + _clickPt(testCase); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + return _clickPt(testCase); + }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + }) + .catch(failTest) + .then(doneFn); + } + // On loading mocks: // - Note, that `require` function calls are resolved at compile time // and thus dynamically concatenated mock paths won't work. // - Some mocks don't specify a width and height, so this needs // to be set explicitly to ensure click coordinates fit. + + // The non-gl traces: use @flaky CI annotation [ testCase('histrogram', require('@mocks/histogram_colorscale.json'), 355, 301, [3, 4, 5]), testCase('box', require('@mocks/box_grouped_horz.json'), 610, 342, [[2], [], []], @@ -643,34 +669,22 @@ describe('Click-to-select', function() { // so set dragmode to zoom testCase('scatterpolar', require('@mocks/polar_scatter.json'), 130, 290, [[], [], [], [19], [], []], { dragmode: 'zoom' }), + ] + .forEach(function(testCase) { + it('@flaky trace type ' + testCase.label, function(done) { + _run(testCase, done); + }); + }); + + // The gl traces: use @gl CI annotation + [ testCase('scatterpolargl', require('@mocks/glpolar_scatter.json'), 130, 290, - [[], [], [], [19], [], []], { dragmode: 'zoom' }).enableGl(), - testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]).enableGl() + [[], [], [], [19], [], []], { dragmode: 'zoom' }), + testCase('splom', require('@mocks/splom_lower.json'), 427, 400, [[], [7], []]) ] .forEach(function(testCase) { - var ciAnnotation = testCase.gl ? 'gl' : 'flaky'; - it('@' + ciAnnotation + ' trace type ' + testCase.label, function(done) { - Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) - .then(function() { - return _immediateClickPt(testCase); - }) - .then(function() { - assertSelectedPoints(testCase.expectedPts); - return Plotly.relayout(gd, 'dragmode', 'lasso'); - }) - .then(function() { - _clickPt(testCase); - return deselectPromise; - }) - .then(function() { - assertSelectionCleared(); - return _clickPt(testCase); - }) - .then(function() { - assertSelectedPoints(testCase.expectedPts); - }) - .catch(failTest) - .then(done); + it('@gl trace type ' + testCase.label, function(done) { + _run(testCase, done); }); }); }); @@ -737,12 +751,7 @@ describe('Click-to-select', function() { x: x, y: y, expectedPts: expectedPts, - configOptions: configOptions, - gl: false, - enableGl: function() { - this.gl = true; - return this; - } + configOptions: configOptions }; } }); From 2bc582bfaadfdd1d8ebcd3a9ee9b0018cfb2ae61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 5 Sep 2018 18:09:22 +0200 Subject: [PATCH 48/54] Apply @noCI tag to mapbox tests in select_test.js [1852] --- src/plots/mapbox/mapbox.js | 1 - test/jasmine/tests/select_test.js | 67 ++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index 75c91941702..77da3871017 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -424,7 +424,6 @@ proto.updateFx = function(fullLayout) { map.dragPan.disable(); map.on('zoomstart', self.clearSelect); - self.dragOptions.prepFn = function(e, startX, startY) { prepSelect(e, startX, startY, self.dragOptions, dragMode); }; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 59730275821..0a384ecc6f3 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -655,8 +655,6 @@ describe('Click-to-select', function() { testCase('ohlc', require('@mocks/ohlc_first.json'), 669, 165, [9]), testCase('candlestick', require('@mocks/finance_style.json'), 331, 162, [[], [5]]), testCase('choropleth', require('@mocks/geo_choropleth-text.json'), 440, 163, [6]), - testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, - { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), testCase('scattergeo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), testCase('scatterternary', require('@mocks/ternary_markers.json'), 485, 335, [7]), @@ -687,6 +685,17 @@ describe('Click-to-select', function() { _run(testCase, done); }); }); + + // The mapbox traces: use @noCI annotation cause they are usually too resource-intensive + [ + testCase('scattermapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, + { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }) + ] + .forEach(function(testCase) { + it('@noCI trace type ' + testCase.label, function(done) { + _run(testCase, done); + }); + }); }); describe('triggers \'plotly_selected\' before \'plotly_click\'', function() { @@ -694,33 +703,45 @@ describe('Click-to-select', function() { testCase('cartesian', require('@mocks/14.json'), 270, 160, [7]), testCase('geo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]), testCase('ternary', require('@mocks/ternary_markers.json'), 485, 335, [7]), - testCase('mapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, - { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }), testCase('polar', require('@mocks/polar_scatter.json'), 130, 290, [[], [], [], [19], [], []], { dragmode: 'zoom' }) ].forEach(function(testCase) { it('@flaky for base plot ' + testCase.label, function(done) { - Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) - .then(function() { - var clickHandlerCalled = false; - var selectedHandlerCalled = false; - - gd.on('plotly_selected', function() { - expect(clickHandlerCalled).toBe(false); - selectedHandlerCalled = true; - }); - gd.on('plotly_click', function() { - clickHandlerCalled = true; - expect(selectedHandlerCalled).toBe(true); - done(); - }); - - return click(testCase.x, testCase.y); - }) - .catch(failTest) - .then(done); + _run(testCase, done); }); }); + + // The mapbox traces: use @noCI annotation cause they are usually too resource-intensive + [ + testCase('mapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {}, + { mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }) + ].forEach(function(testCase) { + it('@noCI for base plot ' + testCase.label, function(done) { + _run(testCase, done); + }); + }); + + function _run(testCase, doneFn) { + Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) + .then(function() { + var clickHandlerCalled = false; + var selectedHandlerCalled = false; + + gd.on('plotly_selected', function() { + expect(clickHandlerCalled).toBe(false); + selectedHandlerCalled = true; + }); + gd.on('plotly_click', function() { + clickHandlerCalled = true; + expect(selectedHandlerCalled).toBe(true); + doneFn(); + }); + + return click(testCase.x, testCase.y); + }) + .catch(failTest) + .then(doneFn); + } }); function testCase(label, mock, x, y, expectedPts, layoutOptions, configOptions) { From 96fcdd784d14331c48ff8e181d137b6a46e6cafc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Wed, 5 Sep 2018 18:10:05 +0200 Subject: [PATCH 49/54] Minor change: move helper method in select_test.s [1852] --- test/jasmine/tests/select_test.js | 49 +++++++++++++++---------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 0a384ecc6f3..b8bc3f7e1dc 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -614,31 +614,6 @@ describe('Click-to-select', function() { }); describe('is supported by', function() { - - function _run(testCase, doneFn) { - Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) - .then(function() { - return _immediateClickPt(testCase); - }) - .then(function() { - assertSelectedPoints(testCase.expectedPts); - return Plotly.relayout(gd, 'dragmode', 'lasso'); - }) - .then(function() { - _clickPt(testCase); - return deselectPromise; - }) - .then(function() { - assertSelectionCleared(); - return _clickPt(testCase); - }) - .then(function() { - assertSelectedPoints(testCase.expectedPts); - }) - .catch(failTest) - .then(doneFn); - } - // On loading mocks: // - Note, that `require` function calls are resolved at compile time // and thus dynamically concatenated mock paths won't work. @@ -696,6 +671,30 @@ describe('Click-to-select', function() { _run(testCase, done); }); }); + + function _run(testCase, doneFn) { + Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config) + .then(function() { + return _immediateClickPt(testCase); + }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + return Plotly.relayout(gd, 'dragmode', 'lasso'); + }) + .then(function() { + _clickPt(testCase); + return deselectPromise; + }) + .then(function() { + assertSelectionCleared(); + return _clickPt(testCase); + }) + .then(function() { + assertSelectedPoints(testCase.expectedPts); + }) + .catch(failTest) + .then(doneFn); + } }); describe('triggers \'plotly_selected\' before \'plotly_click\'', function() { From a5a90f9b97182a54b426eaf0736d27ee041375de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Thu, 6 Sep 2018 21:27:38 +0200 Subject: [PATCH 50/54] Add a comment to select_test.js test [1852] --- test/jasmine/tests/select_test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index b8bc3f7e1dc..4c50d400921 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -1307,6 +1307,11 @@ describe('Test select box and lasso in general:', function() { }) .then(function() { _assertSelectedPoints([49, 50, 51, 52, 53, 54, 55, 56, 57]); + + // Note: although Shift has no behavioral effect on clearing a selection + // with a double click, users might hold the Shift key by accident. + // This test ensures selection is cleared as expected although + // the Shift key is held and no selection state is retained in any way. return doubleClick(500, 200, { shiftKey: true }); }) .then(function() { From 87d0497f4ef767aac3203604198beafab42e1bb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 7 Sep 2018 21:22:16 +0200 Subject: [PATCH 51/54] Default hovermode to closest if clickmode is select [1852] --- src/components/fx/layout_attributes.js | 11 +++++- src/components/fx/layout_defaults.js | 12 ++++--- test/jasmine/tests/hover_label_test.js | 49 ++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/components/fx/layout_attributes.js b/src/components/fx/layout_attributes.js index 9fbc6254506..2429a5541f5 100644 --- a/src/components/fx/layout_attributes.js +++ b/src/components/fx/layout_attributes.js @@ -59,7 +59,16 @@ module.exports = { role: 'info', values: ['x', 'y', 'closest', false], editType: 'modebar', - description: 'Determines the mode of hover interactions.' + description: [ + 'Determines the mode of hover interactions.', + 'If `clickmode` includes the *select* flag,', + '`hovermode` defaults to *closest*.', + 'If `clickmode` lacks the *select* flag,', + 'it defaults to *x* or *y* (depending on the trace\'s', + '`orientation` value) for plots based on', + 'cartesian coordinates. For anything else the default', + 'value is *closest*.', + ].join(' ') }, hoverdistance: { valType: 'integer', diff --git a/src/components/fx/layout_defaults.js b/src/components/fx/layout_defaults.js index ed34ce47b8d..877557eb29e 100644 --- a/src/components/fx/layout_defaults.js +++ b/src/components/fx/layout_defaults.js @@ -23,10 +23,14 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { var hovermodeDflt; if(layoutOut._has('cartesian')) { - // flag for 'horizontal' plots: - // determines the state of the mode bar 'compare' hovermode button - layoutOut._isHoriz = isHoriz(fullData); - hovermodeDflt = layoutOut._isHoriz ? 'y' : 'x'; + if(layoutOut.clickmode.indexOf('select') > -1) { + hovermodeDflt = 'closest'; + } else { + // flag for 'horizontal' plots: + // determines the state of the mode bar 'compare' hovermode button + layoutOut._isHoriz = isHoriz(fullData); + hovermodeDflt = layoutOut._isHoriz ? 'y' : 'x'; + } } else hovermodeDflt = 'closest'; diff --git a/test/jasmine/tests/hover_label_test.js b/test/jasmine/tests/hover_label_test.js index 7dc0edf0feb..15a2f1fafcb 100644 --- a/test/jasmine/tests/hover_label_test.js +++ b/test/jasmine/tests/hover_label_test.js @@ -2369,3 +2369,52 @@ describe('hover distance', function() { }); }); }); + +describe('hovermode defaults to', function() { + var gd; + + beforeEach(function() { + gd = createGraphDiv(); + }); + + afterEach(destroyGraphDiv); + + it('\'closest\' for cartesian plots if clickmode includes \'select\'', function(done) { + Plotly.plot(gd, [{ x: [1, 2, 3], y: [4, 5, 6] }], { clickmode: 'event+select' }) + .then(function() { + expect(gd._fullLayout.hovermode).toBe('closest'); + }) + .catch(failTest) + .then(done); + }); + + it('\'x\' for horizontal cartesian plots if clickmode lacks \'select\'', function(done) { + Plotly.plot(gd, [{ x: [1, 2, 3], y: [4, 5, 6], type: 'bar', orientation: 'h' }], { clickmode: 'event' }) + .then(function() { + expect(gd._fullLayout.hovermode).toBe('y'); + }) + .catch(failTest) + .then(done); + }); + + it('\'y\' for vertical cartesian plots if clickmode lacks \'select\'', function(done) { + Plotly.plot(gd, [{ x: [1, 2, 3], y: [4, 5, 6], type: 'bar', orientation: 'v' }], { clickmode: 'event' }) + .then(function() { + expect(gd._fullLayout.hovermode).toBe('x'); + }) + .catch(failTest) + .then(done); + }); + + it('\'closest\' for a non-cartesian plot', function(done) { + var mock = require('@mocks/polar_scatter.json'); + expect(mock.layout.hovermode).toBeUndefined(); + + Plotly.plot(gd, mock.data, mock.layout) + .then(function() { + expect(gd._fullLayout.hovermode).toBe('closest'); + }) + .catch(failTest) + .then(done); + }); +}); From 30789dd9aca73e59db2d18f11b38053b4eeba8f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 7 Sep 2018 20:20:06 +0200 Subject: [PATCH 52/54] Remove :hocho: obsolete polygon.multitester function [1852] - Put throwing an error in place should it be called with this parameter shape still. See PR #2944 for the discussion. --- src/lib/polygon.js | 51 +++++----------------------------------------- 1 file changed, 5 insertions(+), 46 deletions(-) diff --git a/src/lib/polygon.js b/src/lib/polygon.js index a282d232209..49a2e748429 100644 --- a/src/lib/polygon.js +++ b/src/lib/polygon.js @@ -31,7 +31,11 @@ var polygon = module.exports = {}; * returns boolean: is pt inside the polygon (including on its edges) */ polygon.tester = function tester(ptsIn) { - if(Array.isArray(ptsIn[0][0])) return polygon.multitester(ptsIn); + + // Throw an error if any code tries to pass this no + // longer supported argument shape. The local multitester became + // obsolete when introducing click-to-select in PR #2944. + if(Array.isArray(ptsIn[0][0])) throw new Error('multitester called!'); var pts = ptsIn.slice(), xmin = pts[0][0], @@ -174,51 +178,6 @@ polygon.tester = function tester(ptsIn) { }; }; -// TODO Somewhat redundant to multiTester in 'lib/select.js' -/** - * Test multiple polygons - */ -polygon.multitester = function multitester(list) { - var testers = [], - xmin = list[0][0][0], - xmax = xmin, - ymin = list[0][0][1], - ymax = ymin; - - for(var i = 0; i < list.length; i++) { - var tester = polygon.tester(list[i]); - tester.subtract = list[i].subtract; - testers.push(tester); - xmin = Math.min(xmin, tester.xmin); - xmax = Math.max(xmax, tester.xmax); - ymin = Math.min(ymin, tester.ymin); - ymax = Math.max(ymax, tester.ymax); - } - - function contains(pt, arg) { - var yes = false; - for(var i = 0; i < testers.length; i++) { - if(testers[i].contains(pt, arg)) { - // if contained by subtract polygon - exclude the point - yes = testers[i].subtract === false; - } - } - - return yes; - } - - return { - xmin: xmin, - xmax: xmax, - ymin: ymin, - ymax: ymax, - pts: [], - contains: contains, - isRect: false, - degenerate: false - }; -}; - /** * Test if a segment of a points array is bent or straight * From 32aa219f7eb92ac2720a072c709dbc8f39f22682 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Fri, 7 Sep 2018 21:50:03 +0200 Subject: [PATCH 53/54] Simplify clickmode / hovermode coercion code [1852] --- src/components/fx/layout_defaults.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/fx/layout_defaults.js b/src/components/fx/layout_defaults.js index 877557eb29e..5f7372d6edf 100644 --- a/src/components/fx/layout_defaults.js +++ b/src/components/fx/layout_defaults.js @@ -16,14 +16,14 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt); } - coerce('clickmode'); + var clickmode = coerce('clickmode'); var dragMode = coerce('dragmode'); if(dragMode === 'select') coerce('selectdirection'); var hovermodeDflt; if(layoutOut._has('cartesian')) { - if(layoutOut.clickmode.indexOf('select') > -1) { + if(clickmode.indexOf('select') > -1) { hovermodeDflt = 'closest'; } else { // flag for 'horizontal' plots: From b6bd813f6cad9f4bfc40e5f8002d5d886d51f8b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=B6stl?= Date: Mon, 10 Sep 2018 15:02:25 +0200 Subject: [PATCH 54/54] Not throw error for no longer valid polygon.tester argument [1852] - Reason: it's pretty sure it'll no longer be called. - See https://github.com/plotly/plotly.js/pull/2944#discussion_r216072220 --- src/lib/polygon.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/lib/polygon.js b/src/lib/polygon.js index 49a2e748429..fb07a677af6 100644 --- a/src/lib/polygon.js +++ b/src/lib/polygon.js @@ -31,12 +31,6 @@ var polygon = module.exports = {}; * returns boolean: is pt inside the polygon (including on its edges) */ polygon.tester = function tester(ptsIn) { - - // Throw an error if any code tries to pass this no - // longer supported argument shape. The local multitester became - // obsolete when introducing click-to-select in PR #2944. - if(Array.isArray(ptsIn[0][0])) throw new Error('multitester called!'); - var pts = ptsIn.slice(), xmin = pts[0][0], xmax = xmin,