Skip to content

Commit f247d93

Browse files
rmoestlrmoestl
committed
Ensure 'plotly_click' is fired after 'plotly_selected' [1852]
1 parent 88e9993 commit f247d93

File tree

6 files changed

+98
-68
lines changed

6 files changed

+98
-68
lines changed

‎src/plots/cartesian/dragbox.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -231,13 +231,13 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
231231
if(numClicks === 2 && !singleEnd) doubleClick();
232232

233233
if(isMainDrag) {
234-
if(clickmode.indexOf('event') > -1) {
235-
Fx.click(gd, evt, plotinfo.id);
236-
}
237-
238234
if(clickmode.indexOf('select') > -1) {
239235
selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions);
240236
}
237+
238+
if(clickmode.indexOf('event') > -1) {
239+
Fx.click(gd, evt, plotinfo.id);
240+
}
241241
}
242242
else if(numClicks === 1 && singleEnd) {
243243
var ax = ns ? ya0 : xa0,

‎src/plots/geo/geo.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -452,18 +452,18 @@ proto.updateFx = function(fullLayout, geoLayout) {
452452
bgRect.on('click', function() {
453453
// For select and lasso the dragElement is handling clicks
454454
if(dragMode !== 'select' && dragMode !== 'lasso') {
455+
if(clickMode.indexOf('select') > -1) {
456+
selectOnClick(d3.event, gd, [_this.xaxis], [_this.yaxis],
457+
_this.id, dragOptions);
458+
}
459+
455460
if(clickMode.indexOf('event') > -1) {
456461
// TODO: like pie and mapbox, this doesn't support right-click
457462
// actually this one is worse, as right-click starts a pan, or leaves
458463
// select in a weird state.
459464
// Also, only tangentially related, we should cancel hover during pan
460465
Fx.click(gd, d3.event);
461466
}
462-
463-
if(clickMode.indexOf('select') > -1) {
464-
selectOnClick(d3.event, gd, [_this.xaxis], [_this.yaxis],
465-
_this.id, dragOptions);
466-
}
467467
}
468468
});
469469
};

‎src/plots/mapbox/mapbox.js

+9-13
Original file line numberDiff line numberDiff line change
@@ -177,15 +177,6 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) {
177177
Fx.hover(gd, evt, self.id);
178178
});
179179

180-
map.on('click', function(evt) {
181-
// TODO: this does not support right-click. If we want to support it, we
182-
// would likely need to change mapbox to use dragElement instead of straight
183-
// mapbox event binding. Or perhaps better, make a simple wrapper with the
184-
// right mousedown, mousemove, and mouseup handlers just for a left/right click
185-
// pie would use this too.
186-
Fx.click(gd, evt.originalEvent);
187-
});
188-
189180
function unhover() {
190181
Fx.loneUnhover(fullLayout._toppaper);
191182
}
@@ -236,13 +227,18 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) {
236227
return function(evt) {
237228
var clickMode = gd._fullLayout.clickmode;
238229

239-
if(clickMode.indexOf('event') > -1) {
240-
Fx.click(gd, evt.originalEvent);
241-
}
242-
243230
if(clickMode.indexOf('select') > -1) {
244231
selectOnClick(evt.originalEvent, gd, [self.xaxis], [self.yaxis], self.id, dragOptions);
245232
}
233+
234+
if(clickMode.indexOf('event') > -1) {
235+
// TODO: this does not support right-click. If we want to support it, we
236+
// would likely need to change mapbox to use dragElement instead of straight
237+
// mapbox event binding. Or perhaps better, make a simple wrapper with the
238+
// right mousedown, mousemove, and mouseup handlers just for a left/right click
239+
// pie would use this too.
240+
Fx.click(gd, evt.originalEvent);
241+
}
246242
};
247243
};
248244
};

‎src/plots/polar/polar.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -849,13 +849,13 @@ proto.updateMainDrag = function(fullLayout, polarLayout) {
849849
Registry.call('relayout', gd, updateObj);
850850
}
851851

852-
if(clickMode.indexOf('event') > -1) {
853-
Fx.click(gd, evt, _this.id);
854-
}
855-
856852
if(clickMode.indexOf('select') > -1 && numClicks === 1) {
857853
selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOpts);
858854
}
855+
856+
if(clickMode.indexOf('event') > -1) {
857+
Fx.click(gd, evt, _this.id);
858+
}
859859
}
860860

861861
dragOpts.prepFn = function(evt, startX, startY) {

‎src/plots/ternary/ternary.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -503,13 +503,13 @@ proto.initInteractions = function() {
503503
Registry.call('relayout', gd, attrs);
504504
}
505505

506-
if(clickMode.indexOf('event') > -1) {
507-
Fx.click(gd, evt, _this.id);
508-
}
509-
510506
if(clickMode.indexOf('select') > -1 && numClicks === 1) {
511507
selectOnClick(evt, gd, [_this.xaxis], [_this.yaxis], _this.id, dragOptions);
512508
}
509+
510+
if(clickMode.indexOf('event') > -1) {
511+
Fx.click(gd, evt, _this.id);
512+
}
513513
}
514514

