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 IE rendering of SVG as overlay image #4324

Merged
merged 7 commits into from
Sep 29, 2017
Merged

Conversation

WietseWind
Copy link
Contributor

IE11 (and sometimes IE10, timing issue) won't "in memory render" SVG images when the source is a data:... URI, if the img-element isn't present and visible in the DOM. Even when appending the IMG to the DOM with display: none or something like that, the img will be 0x0 px.

This ugly-ass fix will create a div at -1px -1px (absolute) and append the img in this div. After this, de callback will be able to get the image contents.

You can reproduce the problem in IE11 by using a data:... URI to use a SVG for fabric.Image.fromURL (to use it as an overlay with canvas.setOverlayImage).

With this fix, this works fine :)

@asturur
Copy link
Member

asturur commented Sep 19, 2017

Could you provide a failing fiddle to run with ie11/ie10?

i m sort of ok with the fix, i would just make it a member of fabric.util
sort of fabric.util.loadImageInDom or similar.

Also i would target it just at SVGs and not for each dataurl.

@WietseWind
Copy link
Contributor Author

@asturur Here you go:
Fixed: https://canvas-overlay2-ijnumrunoy.now.sh/
Pre-fix: https://canvas-overlay2-akdabjqzoo.now.sh/

You might have to refresh a few times (the Pre-fix version), because it is random (timing?)

I'll refactor my code a bit as you suggested and update the PR.

@WietseWind
Copy link
Contributor Author

✅ Refactor to fabric.util.loadImageInDom
✅ Added DOM cleanup (remove added DOM element after calling the callback)

@asturur
Copy link
Member

asturur commented Sep 19, 2017

What is the difference between appending on the onload event, and make something more slow peaced like, detect if is an dataurl svg, append to dom, then set the source, and then in the onload event ( and in the onerror ) remove from dom ?
This would fix the require for a setTimeout of 1ms that does not give us any guarantee that is the right timing.

And probably would require another way to remove it from dom, maybe returning the div node reference from the method that appends it to body.

I have do not time now to test everything ( node included ) so take your time to think of it, i m completely ok with this fix.
This could maybe also help the SVGs that have no width height or viewbox in firefox.

@asturur
Copy link
Member

asturur commented Sep 19, 2017

#2945

@WietseWind
Copy link
Contributor Author

@asturur The 1ms is enough, since IE somehow only requires the DOM element to be present and visible (outside of the viewport = fine). I tried the cleanup without the setTimeout and this causes timing issues (callback evals before dom-render or something like that...), while 1ms is enough. Tested it on PC, virtualBox with MS test-machines, slow CPU, fast CPU, etc.

I don't know exactly why, but this the (a) fix in IE11. There may be a cleaner approach (for instance: without a setTimeout), but I've spent 2 days on this issue and I didn't find one :(

👍

@WietseWind
Copy link
Contributor Author

P.S. This was the problem in IE too; this PR fixes it :) #2945 (comment)

@asturur
Copy link
Member

asturur commented Sep 19, 2017

i mean something:

     loadImage: function(url, callback, context, crossOrigin) {
       if (!url) {
         callback && callback.call(context, url);
         return;
       }
 
       var img = fabric.util.createImage();
       var domNode;
        if (url.substring(0,14) === 'data:image/svg') {
          // IE10 / IE11-Fix: SVG contents from data: URI
          // will only be available if the IMG is present
          // in the DOM (and visible)
          domNode = fabric.util.loadImageInDom(img);
        }        /** @ignore */
        img.onload = function () {
           if (domNode) {
             // remove node from dom
           }
         callback && callback.call(context, img);
         img = img.onload = img.onerror = null;
        };

Sort of letting the image in the dom before the load process start, seems more natural to me. And could give enough time to render it, unless for ie11 the load is just pull the string in memory without render.

@WietseWind
Copy link
Contributor Author

Nice one. I will test it.

@WietseWind
Copy link
Contributor Author

@asturur Tried it, doesn't work :( If the callback is called after creating the DOM element, IE11 still breaks. If the callback happens after a 1ms delay , IE's fine with it 🙄

However, I did move some logic around, this is a bit cleaner; all DOM related stuff is in loadImageInDom. The onload-callback is set to null and set again in the loadImageInDom method, wrapped in a 1ms delay. Ugly, but it fixes the 0x0 dimensions (and unavailable SVG image) in IE10, IE11 + FF.

@asturur
Copy link
Member

asturur commented Sep 19, 2017

i guess if avoid setting the img to null ( pointless since GC would take care of it anyway, could solve the 1ms issue.

@WietseWind
Copy link
Contributor Author

@asturur Tried it. Doesn't work :(

@asturur
Copy link
Member

asturur commented Sep 29, 2017

To me the code looks exactly what i wanted, plus some extra function wrapping that are not a problem/blocker.
I'm merging as it is and then i ll try if it continue working if i change it.

@asturur asturur merged commit 052631b into fabricjs:master Sep 29, 2017
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.

2 participants