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

Fixup misinterpreted clip-path by some programs after export #5686

Merged
merged 2 commits into from
May 26, 2021
Merged

Fixup misinterpreted clip-path by some programs after export #5686

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa1622f
do not add quotes around url
archmoj May 26, 2021
3a5d532
keep quotes in the case of base url
archmoj May 26, 2021
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 @@ -1270,7 +1270,7 @@ function getFullUrl(localId, gd) {

var context = gd._context;
var baseUrl = context._exportedPlot ? '' : (context._baseUrl || '');
return 'url(\'' + baseUrl + '#' + localId + '\')';
return 'url(' + baseUrl + '#' + localId + ')';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per #5685 (comment) I think it's safer to keep the quotes when baseUrl is truthy (noting that the _exportedPlot condition appropriately clears it vs context._baseUrl).

}

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 = d3Select(this).attr('clip-path');

if(isClipped) {
expect(String(clipPath).substr(0, 5))
.toBe('url(\'', msg + ' clip path ' + '(item ' + i + ')');
expect(String(clipPath).substr(0, 4))
.toBe('url(', msg + ' clip path ' + '(item ' + i + ')');
} else {
expect(clipPath)
.toBe(null, msg + ' clip path ' + '(item ' + i + ')');
Expand Down
2 changes: 1 addition & 1 deletion 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(6, clipPathAttr.length - 2);
var clipPathId = clipPathAttr.substring(5, clipPathAttr.length - 1);
var clipBBox = getClipBBox(clipPathId);

return minBBox(elementBBox, clipBBox);
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 @@ -524,22 +524,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(d3Select('image').attr('clip-path') || '').toMatch(/xy\'\)$/);
expect(d3Select('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(d3Select('image').attr('clip-path') || '').toMatch(/xy2\'\)$/);
expect(d3Select('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(d3Select('image').attr('clip-path') || '').toMatch(/x\'\)$/);
expect(d3Select('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 @@ -553,7 +553,7 @@ describe('images log/linear axis changes', function() {
.then(function() {
expect(gd.layout.images[0].y).toBe(1);

expect(d3Select('image').attr('clip-path') || '').toMatch(/^[^x]+y2\'\)$/);
expect(d3Select('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() {
d3SelectAll('[clip-path]').each(function() {
var cp = d3Select(this).attr('clip-path');

expect(cp.substring(0, 6)).toEqual('url(\'#');
expect(cp.substring(cp.length - 2)).toEqual('\')');
expect(cp.substring(0, 5)).toEqual('url(#');
expect(cp.substring(cp.length - 1)).toEqual(')');
});
})
.then(done, done.fail);
Expand All @@ -568,8 +568,8 @@ describe('plot svg clip paths', function() {
d3SelectAll('[clip-path]').each(function() {
var cp = d3Select(this).attr('clip-path');

expect(cp.substring(0, 6 + href.length)).toEqual('url(\'' + href + '#');
expect(cp.substring(cp.length - 2)).toEqual('\')');
expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#');
expect(cp.substring(cp.length - 1)).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 @@ -494,7 +494,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 @@ -511,7 +511,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 @@ -520,7 +520,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\)$/);
})
.then(done, done.fail);
});
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/snapshot_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ describe('Plotly.Snapshot', function() {

describe('should handle quoted style properties', function() {
function checkURL(actual, msg) {
// which is enough tot check that toSVG did its job right
// which is enough to check that toSVG did its job right
expect((actual || '').substr(0, 6)).toBe('url(\"#', msg);
}

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 @@ -239,10 +239,10 @@ describe('Plotly.toImage', function() {
var clipPath = gSubplot.getAttribute('clip-path');
var len = clipPath.length;

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 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 middle = clipPath.substr(4, 10);
expect(middle.length).toBe(10, 'subplot clipPath uid length');
Expand Down