From 3b1ee6b85ba630f5929def60ccf21ac8b55e1685 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 24 Jun 2016 14:40:12 +0200 Subject: [PATCH 1/5] fix ternary hover after zoom/pan events registered after zooming. This fixes that, and tests the fix. --- src/plots/ternary/ternary.js | 6 ++++-- test/jasmine/tests/hover_label_test.js | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/plots/ternary/ternary.js b/src/plots/ternary/ternary.js index 96d7587f275..1fd6d7094f7 100644 --- a/src/plots/ternary/ternary.js +++ b/src/plots/ternary/ternary.js @@ -659,9 +659,9 @@ proto.initInteractions = function() { Plotly.relayout(gd, attrs); } - dragElement.init(dragOptions); - // finally, set up hover and click + // these event handlers must already be set before dragElement.init + // so it can stash them and override them. dragger.onmousemove = function(evt) { fx.hover(gd, evt, _this.id); gd._fullLayout._lasthover = dragger; @@ -677,6 +677,8 @@ proto.initInteractions = function() { dragger.onclick = function(evt) { fx.click(gd, evt); }; + + dragElement.init(dragOptions); }; function removeZoombox(gd) { diff --git a/test/jasmine/tests/hover_label_test.js b/test/jasmine/tests/hover_label_test.js index 2b3951fb8cb..4406c9744bc 100644 --- a/test/jasmine/tests/hover_label_test.js +++ b/test/jasmine/tests/hover_label_test.js @@ -691,6 +691,26 @@ describe('hover on fill', function() { }).then(done); }); + // click and doubleClick copied over from click_test + // TODO require from elsewhere + function click(x, y) { + mouseEvent('mousemove', x, y); + mouseEvent('mousedown', x, y); + mouseEvent('mouseup', x, y); + } + + function doubleClick(x, y) { + return new Promise(function(resolve) { + click(x, y); + + setTimeout(function() { + click(x, y); + setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); + }, DBLCLICKDELAY / 2); + }); + } + var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; + it('should work for scatterternary too', function(done) { var mock = Lib.extendDeep({}, require('@mocks/ternary_fill.json')); @@ -706,6 +726,12 @@ describe('hover on fill', function() { return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1'); }).then(function() { return assertLabelsCorrect([237, 251], [247.7, 254], 'trace 0'); + }).then(function() { + // trigger an autoscale redraw, which goes through dragElement + return doubleClick(237, 251); + }).then(function() { + // then make sure we can still select a *different* item afterward + return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1'); }).then(done); }); }); From ab3a9a22e560c9fae2eb3b329b8406cf9b5b63ca Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 24 Jun 2016 15:38:24 +0200 Subject: [PATCH 2/5] Tests: pull click and doubleClick out into assets select_test had a slight variant with the callback built into doubleClick - changed it into the other form where you just do .then(callback) --- test/jasmine/assets/click.js | 7 +++++++ test/jasmine/assets/double_click.js | 13 +++++++++++++ test/jasmine/tests/click_test.js | 26 ++++++------------------- test/jasmine/tests/hover_label_test.js | 21 +------------------- test/jasmine/tests/select_test.js | 27 ++++++-------------------- test/jasmine/tests/ternary_test.js | 20 ++----------------- 6 files changed, 35 insertions(+), 79 deletions(-) create mode 100644 test/jasmine/assets/click.js create mode 100644 test/jasmine/assets/double_click.js diff --git a/test/jasmine/assets/click.js b/test/jasmine/assets/click.js new file mode 100644 index 00000000000..d718b200486 --- /dev/null +++ b/test/jasmine/assets/click.js @@ -0,0 +1,7 @@ +var mouseEvent = require('./mouse_event'); + +module.exports = function click(x, y) { + mouseEvent('mousemove', x, y); + mouseEvent('mousedown', x, y); + mouseEvent('mouseup', x, y); +}; \ No newline at end of file diff --git a/test/jasmine/assets/double_click.js b/test/jasmine/assets/double_click.js new file mode 100644 index 00000000000..f1248de42ea --- /dev/null +++ b/test/jasmine/assets/double_click.js @@ -0,0 +1,13 @@ +var click = require('./click'); +var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; + +module.exports = function doubleClick(x, y) { + return new Promise(function(resolve) { + click(x, y); + + setTimeout(function() { + click(x, y); + setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); + }, DBLCLICKDELAY / 2); + }); +}; \ No newline at end of file diff --git a/test/jasmine/tests/click_test.js b/test/jasmine/tests/click_test.js index c08b56ea711..8dd517cdd15 100644 --- a/test/jasmine/tests/click_test.js +++ b/test/jasmine/tests/click_test.js @@ -8,6 +8,12 @@ var mouseEvent = require('../assets/mouse_event'); var getRectCenter = require('../assets/get_rect_center'); var customMatchers = require('../assets/custom_matchers'); +// cartesian click events events use the hover data +// from the mousemove events and then simulate +// a click event on mouseup +var click = require('../assets/click'); +var doubleClick = require('../assets/double_click'); + describe('Test click interactions:', function() { var mock = require('@mocks/14.json'); @@ -31,26 +37,6 @@ describe('Test click interactions:', function() { afterEach(destroyGraphDiv); - // cartesian click events events use the hover data - // from the mousemove events and then simulate - // a click event on mouseup - function click(x, y) { - mouseEvent('mousemove', x, y); - mouseEvent('mousedown', x, y); - mouseEvent('mouseup', x, y); - } - - function doubleClick(x, y) { - return new Promise(function(resolve) { - click(x, y); - - setTimeout(function() { - click(x, y); - setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); - }, DBLCLICKDELAY / 2); - }); - } - function drag(fromX, fromY, toX, toY, delay) { return new Promise(function(resolve) { mouseEvent('mousemove', fromX, fromY); diff --git a/test/jasmine/tests/hover_label_test.js b/test/jasmine/tests/hover_label_test.js index 4406c9744bc..78985c497d2 100644 --- a/test/jasmine/tests/hover_label_test.js +++ b/test/jasmine/tests/hover_label_test.js @@ -8,6 +8,7 @@ var Lib = require('@src/lib'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); var mouseEvent = require('../assets/mouse_event'); +var doubleClick = require('../assets/double_click'); describe('hover info', function() { 'use strict'; @@ -691,26 +692,6 @@ describe('hover on fill', function() { }).then(done); }); - // click and doubleClick copied over from click_test - // TODO require from elsewhere - function click(x, y) { - mouseEvent('mousemove', x, y); - mouseEvent('mousedown', x, y); - mouseEvent('mouseup', x, y); - } - - function doubleClick(x, y) { - return new Promise(function(resolve) { - click(x, y); - - setTimeout(function() { - click(x, y); - setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); - }, DBLCLICKDELAY / 2); - }); - } - var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; - it('should work for scatterternary too', function(done) { var mock = Lib.extendDeep({}, require('@mocks/ternary_fill.json')); diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index a4fd9bb4997..d26fa43f4c1 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -3,6 +3,8 @@ var d3 = require('d3'); var Plotly = require('@lib/index'); var Lib = require('@src/lib'); var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; +var click = require('../assets/click'); +var doubleClick = require('../assets/double_click'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); @@ -35,23 +37,6 @@ describe('select box and lasso', function() { mouseEvent('mouseup', path[len - 1][0], path[len - 1][1]); } - // cartesian click events events use the hover data - // from the mousemove events and then simulate - // a click event on mouseup - function click(x, y) { - mouseEvent('mousemove', x, y); - mouseEvent('mousedown', x, y); - mouseEvent('mouseup', x, y); - } - - function doubleClick(x, y, cb) { - click(x, y); - setTimeout(function() { - click(x, y); - cb(); - }, DBLCLICKDELAY / 2); - } - function assertRange(actual, expected) { var PRECISION = 4; @@ -104,7 +89,7 @@ describe('select box and lasso', function() { drag([[x0, y0], [x1, y1]]); - doubleClick(x2, y2, done); + doubleClick(x2, y2).then(done); }); }); @@ -153,7 +138,7 @@ describe('select box and lasso', function() { drag([[x0, y0], [x1, y1]]); - doubleClick(x2, y2, done); + doubleClick(x2, y2).then(done); }); }); @@ -225,7 +210,7 @@ describe('select box and lasso', function() { y: [0.10209191961595454, 24.512223978291406] }, 'with the correct selected range'); - doubleClick(250, 200, function() { + doubleClick(250, 200).then(function() { expect(doubleClickData).toBe(null, 'with the correct deselect data'); done(); }); @@ -283,7 +268,7 @@ describe('select box and lasso', function() { y: 2.75 }], 'with the correct selected points'); - doubleClick(250, 200, function() { + doubleClick(250, 200).then(function() { expect(doubleClickData).toBe(null, 'with the correct deselect data'); done(); }); diff --git a/test/jasmine/tests/ternary_test.js b/test/jasmine/tests/ternary_test.js index c00df866914..c563a500b1b 100644 --- a/test/jasmine/tests/ternary_test.js +++ b/test/jasmine/tests/ternary_test.js @@ -1,6 +1,5 @@ var Plotly = require('@lib'); var Lib = require('@src/lib'); -var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; var supplyLayoutDefaults = require('@src/plots/ternary/layout/defaults'); @@ -8,6 +7,8 @@ var d3 = require('d3'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); var mouseEvent = require('../assets/mouse_event'); +var click = require('../assets/click'); +var doubleClick = require('../assets/double_click'); var customMatchers = require('../assets/custom_matchers'); @@ -238,23 +239,6 @@ describe('ternary plots', function() { return d3.select('.hoverlayer').selectAll('g'); } - function click(x, y) { - mouseEvent('mousemove', x, y); - mouseEvent('mousedown', x, y); - mouseEvent('mouseup', x, y); - } - - function doubleClick(x, y) { - return new Promise(function(resolve) { - click(x, y); - - setTimeout(function() { - click(x, y); - resolve(); - }, DBLCLICKDELAY / 2); - }); - } - function drag(path) { var len = path.length; From cda7f6cf979068159361b9b7f4e5b22b15381672 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 24 Jun 2016 15:59:44 +0200 Subject: [PATCH 3/5] appease eslint Dunno why eslint cannot find the config file correctly from the test directory when I'm using sublime... --- test/jasmine/assets/click.js | 2 +- test/jasmine/assets/double_click.js | 2 +- test/jasmine/tests/select_test.js | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/test/jasmine/assets/click.js b/test/jasmine/assets/click.js index d718b200486..e2cc43e444b 100644 --- a/test/jasmine/assets/click.js +++ b/test/jasmine/assets/click.js @@ -4,4 +4,4 @@ module.exports = function click(x, y) { mouseEvent('mousemove', x, y); mouseEvent('mousedown', x, y); mouseEvent('mouseup', x, y); -}; \ No newline at end of file +}; diff --git a/test/jasmine/assets/double_click.js b/test/jasmine/assets/double_click.js index f1248de42ea..e92375588a8 100644 --- a/test/jasmine/assets/double_click.js +++ b/test/jasmine/assets/double_click.js @@ -10,4 +10,4 @@ module.exports = function doubleClick(x, y) { setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2); }, DBLCLICKDELAY / 2); }); -}; \ No newline at end of file +}; diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index d26fa43f4c1..885d993afb3 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -2,8 +2,6 @@ var d3 = require('d3'); var Plotly = require('@lib/index'); var Lib = require('@src/lib'); -var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; -var click = require('../assets/click'); var doubleClick = require('../assets/double_click'); var createGraphDiv = require('../assets/create_graph_div'); From c45c0746c5e52abd54457a8e40d36f14f78095a4 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Sat, 25 Jun 2016 01:04:37 +0200 Subject: [PATCH 4/5] hover on fills: constrain label to cartesian and ternary viewports --- src/traces/scatter/hover.js | 9 +++++++++ src/traces/scatterternary/hover.js | 25 +++++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/traces/scatter/hover.js b/src/traces/scatter/hover.js index 373d68d9e3c..cb23c268fa1 100644 --- a/src/traces/scatter/hover.js +++ b/src/traces/scatter/hover.js @@ -107,6 +107,11 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { } if(inside) { + // constrain ymin/max to the visible plot, so the label goes + // at the middle of the piece you can see + ymin = Math.max(ymin, 0); + ymax = Math.min(ymax, ya._length); + // find the overall left-most and right-most points of the // polygon(s) we're inside at their combined vertical midpoint. // This is where we will draw the hover label. @@ -128,6 +133,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { } } + // constrain xmin/max to the visible plot now too + xmin = Math.max(xmin, 0); + xmax = Math.min(xmax, xa._length); + // get only fill or line color for the hover color var color = Color.defaultLine; if(Color.opacity(trace.fillcolor)) color = trace.fillcolor; diff --git a/src/traces/scatterternary/hover.js b/src/traces/scatterternary/hover.js index 67de2f3b9b0..176786ed050 100644 --- a/src/traces/scatterternary/hover.js +++ b/src/traces/scatterternary/hover.js @@ -17,12 +17,29 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { var scatterPointData = scatterHover(pointData, xval, yval, hovermode); if(!scatterPointData || scatterPointData[0].index === false) return; + var newPointData = scatterPointData[0]; + // if hovering on a fill, we don't show any point data so the label is - // unchanged from what scatter gives us. - if(scatterPointData[0].index === undefined) return scatterPointData; + // unchanged from what scatter gives us - except that it needs to + // be constrained to the trianglular plot area, not just the rectangular + // area defined by the synthetic x and y axes + // TODO: in some cases the vertical middle of the shape is not within + // the triangular viewport at all, so the label can become disconnected + // from the shape entirely. But calculating what portion of the shape + // is actually visible, as constrained by the diagonal axis lines, is not + // so easy and anyway we lost the information we would have needed to do + // this inside scatterHover. + if(newPointData.index === undefined) { + var yFracUp = 1 - (newPointData.y0 / pointData.ya._length), + xLen = pointData.xa._length, + xMin = xLen * yFracUp / 2, + xMax = xLen - xMin; + newPointData.x0 = Math.max(Math.min(newPointData.x0, xMax), xMin); + newPointData.x1 = Math.max(Math.min(newPointData.x1, xMax), xMin); + return scatterPointData; + } - var newPointData = scatterPointData[0], - cdi = newPointData.cd[newPointData.index]; + var cdi = newPointData.cd[newPointData.index]; newPointData.a = cdi.a; newPointData.b = cdi.b; From a0a12d8d55c964cbad1a59d396e89a4278676ccc Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Mon, 27 Jun 2016 21:24:38 +0200 Subject: [PATCH 5/5] test clipped positioning of hover labels for scatterternary fill --- test/jasmine/tests/hover_label_test.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/jasmine/tests/hover_label_test.js b/test/jasmine/tests/hover_label_test.js index 78985c497d2..8f4c71e0eae 100644 --- a/test/jasmine/tests/hover_label_test.js +++ b/test/jasmine/tests/hover_label_test.js @@ -694,8 +694,9 @@ describe('hover on fill', function() { it('should work for scatterternary too', function(done) { var mock = Lib.extendDeep({}, require('@mocks/ternary_fill.json')); + var gd = createGraphDiv(); - Plotly.plot(createGraphDiv(), mock.data, mock.layout).then(function() { + Plotly.plot(gd, mock.data, mock.layout).then(function() { // hover over a point when that's closest, even if you're over // a fill, because by default we have hoveron='points+fills' return assertLabelsCorrect([237, 150], [240.0, 144], @@ -707,6 +708,16 @@ describe('hover on fill', function() { return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1'); }).then(function() { return assertLabelsCorrect([237, 251], [247.7, 254], 'trace 0'); + }).then(function() { + // zoom in to test clipping of large out-of-viewport shapes + return Plotly.relayout(gd, { + 'ternary.aaxis.min': 0.5, + 'ternary.baxis.min': 0.25 + }); + }).then(function() { + // this particular one has a hover label disconnected from the shape itself + // so if we ever fix this, the test will have to be fixed too. + return assertLabelsCorrect([295, 218], [275.1, 166], 'trace 2'); }).then(function() { // trigger an autoscale redraw, which goes through dragElement return doubleClick(237, 251);