picImg.currentSrc = picImg.src; with 'use strict'. Writing to read only property

[andrew-jg]

In src/picturefill.js, line 458 is:

picImg.currentSrc = picImg.src;  

currentSrc is a read only property. So this line does nothing in all major browsers. The currentSrc is updated when the src is, and that's done the line before. Not only does it do nothing, but since "use strict"; is used, some browsers will throw exceptions for trying to modify a read only property.

If you remove this line nothing will break and you will have improved your browser compatibility. New versions of IE throw the exception and the canary version of Chrome does as well :)

[Wilto]

Huh. It should never get to that line in a browser where currentSrc is natively supported though, yeah?

[andrew-jg]

I'm not too familiar with the code. Where is the check to see if it is supported?

[Wilto]

Oh, Picturefill bails right away if responsive images are natively supported—at which point currentSrc is read-only:

https://github.com/scottjehl/picturefill/blob/master/src/picturefill.js#L26

[aFarkas]

I think currentSrc and srcset is already implemented by IE, while picture and sizes are not. From my viewpoint this currentSrc part in picturefill could also be completely removed.

[aFarkas]

Some information, why I think we should remove currentSrc entirely from picturefill core and maybe add support via plugin.

A heavily simplified currentSrc polyfill workaround, which you can see now more often looks like this:

var src = img.currentSrc || img.src;

The code above is simplified and will only return correct cross browser values, if the img is complete, otherwise values might go wrong.

Well in current picturefill currentSrc is polyfilled the same way, but only for images that are responsive images and needed to be polyfilled (not only think about IE, but also about Safari, which supports srcset but not currentSrc). This means, that a) picturefill has the same problems the code above has and b) although picturefill is used the author still needs to use the pattern above to account for images, that are not processed by picturefill.

I just updated the code inside of the pf3.0 proposal to reflect the needed changes to implement currentSrc in a more useful way. In this branch it is already used inside picturefill plugin.

[andrew-jg]

Right, yeah I understand the problem better now. As Alexander said, in IE currentSrc and srcset are implemented while picture and sizes aren't. I'm on the IE team and we're seeing it break sites. Thanks Alexander for the code proposal.

[Wilto]

Yeah; we’re gonna need to look into a more granular way of handling our support tests—until now, we could safely assume that full srcset support meant picture would be in play.

we knew this day would come

[albell]

Interesting idea to factor out a currentSrc polyfill. But we should weigh if it's useful utility code that we're going to need internally later. And also if messing with the img prototype would have any side effects. If it turns out a lot of the checks around complete bugs and such need to be internal anyway it might make sense to modularize a currentSrc polyfill, but include it in the standard PF build. Thoughts?

[Wilto]

it might make sense to modularize a currentSrc polyfill, but include it in the standard PF build.

Yeah, I dig this idea.

[aFarkas]

I really like the mutation plugin, because as soon as you use it, it starts to make really fun to script responsive images (The polyfilled mutations are actually working better than in current Chrome).

But I'm not so into adding the currentSrc into picturefill core, because of a) filesize and b) I don't think it is useful for us or a developer.

I can say for sure, that we don't need it internally. For a polyfilled browser only img.src is the url pointing to the image. We don't need to know anything else. Also note, that about 40% of the linked code deals with the special case of Safari 7.1-8.x, where srcset with x is supported, but not currentSrc. (I really welcome, that IE does implement the spec in a logical order, although it is punished.) Most other code parts are dealing with the fact, that currentSrc is not updated if a new src is just selected, it also has to be completed, which makes it a little bit more complicated, otherwise an implementation would look as simple as this:

if(!('currentSrc' in document.createElement('img'))) {
    Object.defineProperty(HTMLImageElement.prototype, 'currentSrc', {
        set: function () {
            if (window.console && console.warn) {
                console.warn('currentSrc can\'t be set on img element');
            }
        },
        get: function () {
            return this.src || '';
        },
        enumerable: true,
        configurable: true
    });
}

[andrew-jg]

Thanks for the fix. Is there still some tests/documentation to be done before it can make its way into master?

[gregwhitworth]

Thank you everyone for looking into this. I would greatly appreciate a solution for this to make its way into the trunk of picturefill as this has forced us to remove the currentSrc API from our implementation of srcset in Project Spartan. We have had to make this decision due to the assumption in the design of this library that the lack of means that no building blocks of responsive images is supported.

Additionally I would like to request that when this fix makes its way through testing and into the trunk of polyfill that this library use any available community PR outlets to have sites update to the latest version of picturefill. We will be doing this as well in conjunction with you. Ultimately, we would like to add the API back in as soon as possible but we can't do that until we feel our end users will receive a good web viewing experience.

Again, thank you for your work on this and please let us know if there is anything that we can do to help.

[aFarkas]

@andrew-jg and @gregwhitworth
I currently have no Spartan, something that might help: Could you test/confirm, whether the following tests do pass in Spartan, while some of the following do not pass?

[gregwhitworth]

On the first set: #7 fails (https://cdn.rawgit.com/scottjehl/picturefill/2.4/tests/index.html?notrycatch=true&testNumber=7):

uses the url from the best px fitExpected: "data:300" Result: undefined Diff: "data:300" undefined Source: at Anonymous function (https://cdn.rawgit.com/scottjehl/picturefill/2.4/tests/tests.js:478:3)

On the second set, all 3 fail, the first two are due to assignment of read-only props in strict mode, and the final one is due to picturefill not being defined.

[Wilto]

We’re going to work on getting a 2.3.1 patch out today.

Definitely send us a heads-up if anything stands to trip up native respimg support in Spartan—we had no idea that this was a blocker for you guys.

[gregwhitworth]

Awesome thanks! Yeah, we flighted it out to our insiders and a flood of new sites were submitted that were broken due to this issue. Please let me know when you get the lib fixed and @andrew-jg or I can test it against all of the bugs we have to ensure it works.

[Wilto]

Any way I can get CC’d on issues like that? Your issue tracker is private, yeah?

[gregwhitworth]

Yeah it's private unless it's initially filed in the public via Connect. The second that @andrew-jg found that the issue was due to this library he posted it here; so it's about as good as a cc at this point since those are being triaged and reduced daily.

[Wilto]

Gotcha. We’ll keep an eye out for him!

[Wilto]

Merged, released in 2.3.1, and published to NPM.