Skip to content

Fix3724 parenthesis in url draws data outside plot area #3725

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 3 commits into from
Apr 5, 2019
Merged

Fix3724 parenthesis in url draws data outside plot area #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
386dc19
add single quotes to clip path url
Apr 4, 2019
e81942a
update tests
Apr 4, 2019
aefd2ab
fixup getBBox test asset
etpinard Apr 5, 2019
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/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ drawing.setClipUrl = function(s, localId, gd) {
var context = gd._context;
var baseUrl = context._exportedPlot ? '' : (context._baseUrl || '');

s.attr('clip-path', 'url(' + baseUrl + '#' + localId + ')');
s.attr('clip-path', 'url(\'' + baseUrl + '#' + localId + '\')');
};

drawing.getTranslate = function(element) {
Expand Down
4 changes: 2 additions & 2 deletions test/jasmine/assets/custom_assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,8 @@ exports.assertClip = function(sel, isClipped, size, msg) {
var clipPath = d3.select(this).attr('clip-path');

if(isClipped) {
expect(String(clipPath).substr(0, 4))
.toBe('url(', msg + ' clip path ' + '(item ' + i + ')');
expect(String(clipPath).substr(0, 5))
.toBe('url(\'', msg + ' clip path ' + '(item ' + i + ')');
} else {
expect(clipPath)
.toBe(null, msg + ' clip path ' + '(item ' + i + ')');
Expand Down
5 changes: 2 additions & 3 deletions test/jasmine/assets/get_bbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = function getBBox(element) {
if(!clipPathAttr) return elementBBox;

// only supports 'url(#<id>)' at the moment
var clipPathId = clipPathAttr.substring(5, clipPathAttr.length - 1);
var clipPathId = clipPathAttr.substring(6, clipPathAttr.length - 2);
var clipBBox = getClipBBox(clipPathId);

return minBBox(elementBBox, clipBBox);
Expand All @@ -28,8 +28,7 @@ function getClipBBox(clipPathId) {
try {
// this line throws an error in FF (38 and 45 at least)
clipBBox = clipPath.node().getBBox();
}
catch(e) {
} catch(e) {
// use DOM attributes as fallback
var path = d3.select(clipPath.node().firstChild);

Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/drawing_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ describe('Drawing', function() {

Drawing.setClipUrl(this.g, 'id1', {_context: {}});

expect(this.g.attr('clip-path')).toEqual('url(#id1)');
expect(this.g.attr('clip-path')).toEqual('url(\'#id1\')');
});

it('should unset the clip-path if arg is falsy', function() {
this.g.attr('clip-path', 'url(#id2)');
this.g.attr('clip-path', 'url(\'#id2\')');

Drawing.setClipUrl(this.g, false);

Expand All @@ -48,7 +48,7 @@ describe('Drawing', function() {
Drawing.setClipUrl(this.g, 'id3', {_context: {_baseUrl: href}});

expect(this.g.attr('clip-path'))
.toEqual('url(' + href + '#id3)');
.toEqual('url(\'' + href + '#id3\')');

base.remove();
});
Expand All @@ -64,7 +64,7 @@ describe('Drawing', function() {

Drawing.setClipUrl(this.g, 'id4', {_context: {_baseUrl: href2}});

var expected = 'url(' + href2 + '#id4)';
var expected = 'url(\'' + href2 + '#id4\')';

expect(this.g.attr('clip-path')).toEqual(expected);

Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/layout_images_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,22 +448,22 @@ describe('images log/linear axis changes', function() {
// automatically when you change xref / yref, we leave it to the caller.

// initial clip path should end in 'xy' to match xref/yref
expect(d3.select('image').attr('clip-path') || '').toMatch(/xy\)$/);
expect(d3.select('image').attr('clip-path') || '').toMatch(/xy\'\)$/);

// linear to log
Plotly.relayout(gd, {'images[0].yref': 'y2'})
.then(function() {
expect(gd.layout.images[0].y).toBe(1);

expect(d3.select('image').attr('clip-path') || '').toMatch(/xy2\)$/);
expect(d3.select('image').attr('clip-path') || '').toMatch(/xy2\'\)$/);

// log to paper
return Plotly.relayout(gd, {'images[0].yref': 'paper'});
})
.then(function() {
expect(gd.layout.images[0].y).toBe(1);

expect(d3.select('image').attr('clip-path') || '').toMatch(/x\)$/);
expect(d3.select('image').attr('clip-path') || '').toMatch(/x\'\)$/);

// change to full paper-referenced, to make sure the clip path disappears
return Plotly.relayout(gd, {'images[0].xref': 'paper'});
Expand All @@ -477,7 +477,7 @@ describe('images log/linear axis changes', function() {
.then(function() {
expect(gd.layout.images[0].y).toBe(1);

expect(d3.select('image').attr('clip-path') || '').toMatch(/^[^x]+y2\)$/);
expect(d3.select('image').attr('clip-path') || '').toMatch(/^[^x]+y2\'\)$/);

// log to linear
return Plotly.relayout(gd, {'images[0].yref': 'y'});
Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,8 +545,8 @@ describe('plot svg clip paths', function() {
d3.selectAll('[clip-path]').each(function() {
var cp = d3.select(this).attr('clip-path');

expect(cp.substring(0, 5)).toEqual('url(#');
expect(cp.substring(cp.length - 1)).toEqual(')');
expect(cp.substring(0, 6)).toEqual('url(\'#');
expect(cp.substring(cp.length - 2)).toEqual('\')');
});
})
.catch(failTest)
Expand All @@ -569,8 +569,8 @@ describe('plot svg clip paths', function() {
d3.selectAll('[clip-path]').each(function() {
var cp = d3.select(this).attr('clip-path');

expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#');
expect(cp.substring(cp.length - 1)).toEqual(')');
expect(cp.substring(0, 6 + href.length)).toEqual('url(\'' + href + '#');
expect(cp.substring(cp.length - 2)).toEqual('\')');
});

base.remove();
Expand Down
6 changes: 3 additions & 3 deletions test/jasmine/tests/shapes_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ describe('shapes axis reference changes', function() {
}

it('draws the right number of objects and updates clip-path correctly', function(done) {
expect(getShape(0).attr('clip-path') || '').toMatch(/x\)$/);
expect(getShape(0).attr('clip-path') || '').toMatch(/x\'\)$/);

Plotly.relayout(gd, {
'shapes[0].xref': 'paper',
Expand All @@ -475,7 +475,7 @@ describe('shapes axis reference changes', function() {
});
})
.then(function() {
expect(getShape(0).attr('clip-path') || '').toMatch(/^[^x]+y2\)$/);
expect(getShape(0).attr('clip-path') || '').toMatch(/^[^x]+y2\'\)$/);

return Plotly.relayout(gd, {
'shapes[0].xref': 'x',
Expand All @@ -484,7 +484,7 @@ describe('shapes axis reference changes', function() {
});
})
.then(function() {
expect(getShape(0).attr('clip-path') || '').toMatch(/xy2\)$/);
expect(getShape(0).attr('clip-path') || '').toMatch(/xy2\'\)$/);
})
.catch(failTest)
.then(done);
Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/toimage_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,10 @@ describe('Plotly.toImage', function() {
var clipPath = gSubplot.getAttribute('clip-path');
var len = clipPath.length;

var head = clipPath.substr(0, 4);
var tail = clipPath.substr(len - 7, len);
expect(head).toBe('url(', 'subplot clipPath head');
expect(tail).toBe('xyplot)', 'subplot clipPath tail');
var head = clipPath.substr(0, 5);
var tail = clipPath.substr(len - 8, len);
expect(head).toBe('url(\'', 'subplot clipPath head');
expect(tail).toBe('xyplot\')', 'subplot clipPath tail');

var middle = clipPath.substr(4, 10);
expect(middle.length).toBe(10, 'subplot clipPath uid length');
Expand Down