Skip to content

Support non-SVG snapshots when using MS Edge #2

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

radhikalism
Copy link
Owner

Allows non-SVG snapshots when using MS Edge (but not IE).

Adds:

  • special handling of base64-encoded image data when constructing the blob in filesaver.js
  • try/catch around canvas.toDataURL invocation in svgtoimg.js and specially handles SecurityError (only anticipated in IE but not Edge); also catches generic errors

Removes:

  • IE non-SVG format guard in download.js
  • IE non-SVG format guard in svgtoimg.js

@radhikalism radhikalism force-pushed the edge-non-svg-snapshots branch from be2a09a to ee427b1 Compare October 11, 2017 12:40
@alexcjohnson
Copy link
Collaborator

This looks very good! Can we also push it up to the modebar download button? I'm thinking remove this block and instead in the catch look for the IE error we just threw and try again with svg? The downside of that approach is it will make modebar button downloads take longer on IE, because you have to do a lot of the work twice.

I think that may be acceptable, but if we wanted to avoid it we could either a) look for a quick way to trigger that same SecurityError, or b) give the modebar button a tighter definition of IE than the current Lib.isIE that just looks for the presence of msSaveBlob.

Once this includes the modebar button, feel free to make a PR to the main plotly.js repo.

@radhikalism
Copy link
Owner Author

radhikalism commented Oct 11, 2017

D'oh, I hadn't tested the download button yet as I'm using a custom button directly invoking downloadImage.

Doing the work twice is not likely to be a huge penalty in time performance, but I wonder if the statefulness will be tricky to manage -- in order to avoid a "Snapshotting in progress..." situation on the second attempt. I'll try it and see.

Option A seems like a better approach over browser sniffing. It might be worth adding a predicate to Lib such as canExportFromCanvas(mimeType) that quickly constructs a scratch canvas off-screen, draws a trivial graphic and tries toDataURL(mimeType) on it. It would save us entering the whole download machinery and would test only the relevant feature rather than browser.

@alexcjohnson
Copy link
Collaborator

D'oh, I hadn't tested the download button yet as I'm using a custom button directly invoking downloadImage.

No worries, that's what reviews are for! Anyway the downloadImage part is the more important (and harder) part, the button is just the interface.

It might be worth adding a predicate to Lib such as canExportFromCanvas(mimeType) that quickly constructs a scratch canvas off-screen, draws a trivial graphic and tries toDataURL(mimeType) on it.

If that's easy to do, then absolutely, sounds like the best route!

@etpinard
Copy link

Quick question: does Plotly.toImage work in Edge?

@@ -18,20 +18,6 @@ function svgToImg(opts) {
var Image = window.Image;
var svg = opts.svg;
var format = opts.format || 'png';

// IE only support svg
if(Lib.isIE() && format !== 'svg') {
Copy link

@etpinard etpinard Oct 12, 2017

Choose a reason for hiding this comment

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

So this means Lib.isIE returns true in Edge? Is there an easy way to fix that? We could add an Lib.isMSbrowser if ever we need IE+Edge logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd frankly vote to go for explicit feature detection - rename the current Lib.isIE to something like usesMsSaveBlob, and detect other stuff as needed like @arbscht's suggested canExportFromCanvas.

image export is currently the only place we use Lib.isIE anyway.

Choose a reason for hiding this comment

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

Looks like isIE is only used in for image/download purposes:

image

so yeah, explicit detection would be best.

@mitchellpowell
Copy link

Was any more headway ever made on this issue? Would be nice to export non-SVG for Edge browser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants