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

Picture element not fetching correct source (Chrome, Firefox) #87

Closed
toddmilliken opened this issue Apr 24, 2017 · 17 comments
Closed

Picture element not fetching correct source (Chrome, Firefox) #87

toddmilliken opened this issue Apr 24, 2017 · 17 comments

Comments

@toddmilliken
Copy link

toddmilliken commented Apr 24, 2017

On Chrome and Firefox, only the image on the <img /> tag's data-original attribute is loaded. The source element's data-original-set attributes are ignored.

I tested the demo and verified no change is occurring.
https://verlok.github.io/lazyload/demos/with_picture.html#/it/donna/stivaletti_cod44721746jj.html

I also put together a codepen and can verify its not change
https://codepen.io/emagine/pen/wdzyXq

I'm producing the issue on macOS 10.12.4 (Sierra) using Chrome 58 and Firefox 52.0.2.

Safari 10.1 is working, and Microsoft Edge seems to work when I loaded it up on browserstack.com

@toddmilliken
Copy link
Author

I think the fix is that we need to camelCase the data attribute before running it thru pictureChild.dataset[srcsetDataAttribute] on lazyload.setSources.js line 9.

See https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset#Name_conversion

I'll try and submit a pull request today or tomorrow.

@verlok
Copy link
Owner

verlok commented Apr 25, 2017

Thanks @toddmilliken,
this must be due to the recent switch to the use of datasets.
Today is a holiday here and I'm far away from any coding device :) but I'll merge your PR in if you make it today.
Please use a branch named hotfix/sources to code the fix in, and remember to create the dist folder files and bump the version by 0.0.1

@toddmilliken
Copy link
Author

Ok no problem.

I was thinking of adding a helper function to lazyload.setSources that will take a data attribute input and return the camelcase version. I'm able to get it working but am stuck on building the es6 minified version in the grunt uglify task.

When I run grunt it seems to choke on uglifying the ES6 code on line 73 of lazyload.js. Is there a special work around you have to get this to work? gruntjs/grunt-contrib-uglify#367

@verlok
Copy link
Owner

verlok commented Apr 25, 2017

Wait, what is the problem to be solved exactly? Why can't the final user already put the option in camel case instead of having the script to convert it?

@toddmilliken
Copy link
Author

The issue is that the default attribute data-original-set is being using like this on lazyload.setSources.js on line 9:
pictureChild.dataset['original-set']

This is invalid because dataset accesses the attributes in camelcase form. It needs to be rewritten as:
pictureChild.dataset['originalSet']

It looks like there is an option ( data_srcset ) for users to customize the data attribute on source tags to be whatever they want it to be - so we need to make sure there is a way to retrieve that attribute dynamically in its camelcase form.

The pull request fixes the plugin out of the box by converting orginal-set to originalSet; and it also fixes use-cases where the developer has overwritten the data attribute using the option data_srcset to be something else. This is why I felt the need to add a helper function that will take a string and return it in camelcase form.

This article does a much better job of explaining than I can:
https://www.sitepoint.com/managing-custom-data-html5-dataset-api/

Hope that makes sense.

@verlok
Copy link
Owner

verlok commented Apr 25, 2017

Thanks,
I understood what is happening, but isn't enough to change the default data_srcset option to originalSet?

@toddmilliken
Copy link
Author

If I understand correctly, you're asking if changing the default attributes in lazyload.defaults.js from:
data_srcset: "original-set"
to
data_srcset: "originalSet"

Is that correct? If that is the case, yes -- it would resolve partially but would have two side effects:

  1. HTML5 data attributes are case sensitive and cannot contain capital letters as noted by w3c https://www.w3.org/TR/2011/WD-html5-20110525/elements.html#embedding-custom-non-visible-data-with-the-data-attributes .

  2. This would only work using the default data_srcset option set to originalSet.
    If a developer passed in their own data_srcset parameter that was a multi-word phrase such as my-data-src, the script would read literally pictureChild.dataset["my-data-src"] which will not work in Chrome / Firefox and brings us back to the same issue.

