Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Legend scroll fix #2426

Merged
merged 6 commits into from
Mar 1, 2018
Merged

Legend scroll fix #2426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f75f5ba
fix #2387 - prevent legend scrolling from scrollBox drag or right click
alexcjohnson Feb 28, 2018
ec42784
grab the legend scrollbar at the cursor, not at its middle
alexcjohnson Feb 28, 2018
246b77f
make legend scrollbar scale to the visible fraction
alexcjohnson Feb 28, 2018
569b378
fix #808 - update scrollBar when changing other legend attributes
alexcjohnson Mar 1, 2018
5660bf5
:hocho: `call` when not in a chain
alexcjohnson Mar 1, 2018
f2d8cfd
turn `scrollBoxY` into a positive number
alexcjohnson Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 22 additions & 24 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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,
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Way better to use _ then a DOM attr 🎉

// 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
Expand All @@ -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
);
Expand Down
98 changes: 85 additions & 13 deletions test/jasmine/tests/legend_scroll_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -85,31 +97,30 @@ 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);
var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight;
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);

Expand Down Expand Up @@ -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 + ')');
Expand All @@ -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 + ')');
Expand All @@ -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(),
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand Down