Fix toImage for marker gradient #1694
Conversation
- style="fill: url("#...")" break the XML seriliazer,
- we need to swap those inner \" before seriliazing
and bring those back in the SVG after, similar to how
'font-family' properties are handled.
| } | ||
|
|
||
| var pointElements = svgDOM.getElementsByClassName('point'); | ||
| expect(pointElements.length).toEqual(3); |
There was a problem hiding this comment.
Before this patch, pointElement.length was 0 i.e. Snapshot.toSVG failed to process them.
| @@ -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'; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
hmm, looks like this is already a problem in master:
Plotly.relayout(gd,{'yaxis.title':'"TOBESTRIPPED"'})
on screen:
download:
So you're not making a new bug, but we do need something better for this.
There was a problem hiding this comment.
OK. What's the solution then?
- We could make the regex more precise here so that it grabs only
TOBESTRIPPEDinsidestyle=props. - Use another (more esoteric) dummy string
- Use another seriliazer than what's the browsers give us on the
windowobject?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess only style attributes get these extra quotes.
Exactly!
There was a problem hiding this comment.
Issue ➡️ #1697
thanks!
|
💃 |
fixes https://github.com/plotly/support-assets/issues/87
The
Plotly.toImageas well as the to-image modebar button are currently broken for graphs with setmarker.gradient(added in #1620). Note that the image tests work fine inside the image test container - I'm thinking this has to do with nw.js 0.12 which uses a much older Chromium version (we gotta upgrade that thing soon).The fix applies the same logic as in #604. In #604, the
"surroundingfont-familyproperty value (e.g.style="font-family: "Times New Roman"") were stripped before serializing withXMLSerializerand then put back after. In this PR, the"surrounding thefillproperty value for gradients (e.g.style="fill: url("#dgfdakngkda")") are given the same treatment.Note that, we don't have to the same with
clip-pathattributes,XMLSerializerhandles those fine.In future PRs, we should make that all new attributes that generate DOM style properties wrapped in
"go through this process to avoid bugs like this one.@alexcjohnson