Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

IE11 Bug? Network: Using svg as "image" doesn't work in all zoom levels #3356

Closed
wildsurf opened this issue Aug 9, 2017 · 23 comments
Closed

Comments

@wildsurf
Copy link

wildsurf commented Aug 9, 2017

There seems to be a bug in IE11 when using the network with shape 'image' and svgs.
In IE11, the image is not visible initially, only after zooming out to a very high zoom level do the images pop up.

Here is a fiddle underlining the problem:
https://jsfiddle.net/zzhb8wkb/7/

Any ideas?

@wimrijnders
Copy link
Contributor

This looks like the IE image interpolation problem. See #3297 for a quick fix with options.

@wildsurf
Copy link
Author

wildsurf commented Aug 9, 2017

Thanks for the quick reply! I added shapeProperties.interpolation: false, but the consequence is: no svgs show up at any zoom level.

https://jsfiddle.net/zzhb8wkb/26/

(I don't see any console errors, just the images not showing up.)

@justinharrell
Copy link
Contributor

justinharrell commented Sep 19, 2017

I have a fix for this and #3297 here: justinharrell@c9edcbe

There is the issue where drawImage wants whole numbers which causes IndexSizeError and the ImageOk check which was added that check for naturalWidth which is zero in IE on SVG's.

Not sure if there is a better way or more to it, let me know and I can make a PR.

Interpolation seems to work fine on IE with this as well.

@wimrijnders
Copy link
Contributor

@justinharrell Bloody excellent! Sincerely thanks for looking into it. Interpolation issues on IE/Edge have been the order of the day, I would love a solution for this.

Please post this as a PR, it will be accepted with great pleasure. The only minor qualm I have about it is the IEFix variable, is it necessary? Looks to me like your fix will work on any browser.

@justinharrell
Copy link
Contributor

The IEFix variable is needed to tag the image as being fixed up by _fixImageCoordinates function which sets height and width when zero but can't set naturalWidth. I could call it something else and will be sure to comment it up some.

I wasn't sure of that was the best way to deal with it but it did seem like the most focused to only a special exemption for fixed up images. ImageOk looks relatively new, I guess you went away from image onerror event to that code, but the image.complete isn't consistent between browsers and naturalWidth doesn't work for svg images in IE.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 20, 2017

If you can motivate it (which you just did), it can stay in. Commenting is always good. I think the code looks perfectly fine as it is.

onError still gets used; the ImageCache is filled after an image is successfully loaded. If you know of any other browser inconsistencies that will throw a spanner into the works, please share! Specifically:

image.complete isn't consistent between browsers and naturalWidth doesn't work for svg images in IE

I suppose this should be tested to exhaustion; there will likely be combinations that break down. I haven't been able to test win stuff because I didn't have a win machine on my desk - now I do, I dragged it in between my openSuse and Debians kicking and screaming, specifically for the reason of testing platform dependent vis bugs. It's a wimpy little machine though and complains often on installing and attempted upgrades.

Perhaps you can help me with this; what is the stuff that should be verified for all possible platform + browser combinations? 'Platform' here meaning any platform that is remotely relevant, not just linux and win.

@justinharrell
Copy link
Contributor

justinharrell commented Sep 20, 2017

I will try and look at it a little deeper but if the image isn't even drawn until you get a successful load then I am wondering what the _isImageOk check is for when interpolation is off? It would definitely be simpler to remove that check which would remove the need for the IEFix variable. It seems _isImageOk was added with the CacheImage refactor.

image.complete shows true on FF even if the image had an error loading, which is really the spec, its complete with an error, IE shows complete as false if there was an error, which is more intuitive I think, but not really to spec. So you have the secondary naturalWidth check but it suffers with the same zero issue as width does with SVG IE.

My understanding to most reliable way to determine if a image is loaded is to subscribe to the events, which it sounds like you are doing somewhere else, checking properties is problematic. Things are definitely a mess in this area of browser compatibility.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 20, 2017

then I am wondering what the _isImageOk check is for when interpolation is off?

The image cache is not even used when interpolation is off, so _isImageOk is moot then. It appears to be yet an other visible manifestation of my code paranoia.

... is to subscribe to the events, which it sounds like you[*] are doing somewhere else,

Correct,

Anything you can do or suggest to get the compatibility better is more than welcome, really.


[*] This is actually a difficult word to silently concede to. Many people have worked on this code and so I find it hard to call it 'mine'. I just happen to be the person currently having an interest in maintaining it.

@justinharrell
Copy link
Contributor

Ah ok, I looked right over those handlers, I will remove the _isImageOk in my branch, I like that even better.

We are primarily windows with Chrome and IE which is what prompted me to look into this after upgrading from 4.18 which was working fine to 4.20. So we have no problem helping out with IE compatibility.

I also have some performance improvements for the label module pushed in my branch which seems to help with the jank in IE reported here: #3425 but there is definitely some deeper going on here, however it helps on all browsers too, I based it on profiling in Chrome. Label could definitely be optimized further by caching the rendered label and reusing if scale factor doesn't change since fillText is so slow.

@wimrijnders
Copy link
Contributor

Great! I truly appreciate any effort you care to expend. Usually it's just me wrestling with the code.

Label is something which is keeping me very occupied right now; too much stuff going on there, and sometimes off the mark - e.g. #3464 is grabbing a lot of mindshare from me. So as with previous, help is really welcome here (I'm think I'm beginning to sound desperate).

Just a fair warning, Label is in a state of flux so it may not be obvious how to integrate anything into current. Other than that - bring it on!

@justinharrell
Copy link
Contributor

Yes I did notice significant changes going on in the develop branch for label, that and some other issues in develop is keeping me on master for now. I will try to merge those changes into the develop changes at some point.

@wimrijnders
Copy link
Contributor

Yes I did notice significant changes going on in the develop branch for label....

More on the way. I'm refactoring the shi daylights out of it, especially for better understanding.

...some other issues in develop is keeping me on master for now

Meaning that these issues somehow impede your progress? Such as bugs that can't be overcome?

@justinharrell
Copy link
Contributor

Meaning that these issues somehow impede your progress? Such as bugs that can't be overcome?

My network wouldn't load :), looks like there may be an issue with an empty edge dataset, I start the network with empty datasets then add data to them after initialization. There seemed to be a change to a for in on a array that was failing. I want to take another look so I could get back on develop, just haven't had a chance.

@wimrijnders
Copy link
Contributor

Smells like an issue to me. Care to make a minimal example to demonstrate this?

@wimrijnders
Copy link
Contributor

I start the network with empty datasets then add data to them after initialization.

Aha. Yes, the code was, um, 'lacking' some features in that respect. I did an effort to get the dynamic updating up to scratch, see #3330, #3399. I'm not hoping for it, but it wouldn't surprise me if something was still missing. Or worse, that given PR's introduced other issues. So please give the example so I can check.

@justinharrell
Copy link
Contributor

justinharrell commented Sep 20, 2017

So I took a look again, this is an interesting issue, could not reproduce using the test pages, realized our common app framework is adding to the Array.prototype which causes problems with for in in Clustering.js. I would recommend doing a normal for or maybe forEach (could be slower) on arrays for this issue, for in is iterating properties of the array. See: https://stackoverflow.com/questions/500504/why-is-using-for-in-with-array-iteration-a-bad-idea

To reproduce simply add Array.prototype.foo = 1; before network initialization.

@wimrijnders
Copy link
Contributor

I did that and I watched basicUsage burn. Holy crap. Why you no issue? I'm opening an issue on this right away, credits to you of course.

@justinharrell
Copy link
Contributor

Yes sorry was focused on the SVG IE issue so I just switched to master when I encountered this one :).

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 22, 2017

@justinharrell My latest big change on Label has been accepted and merged (#3470); feel free to add your improvements! Also, please do PR the fix on ImageCache. Thanks!

@justinharrell
Copy link
Contributor

Will do, I was waiting for the for in fix so I could do my PR against develop now I see that is fixed as well. Will take a look at label again, my changes made a visible improvement (less stutter) to IE performance and where pretty minor, but may not apply anymore depending on your changes.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 22, 2017

Hokay. That fix has been accepted as well. Stutter in IE you say? Is there any relation to #3425?

Would like to hear if the for-in issue has been resolved!


Oh, the changes definitely apply. I haven't done any optimization there, just a bug fix and refactoring.

@justinharrell
Copy link
Contributor

Is there any relation to #3425?

Yes mentioned here a couple days ago, it does not solve the stabilized issue but seems to improve the situation some. It may simply be IE is slower at drawing text, again if the rendered label could be cached and simply blitted if the scale hasn't changed I think it might solve the issue completely but will take more investigation to see if thats the solution.

@justinharrell
Copy link
Contributor

Would like to hear if the for-in issue has been resolved!

It is resolved, thanks!

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

No branches or pull requests

4 participants