Planning for <picture> markup #125

Closed
scottjehl opened this Issue Feb 28, 2014 · 29 comments

Comments

Projects
None yet
8 participants
Owner

scottjehl commented Feb 28, 2014

Now that srcset has landed in browsers and picture is coming soon, it'd be a good time to get Picturefill back to its original goal as a simple polyfill for the standard markup.

This thread can be used for planning that out, as there are a number of things to consider. Here are a few off the top of my head:

  • IE9 (maybe 10 too, can't remember - and desktop and mobile versions) does not recognize source elements if they don't have a video or audio element parent. We'll need to find a way to work around this one - ideally without changing the markup pattern to accommodate, but that may not be possible. Here's how I worked around it in the past (note: gross) https://github.com/scottjehl/picturefill/tree/original-picture-markup
  • I think it'd be good if we refactor a bit to get the code more modular and extensible.
  • I think it might be nice if source[srcset] handling was written as an excludable extension to Picturefill, so people can include it in the build if needed.

Also, we should get this thing running on grunt, updated on Bower, and unit tested.

Woo!

Owner

scottjehl commented Feb 28, 2014

Also, @jansepar has done a great job getting this started here, and it sounds like he's up for helping with this refactor https://github.com/jansepar/picturefill

Contributor

mdrummond commented Feb 28, 2014

I was just doing some refactoring of picturefill on a Drupal install based on the suggestions in this patch. The default Picturefill can get pretty slow if there are a number of images picturefilled in IE9. By reducing the number of get attribute calls and switching to the match-media script in weblinc's polyfill for Picture, I was able to get things running significantly faster. This was the span-based picturefill, but I could try to contribute some of those changes back here if you think it would be worthwhile.

https://drupal.org/comment/7552775#comment-7552775

Owner

scottjehl commented Feb 28, 2014

Quick update:
I revived a branch from the commit just before the project moved to div markup and away from picture. This branch included workarounds for older browsers like IE9 and Android 2, and the readme explains https://github.com/scottjehl/picturefill/tree/original-picture-markup

Contributor

jansepar commented Feb 28, 2014

Yup definitely up for helping. I'll see what I can do over the weekend ;). +1 to tests and bower - I've got tests up and running on my fork.

Collaborator

Wilto commented Feb 28, 2014

nwtn commented Feb 28, 2014

I have a rough implementation of type attribute stuff in https://github.com/nwtn/picturefill/tree/type-attr.

Owner

scottjehl commented Feb 28, 2014

Awesome, @nwtn . Sounds like another feature that'd be nice to build as a module to tie-in?

Collaborator

Wilto commented Feb 28, 2014

+1.

Really dig the idea of having features broken out into modules, too. Thinking we could have the Picturefill core and stand-alone extensions for things like sizes, type, and so on so devs could mix and match the features they need—and of course, an “everything” bundle for kicking the tires.

+1 for modularity. @nwtn Also cool to see some work being done to polyfill the type attribute. :)

nwtn commented Feb 28, 2014

@scottjehl +1 ya modular would be great.

Question: does using the real syntax in picturefill mean img not inside noscript? because double downloads and all that.

Owner

scottjehl commented Feb 28, 2014

Good question. No, I do think it's probably best to stick with noscript around the img element for a good while.

On Feb 28, 2014, at 12:26 PM, David Newton notifications@github.com wrote:

@scottjehl +1 ya modular would be great.

Question: does using the real syntax in picturefill mean img not inside noscript? because double downloads and all that.


Reply to this email directly or view it on GitHub.

nwtn commented Feb 28, 2014

Since the new picture is leveraging the actual <img> element to do all the work, putting it in a noscript tag would make it not work in any UA that actually does support the picture element. So it would defeat the purpose of moving to the real syntax, I think. I don't have a great solution to suggest unfortunately.

Owner

scottjehl commented Feb 28, 2014

I think that's okay, so long as the Polyfill script is written in such a way that it handles a very broad group of browsers, including those that do not even support media queries at all.
As long as the last source element can act as a catch-all and match the fallback image, it seems like it'd be okay to me.

On this note though, I do think that sticking with older JavaScript methods throughout our codebase will go a long way. For example, using getElementsByTagName instead of QSA, despite the inconvenience. Aiming for support in any browser with a basic JS implementation would be ideal to pair with the noscript usage.

On Feb 28, 2014, at 12:46 PM, David Newton notifications@github.com wrote:

Since the new picture is leveraging the actual element to do all the work, putting it in a noscript tag would make it not work in any UA that actually does support the picture element. So it would defeat the purpose of moving to the real syntax, I think. I don't have a great solution to suggest unfortunately.


Reply to this email directly or view it on GitHub.

Collaborator

jonathantneal commented Feb 28, 2014

Awesome! Any plans to implement RespImg or is that out of scope?

Contributor

jansepar commented Feb 28, 2014

@jonathantneal that one is out of date. From that page: "This specification is obsolete and has been replaced by the document at http://picture.responsiveimages.org/. Do not attempt to implement this specification. Do not refer to this specification except as a historical artifact."

Collaborator

jonathantneal commented Feb 28, 2014

@jansepar, thanks for pointing that out to me. The parts I liked from RespImg made their way into <picture> so I’m a happy camper, and also a little more with the times.

@obxdesignworks obxdesignworks referenced this issue in robwierzbowski/jekyll-picture-tag Mar 1, 2014

Merged

Patch: fix markdown indentation #31

Contributor

jansepar commented Mar 10, 2014

