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.

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

Fix toSVG for graphs that call Drawing.gradient #2914

Merged
merged 5 commits into from
Aug 16, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Aug 16, 2018

This PR fixes toSVG (and hence toImage and downloadImage) for graphs with filled colorbars (from #2910) and contour legend items (added in #2891) in "newer" browsers.

These bugs are related to double-nested " inside svg node style - which browsers have a hard time serializing (cc #1697). Why were they unnoticed in their respective PRs: our old nw.js image-server doesn't care about those double-nested ".

What saved us from releasing these bugs in 1.40.0 is ./tasks/noci_test.sh which test mapbox and cone mocks using orca (which relies on a much newer version of Chromium) where the mocks with colorbars failed to export.

Now, this PR offers two solutions:

The second solution seems to work well, and it leads to 🔪 a fairly big hacky block in toSVG. Thoughts?

- so that we don't have to fiddle with quotes inside of quotes
  in Snapshot.toSVG
- clear corresponding 'style' setting to avoid potential conflicts
- adapt toSVG tests
@etpinard etpinard added bug something broken status: reviewable labels Aug 16, 2018
@etpinard etpinard added this to the v1.40.0 milestone Aug 16, 2018
@alexcjohnson
Copy link
Collaborator

The first solution (extending the toSVG patch) is certainly the smaller conceptual change. Switching to attributes is certainly appealing though - and we have at least one issue #2355 encouraging us to switch whole hog. My main concern about this is how easy it is to (accidentally) override attributes with css. I worry that we will

  • get lots of users complaining that plotly.js doesn't work in their page/app because other css (which they may or may not control but anyway are not expecting will impact plots) is interfering with the plot.
  • get users intentionally overriding plot styling with css, thereby breaking portability of these plots

So I guess in the end I'd vote for the toSVG patch, at least until we have a serious conversation / investigation about attributes.

@etpinard
Copy link
Contributor Author

So I guess in the end I'd vote for the toSVG patch, at least until we have a serious conversation / investigation about attributes.

Sounds good. I'll revert 1c8017e so it will remain in git history in case we change our mind later.

@alexcjohnson
Copy link
Collaborator

I'll revert 1c8017e so it will remain in git history in case we change our mind later.

Good call. 💃

@etpinard etpinard merged commit fb9e903 into master Aug 16, 2018
@etpinard etpinard deleted the fix-linearGradient-tosvg branch August 16, 2018 19:09
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