Skip to content

Legend scroll fix #2426

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

Merged
merged 6 commits into from
Mar 1, 2018
Merged

Legend scroll fix #2426

Show file tree
Hide file tree
Changes from 2 commits
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
2 changes: 1 addition & 1 deletion src/components/dragelement/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/components/legend/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
'use strict';

module.exports = {
scrollBarWidth: 4,
scrollBarWidth: 6,
scrollBarHeight: 20,
scrollBarColor: '#808BA4',
scrollBarMargin: 4
Expand Down
23 changes: 16 additions & 7 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -282,9 +282,19 @@ module.exports = function draw(gd) {
scrollBar.on('.drag', null);
scrollBox.on('.drag', null);

var drag = d3.behavior.drag().on('drag', function() {
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(
d3.event.y - constants.scrollBarHeight / 2,
e.clientY - eventY0 + scrollBarY0,
constants.scrollBarMargin,
constants.scrollBarMargin + scrollBarYMax);
scrollBoxY = - (scrollBarY - constants.scrollBarMargin) /
Expand All @@ -293,7 +303,6 @@ module.exports = function draw(gd) {
});

scrollBar.call(drag);
scrollBox.call(drag);
}


Expand Down
89 changes: 77 additions & 12 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 @@ -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() {
Expand Down Expand Up @@ -76,18 +77,19 @@ 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 scrollBar = getScrollBar();
var legendHeight = getLegendHeight(gd);
var scrollBoxYMax = gd._fullLayout.legend._height - legendHeight;
var scrollBarYMax = legendHeight -
scrollBar.getBoundingClientRect().height -
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));

Expand All @@ -96,6 +98,69 @@ describe('The legend', function() {
'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 dy = 50;
var finalDataScroll = '' + Lib.constrain(initialDataScroll -
dy / scrollBarYMax * scrollBoxYMax,
-scrollBoxYMax, 0);

var y0 = scrollBarBB.top + scrollBarBB.height / 5;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

notice that I moved the drag point away from the center of the scrollBar, thereby testing that it drags from where you grab it rather than snapping to the center.

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these general legend scrollbox tests and/or tests that 🔒 the fix for #2387? (I'm asking as I would be surprised if #2387 was reproducible in a jasmine test).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, these do test #2387, if I'm interpreting it correctly. What I think was happening in #2387 is that somehow the scrollBox drag was being triggered - and that's what I decided (with no dissent) had no reason ever to happen. So this test shows that regular drag works, then the next one shows that the scrollBox won't drag even if it does somehow get the event, and then the third new test shows that right-click (the only form of #2387 I could reproduce on my own computer) even on the scrollBar will not trigger drag.

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(),
Expand Down