515515
function zoomPrep(e, startX, startY) {

‎test/jasmine/tests/select_test.js

+72-38
Original file line numberDiff line numberDiff line change
@@ -649,28 +649,8 @@ describe('Click-to-select', function() {
649649
]
650650
.forEach(function(testCase) {
651651
var ciAnnotation = testCase.gl ? 'gl' : 'flaky';
652-
it('@' + ciAnnotation + ' trace type ' + testCase.traceType, function(done) {
653-
var defaultLayoutOpts = {
654-
layout: {
655-
clickmode: 'event+select',
656-
dragmode: 'pan',
657-
hovermode: 'closest'
658-
}
659-
};
660-
var customLayoutOptions = {
661-
layout: testCase.layoutOptions
662-
};
663-
var customConfigOptions = {
664-
config: testCase.configOptions
665-
};
666-
var mockCopy = Lib.extendDeep(
667-
{},
668-
testCase.mock,
669-
defaultLayoutOpts,
670-
customLayoutOptions,
671-
customConfigOptions);
672-
673-
Plotly.plot(gd, mockCopy.data, mockCopy.layout, mockCopy.config)
652+
it('@' + ciAnnotation + ' trace type ' + testCase.label, function(done) {
653+
Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config)
674654
.then(function() {
675655
return _immediateClickPt(testCase);
676656
})
@@ -693,24 +673,78 @@ describe('Click-to-select', function() {
693673
.then(done);
694674
});
695675
});
676+
});
696677

697-
function testCase(traceType, mock, x, y, expectedPts, layoutOptions, configOptions) {
698-
return {
699-
traceType: traceType,
700-
mock: mock,
701-
layoutOptions: layoutOptions,
702-
x: x,
703-
y: y,
704-
expectedPts: expectedPts,
705-
configOptions: configOptions,
706-
gl: false,
707-
enableGl: function() {
708-
this.gl = true;
709-
return this;
710-
}
711-
};
712-
}
678+
describe('triggers \'plotly_selected\' before \'plotly_click\'', function() {
679+
[
680+
testCase('cartesian', require('@mocks/14.json'), 270, 160, [7]),
681+
testCase('geo', require('@mocks/geo_scattergeo-locations.json'), 285, 240, [1]),
682+
testCase('ternary', require('@mocks/ternary_markers.json'), 485, 335, [7]),
683+
testCase('mapbox', require('@mocks/mapbox_0.json'), 650, 195, [[2], []], {},
684+
{ mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN }),
685+
testCase('polar', require('@mocks/polar_scatter.json'), 130, 290,
686+
[[], [], [], [19], [], []], { dragmode: 'zoom' })
687+
].forEach(function(testCase) {
688+
it('@flaky for base plot ' + testCase.label, function(done) {
689+
Plotly.plot(gd, testCase.mock.data, testCase.mock.layout, testCase.mock.config)
690+
.then(function() {
691+
var clickHandlerCalled = false;
692+
var selectedHandlerCalled = false;
693+
694+
gd.on('plotly_selected', function() {
695+
expect(clickHandlerCalled).toBe(false);
696+
selectedHandlerCalled = true;
697+
});
698+
gd.on('plotly_click', function() {
699+
clickHandlerCalled = true;
700+
expect(selectedHandlerCalled).toBe(true);
701+
done();
702+
});
703+
704+
return click(testCase.x, testCase.y);
705+
})
706+
.catch(failTest)
707+
.then(done);
708+
});
709+
});
713710
});
711+
712+
function testCase(label, mock, x, y, expectedPts, layoutOptions, configOptions) {
713+
var defaultLayoutOpts = {
714+
layout: {
715+
clickmode: 'event+select',
716+
dragmode: 'pan',
717+
hovermode: 'closest'
718+
}
719+
};
720+
var customLayoutOptions = {
721+
layout: layoutOptions
722+
};
723+
var customConfigOptions = {
724+
config: configOptions
725+
};
726+
var mockCopy = Lib.extendDeep(
727+
{},
728+
mock,
729+
defaultLayoutOpts,
730+
customLayoutOptions,
731+
customConfigOptions);
732+
733+
return {
734+
label: label,
735+
mock: mockCopy,
736+
layoutOptions: layoutOptions,
737+
x: x,
738+
y: y,
739+
expectedPts: expectedPts,
740+
configOptions: configOptions,
741+
gl: false,
742+
enableGl: function() {
743+
this.gl = true;
744+
return this;
745+
}
746+
};
747+
}
714748
});
715749

716750
describe('Test select box and lasso in general:', function() {

0 commit comments

Comments
 (0)