From a66aa2d8fb2669d56211c7285910174469f47b82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Fri, 1 Sep 2017 16:44:25 -0400 Subject: [PATCH 1/6] make sure ax._boundingBox is defined when showticklabels is false - so that hover knows where to draw spikes --- src/plots/cartesian/axes.js | 55 +++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index 25d5f02f597..ba072d7f0db 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -1828,6 +1828,7 @@ axes.doTicks = function(gd, axid, skipTitle) { if(!ax.showticklabels || !isNumeric(position)) { tickLabels.remove(); drawAxTitle(); + calcBoundingBox(); return; } @@ -1993,23 +1994,43 @@ axes.doTicks = function(gd, axid, skipTitle) { } function calcBoundingBox() { - var bBox = container.node().getBoundingClientRect(); - var gdBB = gd.getBoundingClientRect(); - - /* - * the way we're going to use this, the positioning that matters - * is relative to the origin of gd. This is important particularly - * if gd is scrollable, and may have been scrolled between the time - * we calculate this and the time we use it - */ - ax._boundingBox = { - width: bBox.width, - height: bBox.height, - left: bBox.left - gdBB.left, - right: bBox.right - gdBB.left, - top: bBox.top - gdBB.top, - bottom: bBox.bottom - gdBB.top - }; + if(ax.showticklabels) { + var gdBB = gd.getBoundingClientRect(); + var bBox = container.node().getBoundingClientRect(); + + /* + * the way we're going to use this, the positioning that matters + * is relative to the origin of gd. This is important particularly + * if gd is scrollable, and may have been scrolled between the time + * we calculate this and the time we use it + */ + + ax._boundingBox = { + width: bBox.width, + height: bBox.height, + left: bBox.left - gdBB.left, + right: bBox.right - gdBB.left, + top: bBox.top - gdBB.top, + bottom: bBox.bottom - gdBB.top + }; + } else { + var gs = fullLayout._size; + var pos; + + // set dummy bbox for ticklabel-less axes + + if(axLetter === 'x') { + pos = ax.anchor === 'free' ? + gs.t + gs.h * (1 - ax.position) : + gs.t + gs.h * (1 - ax._anchorAxis.domain[{bottom: 0, top: 1}[ax.side]]); + ax._boundingBox = {top: pos, bottom: pos}; + } else { + pos = ax.anchor === 'free' ? + gs.l + gs.w * ax.position : + gs.l + gs.w * ax._anchorAxis.domain[{left: 0, right: 1}[ax.side]]; + ax._boundingBox = {left: pos, right: pos}; + } + } /* * for spikelines: what's the full domain of positions in the From b36392e6d658618961301cafbed55833e1214097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Tue, 5 Sep 2017 11:19:55 -0400 Subject: [PATCH 2/6] fill in dummy boundingBox object --- src/plots/cartesian/axes.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index ba072d7f0db..9f87837b245 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -2023,12 +2023,28 @@ axes.doTicks = function(gd, axid, skipTitle) { pos = ax.anchor === 'free' ? gs.t + gs.h * (1 - ax.position) : gs.t + gs.h * (1 - ax._anchorAxis.domain[{bottom: 0, top: 1}[ax.side]]); - ax._boundingBox = {top: pos, bottom: pos}; + + ax._boundingBox = { + top: pos, + bottom: pos, + left: ax._offset, + rigth: ax._offset + ax._length, + width: ax._length, + height: 0 + }; } else { pos = ax.anchor === 'free' ? gs.l + gs.w * ax.position : gs.l + gs.w * ax._anchorAxis.domain[{left: 0, right: 1}[ax.side]]; - ax._boundingBox = {left: pos, right: pos}; + + ax._boundingBox = { + left: pos, + right: pos, + bottom: ax._offset + ax._length, + top: ax._offset, + height: ax._length, + width: 0 + }; } } From 874b1ba4d2a53ea4580a851e36ee4e542a80ad4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Tue, 5 Sep 2017 11:22:22 -0400 Subject: [PATCH 3/6] split cases of early returns in drawLabels - when label position is undefined, don't (re)-calc its bounding box as that could override previous 'correct' calculations (e.g. when overlaying axes are present) - add dummy bounding box value whenever showticklabels is false --- src/plots/cartesian/axes.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index 9f87837b245..dfab610f57c 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -1825,7 +1825,13 @@ axes.doTicks = function(gd, axid, skipTitle) { // tick labels - for now just the main labels. // TODO: mirror labels, esp for subplots var tickLabels = container.selectAll('g.' + tcls).data(vals, datafn); - if(!ax.showticklabels || !isNumeric(position)) { + + if(!isNumeric(position)) { + tickLabels.remove(); + drawAxTitle(); + return; + } + if(!ax.showticklabels) { tickLabels.remove(); drawAxTitle(); calcBoundingBox(); From d306612e07d01cc9e1d315e9236caf5c6e9584e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Tue, 5 Sep 2017 11:22:51 -0400 Subject: [PATCH 4/6] add test for spike on `showticklabels: false` axes --- test/jasmine/tests/hover_spikeline_test.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/test/jasmine/tests/hover_spikeline_test.js b/test/jasmine/tests/hover_spikeline_test.js index 0b1a1cc1ea8..fe950026aac 100644 --- a/test/jasmine/tests/hover_spikeline_test.js +++ b/test/jasmine/tests/hover_spikeline_test.js @@ -4,6 +4,7 @@ var Plotly = require('@lib/index'); var Fx = require('@src/components/fx'); var Lib = require('@src/lib'); +var fail = require('../assets/fail_test'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); @@ -16,6 +17,7 @@ describe('spikeline', function() { describe('hover', function() { var mockCopy = Lib.extendDeep({}, mock); + var gd; mockCopy.layout.xaxis.showspikes = true; mockCopy.layout.xaxis.spikemode = 'toaxis'; @@ -24,8 +26,10 @@ describe('spikeline', function() { mockCopy.layout.xaxis2.showspikes = true; mockCopy.layout.xaxis2.spikemode = 'toaxis'; mockCopy.layout.hovermode = 'closest'; + beforeEach(function(done) { - Plotly.plot(createGraphDiv(), mockCopy.data, mockCopy.layout).then(done); + gd = createGraphDiv(); + Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); }); it('draws lines and markers on enabled axes', function() { @@ -34,6 +38,20 @@ describe('spikeline', function() { expect(d3.selectAll('circle.spikeline').size()).toEqual(1); }); + it('draws lines and markers on enabled axes w/o tick labels', function(done) { + Plotly.relayout(gd, { + 'xaxis.showticklabels': false, + 'yaxis.showticklabels': false + }) + .then(function() { + Fx.hover('graph', {xval: 2, yval: 3}, 'xy'); + expect(d3.selectAll('line.spikeline').size()).toEqual(4); + expect(d3.selectAll('circle.spikeline').size()).toEqual(1); + }) + .catch(fail) + .then(done); + }); + it('doesn\'t draw lines and markers on disabled axes', function() { Fx.hover('graph', {xval: 30, yval: 40}, 'x2y2'); expect(d3.selectAll('line.spikeline').size()).toEqual(2); From 92b606b76adf1a0806e2b051d603fbe501e2bfba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Tue, 5 Sep 2017 15:25:46 -0400 Subject: [PATCH 5/6] revamp hover spike tests - assert number of spike line visible as well as their position attributes --- test/jasmine/tests/hover_spikeline_test.js | 107 +++++++++++++++------ 1 file changed, 79 insertions(+), 28 deletions(-) diff --git a/test/jasmine/tests/hover_spikeline_test.js b/test/jasmine/tests/hover_spikeline_test.js index fe950026aac..8ca09b4ab41 100644 --- a/test/jasmine/tests/hover_spikeline_test.js +++ b/test/jasmine/tests/hover_spikeline_test.js @@ -7,55 +7,106 @@ var Lib = require('@src/lib'); var fail = require('../assets/fail_test'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); +var customMatchers = require('../assets/custom_matchers'); describe('spikeline', function() { 'use strict'; - var mock = require('@mocks/19.json'); + beforeAll(function() { + jasmine.addMatchers(customMatchers); + }); afterEach(destroyGraphDiv); describe('hover', function() { - var mockCopy = Lib.extendDeep({}, mock); var gd; - mockCopy.layout.xaxis.showspikes = true; - mockCopy.layout.xaxis.spikemode = 'toaxis'; - mockCopy.layout.yaxis.showspikes = true; - mockCopy.layout.yaxis.spikemode = 'toaxis+marker'; - mockCopy.layout.xaxis2.showspikes = true; - mockCopy.layout.xaxis2.spikemode = 'toaxis'; - mockCopy.layout.hovermode = 'closest'; + function makeMock() { + var _mock = Lib.extendDeep({}, require('@mocks/19.json')); + _mock.layout.xaxis.showspikes = true; + _mock.layout.xaxis.spikemode = 'toaxis'; + _mock.layout.yaxis.showspikes = true; + _mock.layout.yaxis.spikemode = 'toaxis+marker'; + _mock.layout.xaxis2.showspikes = true; + _mock.layout.xaxis2.spikemode = 'toaxis'; + _mock.layout.hovermode = 'closest'; + return _mock; + } + + function _hover(evt, subplot) { + Fx.hover(gd, evt, subplot); + delete gd._lastHoverTime; + } + + function _assert(lineExpect, circleExpect) { + var lines = d3.selectAll('line.spikeline'); + var circles = d3.selectAll('circle.spikeline'); + + expect(lines.size()).toBe(lineExpect.length, '# of line nodes'); + expect(circles.size()).toBe(circleExpect.length, '# of circle nodes'); + + lines.each(function(_, i) { + var sel = d3.select(this); + ['x1', 'y1', 'x2', 'y2'].forEach(function(d, j) { + expect(sel.attr(d)) + .toBeWithin(lineExpect[i][j], 1, 'line ' + i + ' attr ' + d); + }); + }); - beforeEach(function(done) { + circles.each(function(_, i) { + var sel = d3.select(this); + ['cx', 'cy'].forEach(function(d, j) { + expect(sel.attr(d)) + .toBeWithin(circleExpect[i][j], 1, 'circle ' + i + ' attr ' + d); + }); + }); + } + + it('draws lines and markers on enabled axes', function(done) { gd = createGraphDiv(); - Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); - }); + var _mock = makeMock(); - it('draws lines and markers on enabled axes', function() { - Fx.hover('graph', {xval: 2, yval: 3}, 'xy'); - expect(d3.selectAll('line.spikeline').size()).toEqual(4); - expect(d3.selectAll('circle.spikeline').size()).toEqual(1); + Plotly.plot(gd, _mock).then(function() { + _hover({xval: 2, yval: 3}, 'xy'); + _assert( + [[80, 250, 557, 250], [80, 250, 557, 250], [557, 401, 557, 250], [557, 401, 557, 250]], + [[83, 250]] + ); + }) + .then(function() { + _hover({xval: 30, yval: 40}, 'x2y2'); + _assert( + [[820, 220, 820, 167], [820, 220, 820, 167]], + [] + ); + }) + .catch(fail) + .then(done); }); it('draws lines and markers on enabled axes w/o tick labels', function(done) { - Plotly.relayout(gd, { - 'xaxis.showticklabels': false, - 'yaxis.showticklabels': false + gd = createGraphDiv(); + var _mock = makeMock(); + + _mock.layout.xaxis.showticklabels = false; + _mock.layout.yaxis.showticklabels = false; + + Plotly.plot(gd, _mock).then(function() { + _hover({xval: 2, yval: 3}, 'xy'); + _assert( + [[80, 250, 557, 250], [80, 250, 557, 250], [557, 401, 557, 250], [557, 401, 557, 250]], + [[83, 250]] + ); }) .then(function() { - Fx.hover('graph', {xval: 2, yval: 3}, 'xy'); - expect(d3.selectAll('line.spikeline').size()).toEqual(4); - expect(d3.selectAll('circle.spikeline').size()).toEqual(1); + _hover({xval: 30, yval: 40}, 'x2y2'); + _assert( + [[820, 220, 820, 167], [820, 220, 820, 167]], + [] + ); }) .catch(fail) .then(done); }); - - it('doesn\'t draw lines and markers on disabled axes', function() { - Fx.hover('graph', {xval: 30, yval: 40}, 'x2y2'); - expect(d3.selectAll('line.spikeline').size()).toEqual(2); - expect(d3.selectAll('circle.spikeline').size()).toEqual(0); - }); }); }); From e45142ddda1f41fe1ac2e5a581eaec96a92a826f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20T=C3=A9treault-Pinard?= Date: Tue, 5 Sep 2017 15:43:20 -0400 Subject: [PATCH 6/6] add tolerance --- test/jasmine/tests/hover_spikeline_test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/hover_spikeline_test.js b/test/jasmine/tests/hover_spikeline_test.js index 8ca09b4ab41..dad000be93f 100644 --- a/test/jasmine/tests/hover_spikeline_test.js +++ b/test/jasmine/tests/hover_spikeline_test.js @@ -39,6 +39,7 @@ describe('spikeline', function() { } function _assert(lineExpect, circleExpect) { + var TOL = 5; var lines = d3.selectAll('line.spikeline'); var circles = d3.selectAll('circle.spikeline'); @@ -49,7 +50,7 @@ describe('spikeline', function() { var sel = d3.select(this); ['x1', 'y1', 'x2', 'y2'].forEach(function(d, j) { expect(sel.attr(d)) - .toBeWithin(lineExpect[i][j], 1, 'line ' + i + ' attr ' + d); + .toBeWithin(lineExpect[i][j], TOL, 'line ' + i + ' attr ' + d); }); }); @@ -57,7 +58,7 @@ describe('spikeline', function() { var sel = d3.select(this); ['cx', 'cy'].forEach(function(d, j) { expect(sel.attr(d)) - .toBeWithin(circleExpect[i][j], 1, 'circle ' + i + ' attr ' + d); + .toBeWithin(circleExpect[i][j], TOL, 'circle ' + i + ' attr ' + d); }); }); }