From f75f5ba325e7c80d93272b71585d0e31ceac4f48 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 13:07:31 -0500 Subject: [PATCH 1/6] fix #2387 - prevent legend scrolling from scrollBox drag or right click --- src/components/dragelement/index.js | 2 +- src/components/legend/constants.js | 2 +- src/components/legend/draw.js | 15 ++-- test/jasmine/tests/legend_scroll_test.js | 88 ++++++++++++++++++++---- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/src/components/dragelement/index.js b/src/components/dragelement/index.js index 90785a2eb0d..ecf2869af30 100644 --- a/src/components/dragelement/index.js +++ b/src/components/dragelement/index.js @@ -135,7 +135,7 @@ dragElement.init = function init(options) { startY = offset[1]; initialTarget = e.target; initialEvent = e; - rightClick = (e.buttons && e.buttons === 2) || e.ctrlKey; + rightClick = e.buttons === 2 || e.ctrlKey; newMouseDownTime = (new Date()).getTime(); if(newMouseDownTime - gd._mouseDownTime < DBLCLICKDELAY) { diff --git a/src/components/legend/constants.js b/src/components/legend/constants.js index d968d7fef23..719aa50351b 100644 --- a/src/components/legend/constants.js +++ b/src/components/legend/constants.js @@ -9,7 +9,7 @@ 'use strict'; module.exports = { - scrollBarWidth: 4, + scrollBarWidth: 6, scrollBarHeight: 20, scrollBarColor: '#808BA4', scrollBarMargin: 4 diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index 3ba1a94194b..f472c8c51ba 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -94,10 +94,10 @@ module.exports = function draw(gd) { scrollBar.enter().append('rect') .attr({ 'class': 'scrollbar', - 'rx': 20, - 'ry': 2, - 'width': 0, - 'height': 0 + rx: 20, + ry: 3, + width: 0, + height: 0 }) .call(Color.fill, '#808BA4'); @@ -283,8 +283,12 @@ module.exports = function draw(gd) { scrollBox.on('.drag', null); var drag = d3.behavior.drag().on('drag', function() { + var e3 = d3.event; + var e = e3.sourceEvent; + if(e.buttons === 2 || e.ctrlKey) return; + scrollBarY = Lib.constrain( - d3.event.y - constants.scrollBarHeight / 2, + e3.y - constants.scrollBarHeight / 2, constants.scrollBarMargin, constants.scrollBarMargin + scrollBarYMax); scrollBoxY = - (scrollBarY - constants.scrollBarMargin) / @@ -293,7 +297,6 @@ module.exports = function draw(gd) { }); scrollBar.call(drag); - scrollBox.call(drag); } diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js index 89d7ed77f9a..f15075a34e8 100644 --- a/test/jasmine/tests/legend_scroll_test.js +++ b/test/jasmine/tests/legend_scroll_test.js @@ -7,6 +7,7 @@ var d3 = require('d3'); var createGraph = require('../assets/create_graph_div'); var destroyGraph = require('../assets/destroy_graph_div'); var getBBox = require('../assets/get_bbox'); +var mouseEvent = require('../assets/mouse_event'); var mock = require('../../image/mocks/legend_scroll.json'); describe('The legend', function() { @@ -76,18 +77,18 @@ describe('The legend', function() { }); it('should scroll when there\'s a wheel event', function() { - var legend = getLegend(), - scrollBox = getScrollBox(), - legendHeight = getLegendHeight(gd), - scrollBoxYMax = gd._fullLayout.legend._height - legendHeight, - scrollBarYMax = legendHeight - - constants.scrollBarHeight - - 2 * constants.scrollBarMargin, - initialDataScroll = scrollBox.getAttribute('data-scroll'), - wheelDeltaY = 100, - finalDataScroll = '' + Lib.constrain(initialDataScroll - - wheelDeltaY / scrollBarYMax * scrollBoxYMax, - -scrollBoxYMax, 0); + var legend = getLegend(); + var scrollBox = getScrollBox(); + var legendHeight = getLegendHeight(gd); + var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight; + var scrollBarYMax = legendHeight - + constants.scrollBarHeight - + 2 * constants.scrollBarMargin; + var initialDataScroll = scrollBox.getAttribute('data-scroll'); + var wheelDeltaY = 100; + var finalDataScroll = '' + Lib.constrain(initialDataScroll - + wheelDeltaY / scrollBarYMax * scrollBoxYMax, + -scrollBoxYMax, 0); legend.dispatchEvent(scrollTo(wheelDeltaY)); @@ -96,6 +97,69 @@ describe('The legend', function() { 'translate(0, ' + finalDataScroll + ')'); }); + function dragScroll(element, rightClick) { + var scrollBox = getScrollBox(); + var scrollBar = getScrollBar(); + var legendHeight = getLegendHeight(gd); + var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight; + var scrollBarYMax = legendHeight - + constants.scrollBarHeight - + 2 * constants.scrollBarMargin; + var initialDataScroll = scrollBox.getAttribute('data-scroll'); + var dy = 50; + var finalDataScroll = '' + Lib.constrain(initialDataScroll - + dy / scrollBarYMax * scrollBoxYMax, + -scrollBoxYMax, 0); + + var scrollBarBB = scrollBar.getBoundingClientRect(); + var y0 = scrollBarBB.top + scrollBarBB.height / 2; + var y1 = y0 + dy; + + var elBB = element.getBoundingClientRect(); + var x = elBB.left + elBB.width / 2; + + var opts = {element: element}; + if(rightClick) { + opts.button = 2; + opts.buttons = 2; + } + + mouseEvent('mousedown', x, y0, opts); + mouseEvent('mousemove', x, y1, opts); + mouseEvent('mouseup', x, y1, opts); + + expect(finalDataScroll).not.toBe(initialDataScroll); + + return finalDataScroll; + } + + it('should scroll on dragging the scrollbar', function() { + var finalDataScroll = dragScroll(getScrollBar()); + var scrollBox = getScrollBox(); + + expect(scrollBox.getAttribute('data-scroll')).toBe(finalDataScroll); + expect(scrollBox.getAttribute('transform')).toBe( + 'translate(0, ' + finalDataScroll + ')'); + }); + + it('should not scroll on dragging the scrollbox', function() { + var scrollBox = getScrollBox(); + var finalDataScroll = dragScroll(scrollBox); + + expect(scrollBox.getAttribute('data-scroll')).not.toBe(finalDataScroll); + expect(scrollBox.getAttribute('transform')).not.toBe( + 'translate(0, ' + finalDataScroll + ')'); + }); + + it('should not scroll on dragging the scrollbar with a right click', function() { + var finalDataScroll = dragScroll(getScrollBar(), true); + var scrollBox = getScrollBox(); + + expect(scrollBox.getAttribute('data-scroll')).not.toBe(finalDataScroll); + expect(scrollBox.getAttribute('transform')).not.toBe( + 'translate(0, ' + finalDataScroll + ')'); + }); + it('should keep the scrollbar position after a toggle event', function(done) { var legend = getLegend(), scrollBox = getScrollBox(), From ec42784f1b05fd66dee5a2f7ad11aeeaf6e0a0f3 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 15:18:18 -0500 Subject: [PATCH 2/6] grab the legend scrollbar at the cursor, not at its middle --- src/components/legend/draw.js | 14 ++++++++++---- test/jasmine/tests/legend_scroll_test.js | 9 +++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index f472c8c51ba..fb41ee481b0 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -282,13 +282,19 @@ module.exports = function draw(gd) { scrollBar.on('.drag', null); scrollBox.on('.drag', null); - var drag = d3.behavior.drag().on('drag', function() { - var e3 = d3.event; - var e = e3.sourceEvent; + var eventY0, scrollBarY0; + + var drag = d3.behavior.drag() + .on('dragstart', function() { + eventY0 = d3.event.sourceEvent.clientY; + scrollBarY0 = scrollBarY; + }) + .on('drag', function() { + var e = d3.event.sourceEvent; if(e.buttons === 2 || e.ctrlKey) return; scrollBarY = Lib.constrain( - e3.y - constants.scrollBarHeight / 2, + e.clientY - eventY0 + scrollBarY0, constants.scrollBarMargin, constants.scrollBarMargin + scrollBarYMax); scrollBoxY = - (scrollBarY - constants.scrollBarMargin) / diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js index f15075a34e8..3b9604d75d1 100644 --- a/test/jasmine/tests/legend_scroll_test.js +++ b/test/jasmine/tests/legend_scroll_test.js @@ -79,10 +79,11 @@ describe('The legend', function() { it('should scroll when there\'s a wheel event', function() { var legend = getLegend(); var scrollBox = getScrollBox(); + var scrollBar = getScrollBar(); var legendHeight = getLegendHeight(gd); var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight; var scrollBarYMax = legendHeight - - constants.scrollBarHeight - + scrollBar.getBoundingClientRect().height - 2 * constants.scrollBarMargin; var initialDataScroll = scrollBox.getAttribute('data-scroll'); var wheelDeltaY = 100; @@ -100,10 +101,11 @@ describe('The legend', function() { function dragScroll(element, rightClick) { var scrollBox = getScrollBox(); var scrollBar = getScrollBar(); + var scrollBarBB = scrollBar.getBoundingClientRect(); var legendHeight = getLegendHeight(gd); var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight; var scrollBarYMax = legendHeight - - constants.scrollBarHeight - + scrollBarBB.height - 2 * constants.scrollBarMargin; var initialDataScroll = scrollBox.getAttribute('data-scroll'); var dy = 50; @@ -111,8 +113,7 @@ describe('The legend', function() { dy / scrollBarYMax * scrollBoxYMax, -scrollBoxYMax, 0); - var scrollBarBB = scrollBar.getBoundingClientRect(); - var y0 = scrollBarBB.top + scrollBarBB.height / 2; + var y0 = scrollBarBB.top + scrollBarBB.height / 5; var y1 = y0 + dy; var elBB = element.getBoundingClientRect(); From 246b77fb563c21176df56aad31bf366c734530a3 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 15:56:56 -0500 Subject: [PATCH 3/6] make legend scrollbar scale to the visible fraction fixes the legend part of #1859 --- src/components/legend/constants.js | 2 +- src/components/legend/draw.js | 28 +++++++++++----------- test/jasmine/tests/legend_scroll_test.js | 30 +++++++++++++++--------- 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/components/legend/constants.js b/src/components/legend/constants.js index 719aa50351b..6ac1fa2119c 100644 --- a/src/components/legend/constants.js +++ b/src/components/legend/constants.js @@ -10,7 +10,7 @@ module.exports = { scrollBarWidth: 6, - scrollBarHeight: 20, + scrollBarMinHeight: 20, scrollBarColor: '#808BA4', scrollBarMargin: 4 }; diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index fb41ee481b0..03ae008903c 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -207,13 +207,6 @@ module.exports = function draw(gd) { // legend, background and border, scroll box and scroll bar Drawing.setTranslate(legend, lx, ly); - var scrollBarYMax = legendHeight - - constants.scrollBarHeight - - 2 * constants.scrollBarMargin, - scrollBoxYMax = opts._height - legendHeight, - scrollBarY, - scrollBoxY; - if(opts._height <= legendHeight || gd._context.staticPlot) { // if scrollbar should not be shown. bg.attr({ @@ -235,8 +228,15 @@ module.exports = function draw(gd) { scrollBox.call(Drawing.setClipUrl, clipId); } else { - scrollBarY = constants.scrollBarMargin, - scrollBoxY = scrollBox.attr('data-scroll') || 0; + var scrollBarHeight = Math.max(constants.scrollBarMinHeight, + legendHeight * legendHeight / opts._height); + var scrollBarYMax = legendHeight - + scrollBarHeight - + 2 * constants.scrollBarMargin; + var scrollBoxYMax = opts._height - legendHeight; + + var scrollBarY = constants.scrollBarMargin; + var scrollBoxY = scrollBox.attr('data-scroll') || 0; // increase the background and clip-path width // by the scrollbar width and margin @@ -262,7 +262,7 @@ module.exports = function draw(gd) { scrollBox.call(Drawing.setClipUrl, clipId); - if(firstRender) scrollHandler(scrollBarY, scrollBoxY); + if(firstRender) scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); legend.on('wheel', null); // to be safe, remove previous listeners legend.on('wheel', function() { @@ -272,7 +272,7 @@ module.exports = function draw(gd) { -scrollBoxYMax, 0); scrollBarY = constants.scrollBarMargin - scrollBoxY / scrollBoxYMax * scrollBarYMax; - scrollHandler(scrollBarY, scrollBoxY); + scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); if(scrollBoxY !== 0 && scrollBoxY !== -scrollBoxYMax) { d3.event.preventDefault(); } @@ -299,14 +299,14 @@ module.exports = function draw(gd) { constants.scrollBarMargin + scrollBarYMax); scrollBoxY = - (scrollBarY - constants.scrollBarMargin) / scrollBarYMax * scrollBoxYMax; - scrollHandler(scrollBarY, scrollBoxY); + scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); }); scrollBar.call(drag); } - function scrollHandler(scrollBarY, scrollBoxY) { + function scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight) { scrollBox .attr('data-scroll', scrollBoxY) .call(Drawing.setTranslate, 0, scrollBoxY); @@ -316,7 +316,7 @@ module.exports = function draw(gd) { legendWidth, scrollBarY, constants.scrollBarWidth, - constants.scrollBarHeight + scrollBarHeight ); clipPath.select('rect').attr({ y: opts.borderwidth - scrollBoxY diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js index 3b9604d75d1..b3e64ad1849 100644 --- a/test/jasmine/tests/legend_scroll_test.js +++ b/test/jasmine/tests/legend_scroll_test.js @@ -138,27 +138,30 @@ describe('The legend', function() { var finalDataScroll = dragScroll(getScrollBar()); var scrollBox = getScrollBox(); - expect(scrollBox.getAttribute('data-scroll')).toBe(finalDataScroll); + var dataScroll = scrollBox.getAttribute('data-scroll'); + expect(dataScroll).toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + finalDataScroll + ')'); + 'translate(0, ' + dataScroll + ')'); }); it('should not scroll on dragging the scrollbox', function() { var scrollBox = getScrollBox(); var finalDataScroll = dragScroll(scrollBox); - expect(scrollBox.getAttribute('data-scroll')).not.toBe(finalDataScroll); - expect(scrollBox.getAttribute('transform')).not.toBe( - 'translate(0, ' + finalDataScroll + ')'); + var dataScroll = scrollBox.getAttribute('data-scroll'); + expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); + expect(scrollBox.getAttribute('transform')).toBe( + 'translate(0, ' + dataScroll + ')'); }); it('should not scroll on dragging the scrollbar with a right click', function() { var finalDataScroll = dragScroll(getScrollBar(), true); var scrollBox = getScrollBox(); - expect(scrollBox.getAttribute('data-scroll')).not.toBe(finalDataScroll); - expect(scrollBox.getAttribute('transform')).not.toBe( - 'translate(0, ' + finalDataScroll + ')'); + var dataScroll = scrollBox.getAttribute('data-scroll'); + expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); + expect(scrollBox.getAttribute('transform')).toBe( + 'translate(0, ' + dataScroll + ')'); }); it('should keep the scrollbar position after a toggle event', function(done) { @@ -237,13 +240,18 @@ describe('The legend', function() { scrollBar = getScrollBar(), legendHeight = getLegendHeight(gd); - // The scrollbar is 20px tall and has 4px margins + // The scrollbar is >20px tall and has 4px margins + var scrollBarHeight = scrollBar.getBoundingClientRect().height; + // in this mock there are 22 traces, and 13 are visible in the legend + // at any given time + expect(scrollBarHeight).toBeCloseTo(legendHeight * 13 / 22, -1); legend.dispatchEvent(scrollTo(-1000)); - expect(+scrollBar.getAttribute('y')).toBe(4); + expect(+scrollBar.getAttribute('y')).toBeCloseTo(4, 3); legend.dispatchEvent(scrollTo(10000)); - expect(+scrollBar.getAttribute('y')).toBe(legendHeight - 4 - 20); + expect(+scrollBar.getAttribute('y')) + .toBeCloseTo(legendHeight - 4 - scrollBarHeight, 3); }); it('should be removed from DOM when \'showlegend\' is relayout\'ed to false', function(done) { From 569b37893c7082e9ed43740dd87a2d2fc36ce41b Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 21:50:09 -0500 Subject: [PATCH 4/6] fix #808 - update scrollBar when changing other legend attributes --- src/components/legend/draw.js | 46 ++++++----- test/jasmine/tests/legend_scroll_test.js | 98 ++++++++++++++++++++---- 2 files changed, 107 insertions(+), 37 deletions(-) diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index 03ae008903c..ed579cf6d1a 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -207,6 +207,10 @@ module.exports = function draw(gd) { // legend, background and border, scroll box and scroll bar Drawing.setTranslate(legend, lx, ly); + // to be safe, remove previous listeners + scrollBar.on('.drag', null); + legend.on('wheel', null); + if(opts._height <= legendHeight || gd._context.staticPlot) { // if scrollbar should not be shown. bg.attr({ @@ -226,6 +230,9 @@ module.exports = function draw(gd) { }); scrollBox.call(Drawing.setClipUrl, clipId); + + Drawing.setRect(scrollBar, 0, 0, 0, 0); + delete opts._scrollY; } else { var scrollBarHeight = Math.max(constants.scrollBarMinHeight, @@ -234,9 +241,10 @@ module.exports = function draw(gd) { scrollBarHeight - 2 * constants.scrollBarMargin; var scrollBoxYMax = opts._height - legendHeight; + var scrollRatio = scrollBarYMax / scrollBoxYMax; - var scrollBarY = constants.scrollBarMargin; - var scrollBoxY = scrollBox.attr('data-scroll') || 0; + // scrollBoxY is 0 or a negative number + var scrollBoxY = Math.max(opts._scrollY || 0, -scrollBoxYMax); // increase the background and clip-path width // by the scrollbar width and margin @@ -262,59 +270,49 @@ module.exports = function draw(gd) { scrollBox.call(Drawing.setClipUrl, clipId); - if(firstRender) scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); + scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); - legend.on('wheel', null); // to be safe, remove previous listeners legend.on('wheel', function() { scrollBoxY = Lib.constrain( - scrollBox.attr('data-scroll') - + opts._scrollY - d3.event.deltaY / scrollBarYMax * scrollBoxYMax, -scrollBoxYMax, 0); - scrollBarY = constants.scrollBarMargin - - scrollBoxY / scrollBoxYMax * scrollBarYMax; - scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); + scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); if(scrollBoxY !== 0 && scrollBoxY !== -scrollBoxYMax) { d3.event.preventDefault(); } }); - // to be safe, remove previous listeners - scrollBar.on('.drag', null); - scrollBox.on('.drag', null); - - var eventY0, scrollBarY0; + var eventY0, scrollBoxY0; var drag = d3.behavior.drag() .on('dragstart', function() { eventY0 = d3.event.sourceEvent.clientY; - scrollBarY0 = scrollBarY; + scrollBoxY0 = scrollBoxY; }) .on('drag', function() { var e = d3.event.sourceEvent; if(e.buttons === 2 || e.ctrlKey) return; - scrollBarY = Lib.constrain( - e.clientY - eventY0 + scrollBarY0, - constants.scrollBarMargin, - constants.scrollBarMargin + scrollBarYMax); - scrollBoxY = - (scrollBarY - constants.scrollBarMargin) / - scrollBarYMax * scrollBoxYMax; - scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight); + scrollBoxY = Lib.constrain( + (eventY0 - e.clientY) / scrollRatio + scrollBoxY0, + -scrollBoxYMax, 0); + scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); }); scrollBar.call(drag); } - function scrollHandler(scrollBarY, scrollBoxY, scrollBarHeight) { + function scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio) { + opts._scrollY = gd._fullLayout.legend._scrollY = scrollBoxY; scrollBox - .attr('data-scroll', scrollBoxY) .call(Drawing.setTranslate, 0, scrollBoxY); scrollBar.call( Drawing.setRect, legendWidth, - scrollBarY, + constants.scrollBarMargin - scrollBoxY * scrollRatio, constants.scrollBarWidth, scrollBarHeight ); diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js index b3e64ad1849..93dbe5ea544 100644 --- a/test/jasmine/tests/legend_scroll_test.js +++ b/test/jasmine/tests/legend_scroll_test.js @@ -6,6 +6,7 @@ var DBLCLICKDELAY = require('@src/constants/interactions').DBLCLICKDELAY; var d3 = require('d3'); var createGraph = require('../assets/create_graph_div'); var destroyGraph = require('../assets/destroy_graph_div'); +var failTest = require('../assets/fail_test'); var getBBox = require('../assets/get_bbox'); var mouseEvent = require('../assets/mouse_event'); var mock = require('../../image/mocks/legend_scroll.json'); @@ -48,6 +49,17 @@ describe('The legend', function() { return d3.select('g.legend').select('.legendtoggle').node(); } + function getScroll(gd) { + return gd._fullLayout.legend._scrollY; + } + + function hasScrollBar() { + var scrollBar = getScrollBar(); + return scrollBar && + +scrollBar.getAttribute('width') > 0 && + +scrollBar.getAttribute('height') > 0; + } + describe('when plotted with many traces', function() { var gd; @@ -85,21 +97,20 @@ describe('The legend', function() { var scrollBarYMax = legendHeight - scrollBar.getBoundingClientRect().height - 2 * constants.scrollBarMargin; - var initialDataScroll = scrollBox.getAttribute('data-scroll'); + var initialDataScroll = getScroll(gd); var wheelDeltaY = 100; - var finalDataScroll = '' + Lib.constrain(initialDataScroll - + var finalDataScroll = Lib.constrain(initialDataScroll - wheelDeltaY / scrollBarYMax * scrollBoxYMax, -scrollBoxYMax, 0); legend.dispatchEvent(scrollTo(wheelDeltaY)); - expect(scrollBox.getAttribute('data-scroll')).toBe(finalDataScroll); + expect(getScroll(gd)).toBe(finalDataScroll); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + finalDataScroll + ')'); }); function dragScroll(element, rightClick) { - var scrollBox = getScrollBox(); var scrollBar = getScrollBar(); var scrollBarBB = scrollBar.getBoundingClientRect(); var legendHeight = getLegendHeight(gd); @@ -107,9 +118,9 @@ describe('The legend', function() { var scrollBarYMax = legendHeight - scrollBarBB.height - 2 * constants.scrollBarMargin; - var initialDataScroll = scrollBox.getAttribute('data-scroll'); + var initialDataScroll = getScroll(gd); var dy = 50; - var finalDataScroll = '' + Lib.constrain(initialDataScroll - + var finalDataScroll = Lib.constrain(initialDataScroll - dy / scrollBarYMax * scrollBoxYMax, -scrollBoxYMax, 0); @@ -138,7 +149,7 @@ describe('The legend', function() { var finalDataScroll = dragScroll(getScrollBar()); var scrollBox = getScrollBox(); - var dataScroll = scrollBox.getAttribute('data-scroll'); + var dataScroll = getScroll(gd); expect(dataScroll).toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + dataScroll + ')'); @@ -148,7 +159,7 @@ describe('The legend', function() { var scrollBox = getScrollBox(); var finalDataScroll = dragScroll(scrollBox); - var dataScroll = scrollBox.getAttribute('data-scroll'); + var dataScroll = getScroll(gd); expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + dataScroll + ')'); @@ -158,12 +169,73 @@ describe('The legend', function() { var finalDataScroll = dragScroll(getScrollBar(), true); var scrollBox = getScrollBox(); - var dataScroll = scrollBox.getAttribute('data-scroll'); + var dataScroll = getScroll(gd); expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + dataScroll + ')'); }); + it('removes scroll bar and handlers when switching to horizontal', function(done) { + expect(hasScrollBar()).toBe(true); + + Plotly.relayout(gd, {'legend.orientation': 'h'}) + .then(function() { + expect(hasScrollBar()).toBe(false); + expect(getScroll(gd)).toBeUndefined(); + + getLegend().dispatchEvent(scrollTo(100)); + expect(hasScrollBar()).toBe(false); + expect(getScroll(gd)).toBeUndefined(); + + return Plotly.relayout(gd, {'legend.orientation': 'v'}); + }) + .then(function() { + expect(hasScrollBar()).toBe(true); + expect(getScroll(gd)).toBe(0); + + getLegend().dispatchEvent(scrollTo(100)); + expect(hasScrollBar()).toBe(true); + expect(getScroll(gd)).not.toBe(0); + }) + .catch(failTest) + .then(done); + }); + + it('updates scrollBar size/existence on deleteTraces', function(done) { + expect(hasScrollBar()).toBe(true); + var dataScroll = dragScroll(getScrollBar()); + var scrollBarHeight = getScrollBar().getBoundingClientRect().height; + var scrollBarHeight1; + + Plotly.deleteTraces(gd, [0]) + .then(function() { + expect(getScroll(gd)).toBeCloseTo(dataScroll, 3); + scrollBarHeight1 = getScrollBar().getBoundingClientRect().height; + expect(scrollBarHeight1).toBeGreaterThan(scrollBarHeight); + + // we haven't quite removed the scrollbar, but we should have clipped the scroll value + return Plotly.deleteTraces(gd, [0, 1, 2, 3, 4, 5, 6, 7]); + }) + .then(function() { + expect(getScroll(gd)).toBeGreaterThan(dataScroll + 1); + var scrollBarHeight2 = getScrollBar().getBoundingClientRect().height; + expect(scrollBarHeight2).toBeGreaterThan(scrollBarHeight1); + + // now no more scrollBar + return Plotly.deleteTraces(gd, [0, 1]); + }) + .then(function() { + expect(hasScrollBar()).toBe(false); + expect(getScroll(gd)).toBeUndefined(); + + getLegend().dispatchEvent(scrollTo(100)); + expect(hasScrollBar()).toBe(false); + expect(getScroll(gd)).toBeUndefined(); + }) + .catch(failTest) + .then(done); + }); + it('should keep the scrollbar position after a toggle event', function(done) { var legend = getLegend(), scrollBox = getScrollBox(), @@ -172,12 +244,12 @@ describe('The legend', function() { legend.dispatchEvent(scrollTo(wheelDeltaY)); - var dataScroll = scrollBox.getAttribute('data-scroll'); + var dataScroll = getScroll(gd); toggle.dispatchEvent(new MouseEvent('mousedown')); toggle.dispatchEvent(new MouseEvent('mouseup')); setTimeout(function() { expect(+toggle.parentNode.style.opacity).toBeLessThan(1); - expect(scrollBox.getAttribute('data-scroll')).toBe(dataScroll); + expect(getScroll(gd)).toBe(dataScroll); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + dataScroll + ')'); done(); @@ -210,12 +282,12 @@ describe('The legend', function() { expect(scrollBar.getAttribute('x')).toBe(scrollBarX); expect(scrollBar.getAttribute('y')).toBe(scrollBarY); - var dataScroll = scrollBox.getAttribute('data-scroll'); + var dataScroll = getScroll(gd); toggle.dispatchEvent(new MouseEvent('mousedown')); toggle.dispatchEvent(new MouseEvent('mouseup')); setTimeout(function() { expect(+toggle.parentNode.style.opacity).toBeLessThan(1); - expect(scrollBox.getAttribute('data-scroll')).toBe(dataScroll); + expect(getScroll(gd)).toBe(dataScroll); expect(scrollBox.getAttribute('transform')).toBe( 'translate(0, ' + dataScroll + ')'); expect(scrollBar.getAttribute('width')).toBeGreaterThan(0); From 5660bf57c25ac936d243dada472bfed57098513c Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 21:56:01 -0500 Subject: [PATCH 5/6] :hocho: `call` when not in a chain and when `this` isn't important - ie in all of our helper functions but we still need `call` for `d3.behavior.drag()` --- src/components/legend/draw.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index ed579cf6d1a..856e1668970 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -78,9 +78,9 @@ module.exports = function draw(gd) { 'shape-rendering': 'crispEdges' }); - bg.call(Color.stroke, opts.bordercolor); - bg.call(Color.fill, opts.bgcolor); - bg.style('stroke-width', opts.borderwidth + 'px'); + bg.call(Color.stroke, opts.bordercolor) + .call(Color.fill, opts.bgcolor) + .style('stroke-width', opts.borderwidth + 'px'); var scrollBox = legend.selectAll('g.scrollbox') .data([0]); @@ -229,7 +229,7 @@ module.exports = function draw(gd) { y: opts.borderwidth }); - scrollBox.call(Drawing.setClipUrl, clipId); + Drawing.setClipUrl(scrollBox, clipId); Drawing.setRect(scrollBar, 0, 0, 0, 0); delete opts._scrollY; @@ -268,7 +268,7 @@ module.exports = function draw(gd) { y: opts.borderwidth - scrollBoxY }); - scrollBox.call(Drawing.setClipUrl, clipId); + Drawing.setClipUrl(scrollBox, clipId); scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); @@ -306,11 +306,10 @@ module.exports = function draw(gd) { function scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio) { opts._scrollY = gd._fullLayout.legend._scrollY = scrollBoxY; - scrollBox - .call(Drawing.setTranslate, 0, scrollBoxY); + Drawing.setTranslate(scrollBox, 0, scrollBoxY); - scrollBar.call( - Drawing.setRect, + Drawing.setRect( + scrollBar, legendWidth, constants.scrollBarMargin - scrollBoxY * scrollRatio, constants.scrollBarWidth, @@ -441,7 +440,7 @@ function drawTexts(g, gd) { return Plotly.restyle(gd, update, traceIndex); }); } else { - text.call(textLayout); + textLayout(text); } } @@ -671,7 +670,7 @@ function computeLegendDimensions(gd, groups, traces) { var legendItem = d[0], bg = d3.select(this).select('.legendtoggle'); - bg.call(Drawing.setRect, + Drawing.setRect(bg, 0, -legendItem.height / 2, (gd._context.edits.legendText ? 0 : opts._width) + extraWidth, From f2d8cfd41742cba519caefd54146130bdd7c0d0f Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 28 Feb 2018 22:05:15 -0500 Subject: [PATCH 6/6] turn `scrollBoxY` into a positive number too confusing with it always being negative --- src/components/legend/draw.js | 21 +++++++++---------- test/jasmine/tests/legend_scroll_test.js | 26 ++++++++++++------------ 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index 856e1668970..04c5c77c48a 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -243,8 +243,7 @@ module.exports = function draw(gd) { var scrollBoxYMax = opts._height - legendHeight; var scrollRatio = scrollBarYMax / scrollBoxYMax; - // scrollBoxY is 0 or a negative number - var scrollBoxY = Math.max(opts._scrollY || 0, -scrollBoxYMax); + var scrollBoxY = Math.min(opts._scrollY || 0, scrollBoxYMax); // increase the background and clip-path width // by the scrollbar width and margin @@ -265,7 +264,7 @@ module.exports = function draw(gd) { constants.scrollBarMargin, height: legendHeight - 2 * opts.borderwidth, x: opts.borderwidth, - y: opts.borderwidth - scrollBoxY + y: opts.borderwidth + scrollBoxY }); Drawing.setClipUrl(scrollBox, clipId); @@ -274,11 +273,11 @@ module.exports = function draw(gd) { legend.on('wheel', function() { scrollBoxY = Lib.constrain( - opts._scrollY - + opts._scrollY + d3.event.deltaY / scrollBarYMax * scrollBoxYMax, - -scrollBoxYMax, 0); + 0, scrollBoxYMax); scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); - if(scrollBoxY !== 0 && scrollBoxY !== -scrollBoxYMax) { + if(scrollBoxY !== 0 && scrollBoxY !== scrollBoxYMax) { d3.event.preventDefault(); } }); @@ -295,8 +294,8 @@ module.exports = function draw(gd) { if(e.buttons === 2 || e.ctrlKey) return; scrollBoxY = Lib.constrain( - (eventY0 - e.clientY) / scrollRatio + scrollBoxY0, - -scrollBoxYMax, 0); + (e.clientY - eventY0) / scrollRatio + scrollBoxY0, + 0, scrollBoxYMax); scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio); }); @@ -306,17 +305,17 @@ module.exports = function draw(gd) { function scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio) { opts._scrollY = gd._fullLayout.legend._scrollY = scrollBoxY; - Drawing.setTranslate(scrollBox, 0, scrollBoxY); + Drawing.setTranslate(scrollBox, 0, -scrollBoxY); Drawing.setRect( scrollBar, legendWidth, - constants.scrollBarMargin - scrollBoxY * scrollRatio, + constants.scrollBarMargin + scrollBoxY * scrollRatio, constants.scrollBarWidth, scrollBarHeight ); clipPath.select('rect').attr({ - y: opts.borderwidth - scrollBoxY + y: opts.borderwidth + scrollBoxY }); } diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js index 93dbe5ea544..182aa0ba86f 100644 --- a/test/jasmine/tests/legend_scroll_test.js +++ b/test/jasmine/tests/legend_scroll_test.js @@ -99,15 +99,15 @@ describe('The legend', function() { 2 * constants.scrollBarMargin; var initialDataScroll = getScroll(gd); var wheelDeltaY = 100; - var finalDataScroll = Lib.constrain(initialDataScroll - + var finalDataScroll = Lib.constrain(initialDataScroll + wheelDeltaY / scrollBarYMax * scrollBoxYMax, - -scrollBoxYMax, 0); + 0, scrollBoxYMax); legend.dispatchEvent(scrollTo(wheelDeltaY)); expect(getScroll(gd)).toBe(finalDataScroll); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + finalDataScroll + ')'); + 'translate(0, ' + -finalDataScroll + ')'); }); function dragScroll(element, rightClick) { @@ -120,9 +120,9 @@ describe('The legend', function() { 2 * constants.scrollBarMargin; var initialDataScroll = getScroll(gd); var dy = 50; - var finalDataScroll = Lib.constrain(initialDataScroll - + var finalDataScroll = Lib.constrain(initialDataScroll + dy / scrollBarYMax * scrollBoxYMax, - -scrollBoxYMax, 0); + 0, scrollBoxYMax); var y0 = scrollBarBB.top + scrollBarBB.height / 5; var y1 = y0 + dy; @@ -152,7 +152,7 @@ describe('The legend', function() { var dataScroll = getScroll(gd); expect(dataScroll).toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + dataScroll + ')'); + 'translate(0, ' + -dataScroll + ')'); }); it('should not scroll on dragging the scrollbox', function() { @@ -162,7 +162,7 @@ describe('The legend', function() { var dataScroll = getScroll(gd); expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + dataScroll + ')'); + 'translate(0, ' + -dataScroll + ')'); }); it('should not scroll on dragging the scrollbar with a right click', function() { @@ -172,7 +172,7 @@ describe('The legend', function() { var dataScroll = getScroll(gd); expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + dataScroll + ')'); + 'translate(0, ' + -dataScroll + ')'); }); it('removes scroll bar and handlers when switching to horizontal', function(done) { @@ -214,15 +214,15 @@ describe('The legend', function() { expect(scrollBarHeight1).toBeGreaterThan(scrollBarHeight); // we haven't quite removed the scrollbar, but we should have clipped the scroll value - return Plotly.deleteTraces(gd, [0, 1, 2, 3, 4, 5, 6, 7]); + return Plotly.deleteTraces(gd, [0, 1, 2, 3, 4, 5, 6]); }) .then(function() { - expect(getScroll(gd)).toBeGreaterThan(dataScroll + 1); + expect(getScroll(gd)).toBeLessThan(dataScroll - 1); var scrollBarHeight2 = getScrollBar().getBoundingClientRect().height; expect(scrollBarHeight2).toBeGreaterThan(scrollBarHeight1); // now no more scrollBar - return Plotly.deleteTraces(gd, [0, 1]); + return Plotly.deleteTraces(gd, [0, 1, 2]); }) .then(function() { expect(hasScrollBar()).toBe(false); @@ -251,7 +251,7 @@ describe('The legend', function() { expect(+toggle.parentNode.style.opacity).toBeLessThan(1); expect(getScroll(gd)).toBe(dataScroll); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + dataScroll + ')'); + 'translate(0, ' + -dataScroll + ')'); done(); }, DBLCLICKDELAY * 2); }); @@ -289,7 +289,7 @@ describe('The legend', function() { expect(+toggle.parentNode.style.opacity).toBeLessThan(1); expect(getScroll(gd)).toBe(dataScroll); expect(scrollBox.getAttribute('transform')).toBe( - 'translate(0, ' + dataScroll + ')'); + 'translate(0, ' + -dataScroll + ')'); expect(scrollBar.getAttribute('width')).toBeGreaterThan(0); expect(scrollBar.getAttribute('height')).toBeGreaterThan(0); done();