Skip to content

Commit ae820ca

Browse files
committed
test and fix correct clip path and count of drawn shapes
particularly in cases of partial data referencing and layer: below
1 parent 06523d6 commit ae820ca

File tree

2 files changed

+79
-8
lines changed

2 files changed

+79
-8
lines changed

src/components/shapes/draw.js

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,11 @@ function drawOne(gd, index) {
7070
// TODO: use d3 idioms instead of deleting and redrawing every time
7171
if(!optionsIn || options.visible === false) return;
7272

73-
var clipAxes;
73+
// var clipAxes;
7474
if(options.layer !== 'below') {
75-
clipAxes = (options.xref + options.yref).replace(/paper/g, '');
7675
drawShape(gd._fullLayout._shapeUpperLayer);
7776
}
7877
else if(options.xref === 'paper' && options.yref === 'paper') {
79-
clipAxes = '';
8078
drawShape(gd._fullLayout._shapeLowerLayer);
8179
}
8280
else {
@@ -86,10 +84,13 @@ function drawOne(gd, index) {
8684

8785
for(i = 0, n = subplots.length; i < n; i++) {
8886
plotinfo = plots[subplots[i]];
89-
clipAxes = subplots[i];
9087

9188
if(isShapeInSubplot(gd, options, plotinfo)) {
9289
drawShape(plotinfo.shapelayer);
90+
91+
// make sure we only draw the shape once.
92+
// See https://github.com/plotly/plotly.js/issues/1452
93+
break;
9394
}
9495
}
9596
}
@@ -110,10 +111,15 @@ function drawOne(gd, index) {
110111
.call(Color.fill, options.fillcolor)
111112
.call(Drawing.dashLine, options.line.dash, options.line.width);
112113

113-
if(clipAxes) {
114-
path.call(Drawing.setClipUrl,
115-
'clip' + gd._fullLayout._uid + clipAxes);
116-
}
114+
// note that for layer="below" the clipAxes can be different from the
115+
// subplot we're drawing this in. This could cause problems if the shape
116+
// spans two subplots. See https://github.com/plotly/plotly.js/issues/1452
117+
var clipAxes = (options.xref + options.yref).replace(/paper/g, '');
118+
119+
path.call(Drawing.setClipUrl, clipAxes ?
120+
('clip' + gd._fullLayout._uid + clipAxes) :
121+
null
122+
);
117123

118124
if(gd._context.editable) setupDragElement(gd, path, options, index);
119125
}

test/jasmine/tests/shapes_test.js

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,71 @@ describe('Test shapes:', function() {
382382
});
383383
});
384384

385+
describe('shapes axis reference changes', function() {
386+
'use strict';
387+
388+
var gd;
389+
390+
beforeEach(function(done) {
391+
gd = createGraphDiv();
392+
393+
Plotly.plot(gd, [
394+
{y: [1, 2, 3]},
395+
{y: [1, 2, 3], yaxis: 'y2'}
396+
], {
397+
yaxis: {domain: [0, 0.4]},
398+
yaxis2: {domain: [0.6, 1]},
399+
shapes: [{
400+
xref: 'x', yref: 'paper', type: 'rect',
401+
x0: 0.8, x1: 1.2, y0: 0, y1: 1,
402+
fillcolor: '#eee', layer: 'below'
403+
}]
404+
}).then(done);
405+
});
406+
407+
afterEach(destroyGraphDiv);
408+
409+
function getShape(index) {
410+
var s = d3.selectAll('path[data-index="' + index + '"]');
411+
expect(s.size()).toBe(1);
412+
return s;
413+
}
414+
415+
it('draws the right number of objects and updates clip-path correctly', function(done) {
416+
417+
expect(getShape(0).attr('clip-path') || '').toMatch(/x\)$/);
418+
419+
Plotly.relayout(gd, {
420+
'shapes[0].xref': 'paper',
421+
'shapes[0].x0': 0.2,
422+
'shapes[0].x1': 0.6
423+
})
424+
.then(function() {
425+
expect(getShape(0).attr('clip-path')).toBe(null);
426+
427+
return Plotly.relayout(gd, {
428+
'shapes[0].yref': 'y2',
429+
'shapes[0].y0': 1.8,
430+
'shapes[0].y1': 2.2,
431+
});
432+
})
433+
.then(function() {
434+
expect(getShape(0).attr('clip-path') || '').toMatch(/^[^x]+y2\)$/);
435+
436+
return Plotly.relayout(gd, {
437+
'shapes[0].xref': 'x',
438+
'shapes[0].x0': 1.5,
439+
'shapes[0].x1': 20
440+
});
441+
})
442+
.then(function() {
443+
expect(getShape(0).attr('clip-path') || '').toMatch(/xy2\)$/);
444+
})
445+
.catch(failTest)
446+
.then(done);
447+
});
448+
});
449+
385450
describe('shapes autosize', function() {
386451
'use strict';
387452

0 commit comments

Comments
 (0)