@scottjehl would love a review on my PR if you have time sometime soon! Should get us significantly closer to a solution that more closely resembles the picture element spec.

Owner

scottjehl commented Mar 10, 2014

Looking great, @jansepar . I'd love if we could start breaking things into independent modules so that features can be built independently – srcset, type, sizes, and media (unless media should be part of the core.

Also, as long as it tests well, I'm cool with the video workaround that you cleverly found. It'd be nice to get IE included in the overall source-selection logic instead of serving it a fallback.

I'm still not sure we should support the span markup alternative, particularly in the interest of keeping the source code clear and simple. I'll let others weigh in on that but I think that's my main criticism on this pass. I love the effort here - please keep it up! :)

Collaborator

Wilto commented Mar 10, 2014

+1 to this being awesome—great work, man.

I think I still lean towards giving IE the fallback over the video workaround (but I could be convinced either way), and media feels like a “core” sort of thing to me.

Contributor

jansepar commented Mar 10, 2014

@scottjehl @Wilto thanks! I'll look into making it more modular, as well as write some tests to see if there are any performance implications of using the video workaround.

I was also thinking that rather then starting the image downloads at DOMContentLoaded, we could instead load the script async in the <head>, and then resize images over an interval until DOMContentLoaded. That way, we can start downloading images much quicker. cc @yoavweiss. I did something very similar in mobifyjs if you want to see an example: https://github.com/mobify/mobifyjs/blob/v2.0/examples/resizeImages-no-capturing/index.html

Collaborator

yoavweiss commented Mar 10, 2014

+9999 to that
The BBC are doing something similar on RAF, but a simple interval is probably better. (more flexible, and this has nothing to do with animation frames).

Contributor

jansepar commented Mar 10, 2014

Awesome. I already have the codes for that - I'll bring it into this PR :)

Contributor

jansepar commented Mar 16, 2014

@yoavweiss Alright, added async support! The default mode is to poll the document until its ready, meaning you can include the script async: https://github.com/jansepar/picturefill/blob/master/index.html#L12. Here is the code to support that: https://github.com/jansepar/picturefill/blob/master/picturefill.js#L266

Collaborator

yoavweiss commented Mar 16, 2014

Awesome work, thanks! I wonder, how did you get to the 250ms number? Do you
think it's worth while to test multiple interval values and find an optimal
one?

Le dimanche 16 mars 2014, Shawn Jansepar notifications@github.com a
écrit :

@yoavweiss https://github.com/yoavweiss Alright, added async support!
The default mode is not to poll the document until its ready, meaning you
can include the script async:
https://github.com/jansepar/picturefill/blob/master/index.html#L12. Here
is the code to support that:
https://github.com/jansepar/picturefill/blob/master/picturefill.js#L266

Reply to this email directly or view it on GitHubhttps://github.com/scottjehl/picturefill/issues/125#issuecomment-37772173
.

Collaborator

yoavweiss commented Mar 16, 2014

Going over the code, I see that you're going over all the picture elements
for each interval. It'd probably be better to keep track of elements
covered in previous intervals and skip their handling.

Le dimanche 16 mars 2014, Yoav Weiss yoav@yoav.ws a écrit :

Awesome work, thanks! I wonder, how did you get to the 250ms number? Do
you think it's worth while to test multiple interval values and find an
optimal one?

Le dimanche 16 mars 2014, Shawn Jansepar <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
a écrit :

@yoavweiss https://github.com/yoavweiss Alright, added async support!
The default mode is not to poll the document until its ready, meaning you
can include the script async:
https://github.com/jansepar/picturefill/blob/master/index.html#L12. Here
is the code to support that:
https://github.com/jansepar/picturefill/blob/master/picturefill.js#L266

Reply to this email directly or view it on GitHubhttps://github.com/scottjehl/picturefill/issues/125#issuecomment-37772173
.

Contributor

jansepar commented Mar 16, 2014

@yoavweiss the 250 number was a total guess :). As for going over all the picture elements - I did that to keep the change minimal for now. Keeping track of the elements already covered means adding more complexity because we'd need multiple modes (when resizing, we wouldn't want to prevent us from changing those images).

I completely agree that this change should be made, just won't have time to do it today. Once I make that change it also means I'd be more comfortable decreasing the polling interval length!

Collaborator

yoavweiss commented Mar 16, 2014

Sounds like a plan :)

Le dimanche 16 mars 2014, Shawn Jansepar notifications@github.com a
écrit :

@yoavweiss https://github.com/yoavweiss the 250 number was a total
guess :). As for going over all the picture elements - I did that to keep
the change minimal for now. Keeping track of the elements already covered
means adding more complexity because we'd need multiple modes (when
resizing, we wouldn't want to prevent us from changing those images).

I completely agree that this change should be made, just won't have time
to do it today. Once I make that change it also means I'd be more
comfortable decreasing the polling interval length!

Reply to this email directly or view it on GitHubhttps://github.com/scottjehl/picturefill/issues/125#issuecomment-37772717
.

Owner

scottjehl commented Mar 25, 2014

Just adding a note here that our recommended markup may be affected by however this conversation pans out: ResponsiveImagesCG/picture-element#131 (comment)

@scottjehl scottjehl added the v2 label Apr 7, 2014

@scottjehl scottjehl added this to the Version 2 Alpha milestone Apr 7, 2014

Owner

scottjehl commented Apr 7, 2014

Closing this out now that it's replaced by itemized tickets for v2

Milestone here: https://github.com/scottjehl/picturefill/issues?milestone=1&state=open

@scottjehl scottjehl closed this Apr 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment