Skip to content

Fix toImage for marker gradient #1694

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 1 commit into from
May 18, 2017
Merged

Fix toImage for marker gradient #1694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 17 additions & 3 deletions src/snapshot/tosvg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ var Drawing = require('../components/drawing');
var Color = require('../components/color');

var xmlnsNamespaces = require('../constants/xmlns_namespaces');
var DOUBLEQUOTE_REGEX = /"/g;
var DUMMY_SUB = 'TOBESTRIPPED';
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any way this can occur in other contexts in the SVG string? Or a way to add characters that make it more obviously impossible?

Copy link
Contributor Author

@etpinard etpinard May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not that I can think of.

The gradient fill property is set by us and even if font.family: 'TOBESTRIPPED' then in the DOM this will look like: font-family: TOBESTRIPPED (no double quotes) which won't trigger that .replace(DOUBLEQUOTE_REGEX, DUMMY_SUB) call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, looks like this is already a problem in master:

Plotly.relayout(gd,{'yaxis.title':'"TOBESTRIPPED"'})

on screen:
screen shot 2017-05-17 at 5 42 03 pm

download:
newplot 2

So you're not making a new bug, but we do need something better for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. What's the solution then?

  • We could make the regex more precise here so that it grabs only TOBESTRIPPED inside style= props.
  • Use another (more esoteric) dummy string
  • Use another seriliazer than what's the browsers give us on the window object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, I can't see an easy bulletproof solution. Lets just make an issue for the moment since the problem isn't new. Otherwise this looks great. I was surprised that this showed up in gradients since it didn't show up in clip paths (which also use these urls) but I guess only style attributes get these extra quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess only style attributes get these extra quotes.

Exactly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue ➡️ #1697

Copy link
Collaborator

Choose a reason for hiding this comment

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

Issue ➡️ #1697

thanks!

var DUMMY_REGEX = new RegExp('("' + DUMMY_SUB + ')|(' + DUMMY_SUB + '")', 'g');


module.exports = function toSVG(gd, format) {
Expand Down Expand Up @@ -90,10 +93,21 @@ module.exports = function toSVG(gd, format) {
// to a string (browsers convert singles back)
var ff = txt.style('font-family');
if(ff && ff.indexOf('"') !== -1) {
txt.style('font-family', ff.replace(/"/g, 'TOBESTRIPPED'));
txt.style('font-family', ff.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB));
}
});

svg.selectAll('.point').each(function() {
var pt = d3.select(this);
var fill = pt.style('fill');

// similar to font family styles above,
// we must remove " after the SVG DOM has been serialized
if(fill && fill.indexOf('url(') !== -1) {
pt.style('fill', fill.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB));
}
});

if(format === 'pdf' || format === 'eps') {
// these formats make the extra line MathJax adds around symbols look super thick in some cases
// it looks better if this is removed entirely.
Expand All @@ -110,8 +124,8 @@ module.exports = function toSVG(gd, format) {
s = svgTextUtils.html_entity_decode(s);
s = svgTextUtils.xml_entity_encode(s);

// Fix quotations around font strings
s = s.replace(/("TOBESTRIPPED)|(TOBESTRIPPED")/g, '\'');
// Fix quotations around font strings and gradient URLs
s = s.replace(DUMMY_REGEX, '\'');

return s;
};
54 changes: 51 additions & 3 deletions test/jasmine/tests/snapshot_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ var Plotly = require('@lib/index');
var d3 = require('d3');
var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var fail = require('../assets/fail_test');

var subplotMock = require('../../image/mocks/multiple_subplots.json');
var annotationMock = require('../../image/mocks/annotations.json');
Expand Down Expand Up @@ -191,16 +192,15 @@ describe('Plotly.Snapshot', function() {
});

describe('toSVG', function() {
var parser = new DOMParser(),
gd;
var parser = new DOMParser();
var gd;

beforeEach(function() {
gd = createGraphDiv();
});

afterEach(destroyGraphDiv);


it('should not return any nested svg tags of plots', function(done) {
Plotly.plot(gd, subplotMock.data, subplotMock.layout).then(function() {
return Plotly.Snapshot.toSVG(gd);
Expand Down Expand Up @@ -245,5 +245,53 @@ describe('Plotly.Snapshot', function() {
done();
});
});

it('should handle quoted style properties', function(done) {
Plotly.plot(gd, [{
y: [1, 2, 1],
marker: {
gradient: {
type: 'radial',
color: '#fff'
},
color: ['red', 'blue', 'green']
}
}], {
font: { family: 'Times New Roman' }
})
.then(function() {
d3.selectAll('text').each(function() {
var tx = d3.select(this);
expect(tx.style('font-family')).toEqual('\"Times New Roman\"');
});

d3.selectAll('.point').each(function() {
var pt = d3.select(this);
expect(pt.style('fill').substr(0, 6)).toEqual('url(\"#');
});

return Plotly.Snapshot.toSVG(gd);
})
.then(function(svg) {
var svgDOM = parser.parseFromString(svg, 'image/svg+xml');
var i;

var textElements = svgDOM.getElementsByTagName('text');
expect(textElements.length).toEqual(11);

for(i = 0; i < textElements.length; i++) {
expect(textElements[i].style['font-family']).toEqual('\"Times New Roman\"');
}

var pointElements = svgDOM.getElementsByClassName('point');
expect(pointElements.length).toEqual(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this patch, pointElement.length was 0 i.e. Snapshot.toSVG failed to process them.


for(i = 0; i < pointElements.length; i++) {
expect(pointElements[i].style.fill.substr(0, 6)).toEqual('url(\"#');
}
})
.catch(fail)
.then(done);
});
});
});