IE/Edge SVG export fixes #2068
IE/Edge SVG export fixes #2068
Conversation
|
@alexcjohnson Thanks, I can confirm this works for me. SVG files are downloaded, valid, and clipped correctly when zoomed. |
|
Thanks @arbscht - and thanks again for the fix that got us most of the way there! I'm going to wait for a review from @etpinard or @bpostlethwaite before merging - which with any luck will NOT come today 🇨🇦 🦃 |
|
As a sidenote, I don't think this really affects #699. This change only fixes a regression to do with SVG files generated in IE/Edge. Downloading PNG files with MS browsers is still not supported in any code path as far as I can see. In fact, That assumption would need to be changed if one were to implement the solution outlined by @etpinard in #699 (comment) but I'd look at revisiting that separately. |
Ah good read - seems that issue morphed after it was originally posted, as the initial report did deal with a regression introduced in v1.13.0 but later on a discussion of possible improvements for Edge specifically was added. Good, I will leave #699 open after this gets merged. |
|
Oops. Let me try that again: I don't think we'll ever support downloading PNGs in IE. But, making this work in Edge should be doable. |
|
So I would either rename #699 or close and open a new issue named: |
| navigator.msSaveBlob(new Blob([url]), name); | ||
| // At this point we are only dealing with a SVG encoded as | ||
| // a data URL (since IE only supports SVG) | ||
| var encoded = url.split(/^data:image\/svg\+xml,/)[1]; |
There was a problem hiding this comment.
I wonder what's better (i.e. faster)
url.split(/^data:image\/svg\+xml,/)[1];
// or as in plot_api/to_image.js
url.replace(/^data:image\/svg\+xml,/, '');There was a problem hiding this comment.
I don't see perf as too important in this block. An advantage (at least it looks like an advantage to me...) of the current one is that if url does NOT start with the expected 'data:image/svg+xml,' it will throw an error, rather than saving garbage, which seems easier to investigate.
There was a problem hiding this comment.
it will throw an error, rather than saving garbage,
Good point. 👍
test/jasmine/tests/download_test.js
Outdated
| }) | ||
| .then(function() { | ||
| expect(savedBlob).toBeDefined(); | ||
| if(savedBlob === undefined) return; |
There was a problem hiding this comment.
Perhaps
if(saveBlob === undefined) {
fail('undefined save blob');
}would be more readable than an expect + early return sequence.
test/jasmine/tests/download_test.js
Outdated
| .replace(/(\(#)([^")]*)(\))/gi, '(\"#$2\")'); | ||
| }; | ||
| var savedBlob; | ||
| navigator.msSaveBlob = function(blob) { savedBlob = blob; }; |
There was a problem hiding this comment.
It would be best to use jasmine spyOn and callFake that way we wouldn't have to worry about restoring the unmocked behavior.
There was a problem hiding this comment.
ha, you can only spyOn something that already exists - so msSaveBlob I have to handle manually. But Lib.isIE and serializeToString (and perhaps document.createElement??) I can switch to spyOn().and.callFake
|
Great PR. Thanks very much @arbscht for bringing this to our attention. You win the community PR of the week 🏆 I gave up on #699 a long time ago. This will make a bunch of people happy 🎉 @alexcjohnson I made a few comments. All of them are non-blocking, if you're pressed for time. |
|
💃 nicely done. |
|
@alexcjohnson I'm not sure if this is the right place to mention this, but I think I encountered a bug for IE SVG export. If you download an SVG with Internet Explorer Version 11.0.9600.19137 that renders via trident engine (Edge) you get this: I'm not sure how to fix this. Is this a very specific problem with this browser running on trident? |
|
@Braintelligence thanks for the report. Certainly sounds like a bug and related to this PR. But we should discuss in a new issue. Can you open one, include the information you just posted - plus a description of how one ends up running this particular browser version - and link to this PR? Thanks! |
Based off radhikalism#1 by @arbscht
Fixes #2063 and possibly #699
@arbscht can you confirm that this produces good output for you - particularly can you verify that a zoomed-in plot is still clipped correctly in the exported SVG?
cc @etpinard @bpostlethwaite