I'm not sure I can think of another way to solve this issue other than using the helper function to take whatever option is passed to data_srcset and return the camelcase version. I'm open to suggestions though.

@verlok
Copy link
Owner

verlok commented Apr 25, 2017

Yes,
that's exactly what I'm suggesting.
It's because the dataset JavaScript API works exactly like that: if you have a data-my-data-src in your HTML, it would be accessible by JavaScript by element.dataset.myDataSrc.
So changing the default option to originalSrc would do the trick. For developers wanting to use a hyphenated data attribute, we could add a case in the README.

@toddmilliken
Copy link
Author

Ok I think I understand.

It seems a little odd that if a developer wants to override it, that they are forced to have an invalid HTML5 document using a camelcase data attribute on their source tags like <source data-myDataSrc>

It is possible this issue may come up for devs not intimate with the plugin; although it seems like an edge-case.

Any specific reason why we shouldn't use the helper function? It will naturally fix all of the above scenarios without devs having to dig through the readME or being forced to have invalid HTML

@verlok
Copy link
Owner

verlok commented Apr 26, 2017

Hi @toddmilliken,

It seems a little odd that if a developer wants to override it, that they are forced to have an invalid HTML5 document using a camelcase data attribute on their source tags like <source data-myDataSrc>

It isn't forced to do that. The HTML would still be:

<picture>
<source data-my-data-src="...">
<img data-src="..."
</picture>

And the js in that case would be:

new LazyLoad({ data_srcset = "myDataSrc" });

That's because JS natively does the conversion of any "data-*" attribute to camelcase inside the element.dataset property.

Check it out on this pen.

Any specific reason why we shouldn't use the helper function? It will naturally fix all of the above scenarios without devs having to dig through the readME or being forced to have invalid HTML

Because it would be replicating what the browser already does. :)

@verlok
Copy link
Owner

verlok commented Apr 26, 2017

PS: I'm trying the fix but I'm also having problems with uglify... :|
This must be the reason: webpack/webpack#2972

@toddmilliken
Copy link
Author

Ah, ok i see what you mean. Devs just need to pass in their custom data attribute already in camelcase form for it to work; it's up to them to enter it in hyphenated lowercase form in their own HTML.

Thanks for explaining that. Let me know if you want me to revise the pull request per your earlier instructions.

Regarding uglify - I googled "online es6 minifier" and found this to minify lazyload.js to lazyload.min.js:
http://prettydiff.com/

Seems to work ok.

@verlok verlok mentioned this issue Apr 26, 2017
@verlok
Copy link
Owner

verlok commented Apr 26, 2017

I think the best thing is to create a new PR.
About the uglifier, I'm almost sure it worked on my Mac, and I don't understand why it's not working on this Windows machine...

@verlok
Copy link
Owner

verlok commented Apr 26, 2017

@toddmilliken I'll create a PR myself, then we can discuss it together.

@verlok
Copy link
Owner

verlok commented Apr 26, 2017

Sorry @toddmilliken I was using a git-flow plugin which automatically pushed the fix on Github.
This issue is then fixed in version 7.2.2

@verlok verlok closed this as completed Apr 26, 2017
@Niresh12495
Copy link

This issue is related to
#90
I have tested 7.1.0 version of verlok lazyload it is working perfectly but now
7.2.0 == 7.2.1 && 7.2.2 the issue exist in these versions

with element.getAttribute it was working good but now with element.dataset causing the issue check out the video listed below
https://www.youtube.com/watch?v=PsoYIJm6Z9I

@rflmyk
Copy link

rflmyk commented May 23, 2017

I have been a same problem, how I get version 7.2.2? I'm using npm but this version don't over there?
Can help me?

For now I go back to version 7.1.0 how @Niresh12495 said above.

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

No branches or pull requests

4 participants