search

louisremi / background-size-polyfill

Adds support for background-size "cover" and "contain" to IE8
http://louisremi.github.com/background-size-polyfill/
MIT License
1.26k stars 359 forks source link

Breaks IE 9 #37

Open kiezpro opened 10 years ago

kiezpro commented 10 years ago

The polyfill broke IE 9 for me, which I only found out after it would have been a major pain to create a separate stylesheet for browsers requiring the the polyfill. I've written a little patch which resolves the problem by testing for the IE version before applying the event handlers (four dots .... indicate that I've left out the original code here for clarity):

....
var rsrc =  ....
    var re = /MSIE ([0-9\.]+)/;
    var match = re.exec(navigator.userAgent);
    var browserVersion = 0;
    if(match && match.length > 1) {
        browserVersion = parseInt(match[1]);
    }

....
function init() {
    if(browserVersion >= 9) {return true;}
....
function handlePropertychange() {
    if(browserVersion >= 9) {return true;}
....
function handleResize() {
    if(browserVersion >= 9) {return true;}
....
function restore() {
    if(browserVersion >= 9) {return true;}
....

The patch should work for IE < 7, but I haven't tested it with versions below 7.

jefferyto commented 10 years ago

Sorry if the polyfill has caused you trouble - it was intended to save time and headache :smile:

It would be best if the polyfill was only applied for IE < 9 users (so that IE >= 9 users aren't penalized by having to download an extra file they don't need), but it shouldn't cause any errors in IE 9 either. Can you include more details about the error you encountered? A link or jsFiddle demonstrating the issue would be best.

kiezpro commented 10 years ago

Thanks for getting back to me! As you already mentioned, it would have been best to not use the Polyfill for IE >= 9 anyway, so I guess the problem is my own fault :) I don't have time to set up a jsFiddle right now, and the site isn't live yet, so I don't have a link. What happened with IE 9 was that it didn't scale the images down but showed them in their original size. As I've mentioned, I've created a little quick-and-dirty workaround, so my report was rather a heads-up than a real bug report. I've seen that you've picked up on that and created an issue yourself, thanks for that! A feature detection would of course be a lot more elegant than my brute-force browser version testing. I'll see whether I can reproduce the problem in a jsFiddle, but that probably won't happen before next week.

litchfield commented 10 years ago

Yeah FYI, it caused my IE9 to hang and crash completely. Was a tricky one to debug!

designorant commented 10 years ago

I confirm that running the polyfill gives some unexpected results in IE9.

Since this is not needed for IE9 at all, a quick workaround would be to use conditional comments:

<!doctype html>
<!--[if lte IE 8]>     <html class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html class="no-js"> <!--<![endif]-->

and then to make use of that extra lt-ie9 class:

selector { 
    background-size: cover;
}
.lt-ie9 selector { 
    -ms-behavior: url(/backgroundsize.min.htc);
}

Anyway, having the script conditional by default would surely save someone a few head scratches.

MiChAeLoKGB commented 9 years ago

Hmmm, after all this time and still was not fixed. Is there some way, to actually use it only if user has IE <= 8 ? As it breaks my site in IE 9 and I hoped its conditional for IE 8 and older...

The analytics I have from multiple of my sites show me, that actually only about 7% of my users user IE and only 2,2% of them use IE 8, 7 or 6, but still, some fallback is nice. And as this should be an easy way to fix it, I tought, well, why not. But I would certainly not create separate CSS files for such small audience.

Greg-J commented 9 years ago

@MiChAeLoKGB honestly, you shouldn't be trying to use it in IE9 at all. Use the method that @designorant has posted as checking for version should be done in the html and not in the JavaScript for reasons too numerous to list.