Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Image reevaluation not working? #512

Closed
timganter opened this issue Jun 7, 2015 · 15 comments
Closed

Image reevaluation not working? #512

timganter opened this issue Jun 7, 2015 · 15 comments

Comments

@timganter
Copy link

Hi All,

I'm not sure if this is a bug or if my expectations for the behavior are incorrect.

This is in Firefox v38.0.5 for Mac.

1.) Resize a web browser window as small as you can.
2.) Go here: http://scottjehl.github.io/picturefill/examples/demo-01.html
3.) Resize your web browser window larger.

I thought that the image would change and load a larger version if the window is resized. Here's an animated gif showing how upper left always says "medium.jpg".

Many thanks,
-Tim

picturefill

@aFarkas
Copy link
Collaborator

aFarkas commented Jun 7, 2015

Read:
https://bugzilla.mozilla.org/show_bug.cgi?id=1135812
https://bugzilla.mozilla.org/show_bug.cgi?id=1166138
ResponsiveImagesCG/picture-element#157

Try this:

/**
 * FF's first picture implementation is static and does not react to viewport changes, this tiny script fixes this.
 */
(function() {
    /*jshint eqnull:true */
    var ua = navigator.userAgent;

    if ( window.HTMLPictureElement && ((/ecko/).test(ua) && ua.match(/rv\:(\d+)/) && RegExp.$1 < 41) ) {
        addEventListener("resize", (function() {
            var timer;

            var dummySrc = document.createElement("source");

            var fixRespimg = function(img) {
                var source, sizes;
                var picture = img.parentNode;

                if(picture.nodeName.toUpperCase() == 'PICTURE'){
                    source = dummySrc.cloneNode();

                    picture.insertBefore(source, picture.firstElementChild);
                    setTimeout(function() {
                        picture.removeChild(source);
                    });
                } else if(!img._pfLastSize || img.offsetWidth > img._pfLastSize) {
                    img._pfLastSize = img.offsetWidth;
                    sizes = img.sizes;
                    img.sizes += ',100vw';
                    setTimeout(function(){
                        img.sizes = sizes;
                    });
                }
            };

            var findPictureImgs = function() {
                var i;
                var imgs = document.querySelectorAll("picture > img, img[srcset][sizes]");
                for (i = 0; i < imgs.length; i++) {
                    if (imgs[i].currentSrc && !imgs[i].complete) {
                        removeEventListener("resize", onResize);
                        break;
                    }
                    fixRespimg(imgs[i]);
                }
            };
            var onResize = function() {
                clearTimeout(timer);
                timer = setTimeout(findPictureImgs, 99);
            };

            dummySrc.srcset = "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==";

            return onResize;
        })());
    }
})();

an updated version of:
https://github.com/scottjehl/picturefill/blob/afarkas-pf3-proposal/src/plugins/gecko-picture/pf.gecko-picture.js

@timganter
Copy link
Author

Oh wow! Thanks @aFarkas. That totally worked for me. I'm glad I asked! =)

@albell
Copy link
Collaborator

albell commented Jun 9, 2015

@aFarkas @Wilto @mike-engel Should we discuss the possibility of putting this into core? Firefox shipping without handling resize natively is pretty painful. I have a feeling this is going to come up over and over again. I'm unclear from the release notes if they are going to force the patch or if it's going to follow the usual trickledown process. Meanwhile, even if authors don't notice it's broken, we should maybe still fix it. We can could refine the UA test when we know more. Correct resize behavior seems like it's in the wheelhouse for a polyfill.

@Wilto
Copy link
Collaborator

Wilto commented Jun 9, 2015

It also looks like Mozilla is punting their fix to FF 40 now, which is changing my mind on including this by default: https://bugzilla.mozilla.org/show_bug.cgi?id=1135812#c53 I would still want it as an easily-detached “plugin” kind of thing, just to facilitate removal eventually.

@mike-engel
Copy link
Collaborator

Agree with putting it in core as a plugin. Is there any way to detect support for this? I'm guessing not.

@mike-engel mike-engel reopened this Jun 9, 2015
@Wilto
Copy link
Collaborator

Wilto commented Jun 9, 2015

That’s tough. I could see cobbling together a test for it, but not without more overhead than it’s probably worth. This feels like it lands squarely in UA-sniff territory, with a detailed comment pointing to the bugzilla thread.

@aFarkas
Copy link
Collaborator

aFarkas commented Jun 9, 2015

There is already some kind of a detect in this code:

if (imgs[i].currentSrc && !imgs[i].complete) {
    removeEventListener("resize", onResize);
    break;
}

@mike-engel
Copy link
Collaborator

So then if we're having the problem of resizing with FF, is that code running prematurely/falsely?

@mike-engel
Copy link
Collaborator

Alright, from the slack channel, consensus seems to be to include the gecko-picture plugin with pf by default for a while until we're comfortable FF38 usage is low enough.

@albell
Copy link
Collaborator

albell commented Jun 9, 2015

@aFarkas one optimization to consider before we ship: consider using timestamp for debouncing instead of setTimeout. The perf is better:

http://modernjavascript.blogspot.com/2013/08/building-better-debounce.html

@aFarkas
Copy link
Collaborator

aFarkas commented Jun 9, 2015

good call.

But this is more important then (because it runs in all browsers):
https://github.com/scottjehl/picturefill/blob/afarkas-pf3-proposal/src/picturefill.js#L1379-L1383

Would be great to do it there as well. Maybe use a simplified smaller variation.

@aFarkas
Copy link
Collaborator

aFarkas commented Jun 18, 2015

I'm not so sure wether I still want this in by default. This bug is already happening and we haven't done a quick release for the 2.x branch. We also know, that this bug is already fixed with version 40. So people, who want to fix it, already do this by using the "extension". But if we add it to the core, we might never be able to remove it from the core (at least until next FF ESR update, which is in one year).

What do you think?

@Wilto
Copy link
Collaborator

Wilto commented Jun 18, 2015

I think included by default makes sense—for a while, anyway—but definitely not built into the core itself.

@aFarkas
Copy link
Collaborator

aFarkas commented Jun 18, 2015

What is the difference between included and built into the core. My mind changed about this yesterday, because of our webp merge. I'm just a little bit concerned about filesize. We exceeded the 11kb with this. And actually I wanted to stay under 10kb minified.

@Wilto
Copy link
Collaborator

Wilto commented Jun 18, 2015

Something we build into /dist by default for a while, but still develop in a separate file—that way we’re not deleting lines of code when the time comes, just tweaking the dist task.

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

No branches or pull requests

5 participants