Skip to content

Fix RE used to correct quotes used in URLs for IE. #1740

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.

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
Jun 2, 2017

Conversation

bobisch
Copy link
Contributor

@bobisch bobisch commented May 28, 2017

The svgToImg() function tries to normalize the SVG for compatibility
with IE. However, currently it use a regular expression that is too
greedy when matching the attribute values that need quotes changed from
single to double. The result is that invalid XML is generated and will
cause IE11 to fail with a XML 5607 error and svgToImg() will never
return. Edge will not complain but will receive invalid XML, which
causes problems when used.

This change restricts matching to only data surrounded by single quotes.

Seems like a jasmine test should be added for svgToImg() along the
lines of

it('Returns valid XML',...)

Also addresses: https://community.plot.ly/t/plotly-toimage-xml5607-error-in-ie11/4223

The `svgToImg()` function tries to normalize the SVG for compatibility
with IE.  However, currently it use a regular expression that is too
greedy when matching the attribute values that need quotes changed from
single to double.  The result is that invalid XML is generated and will
cause IE11 to fail with a `XML 5607` error and `svgToImg()` will never
return.  Edge will not complain but will receive invalid XML, which
causes problems when used.

This change restricts matching to only data surrounded by single quotes.

Seems like a jasmine test should be added for `svgToImg()` along the
lines of

  `it('Returns valid XML',...)`
@etpinard etpinard added status: in progress bug something broken labels May 29, 2017
@etpinard
Copy link
Contributor

Thanks very much for the PR!

That might be enough to fix #699

@etpinard
Copy link
Contributor

etpinard commented May 29, 2017

@bobisch I'm having problem replicating the problem.

@bobisch
Copy link
Contributor Author

bobisch commented May 31, 2017

@etpinard - sorry, I thought we had some relevant examples, but I guess none highlight this particular issue. Unfortunately, I don't think this will do too much to really fix #699 and related issues since those probably have more to do with the IE limitations caused by tainting svg canvas data. But, this will make it possible to use some work-around(s) such as sending the data to the back-end for rendering or using canvg.

Anyway, to get the error you need to have something that produces at least two clip-path="url(... attributes. In your example I think it works to just add another curve to the plot - see https://codepen.io/bobisch/pen/JNgXrE and that fails under Edge (or IE) without the patch.

@etpinard
Copy link
Contributor

@bobisch thanks for the info!

Bundling up your patch and using it in https://codepen.io/etpinard/pen/WjqNLE gives the correct in IE11:

image

🎉🎉🎉🎉🎉🎉🎉


About adding a new test case, I can't find a way to test this logic outside of IE. Mocking the test environment such that it executes this normally IE-only block in svgtoimg.js yields a string that Chrome's DOMParser can't parse as SVG. We could test the raw string output, but that's too brittle for my tastes.

What we should really do is implement #431 - so I'd vote for not adding any tests here, merge this in and include it in 1.28.0.

Sounds good?

@bobisch
Copy link
Contributor Author

bobisch commented Jun 2, 2017

That seems reasonable to me. Thanks!

Wouldn't it still be reasonable to just add a test that checks to see if svgToImg() at least returns a string and that should be valid (parse-able) XML in any browser? That would catch issues with this code paths mangling of the SVG at least.

@etpinard
Copy link
Contributor

etpinard commented Jun 2, 2017

be valid (parse-able) XML in any browser?

That's the problem. XML parsers are different from browser to browser. A string that might be considered valid in one browser won't necessary be valid in another.

@etpinard etpinard merged commit bc0e06d into plotly:master Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants