Consider removing picturefill.js, or enqueueing it only when needed #186

Closed
azaozz opened this Issue Sep 25, 2015 · 22 comments

Projects

None yet

9 participants

@azaozz
azaozz commented Sep 25, 2015

Reasons:

  • Adding (forcing) a JS library or polyfill to all existing WordPress sites and all existing themes will bring errors / edge cases.
  • The main benefit of picturefill.js is support for <picture>, but that element is not used by default in WordPress.

Looked around a bit trying to find which browsers would benefit from having picturefill.js and not having any <picture> elements. Seems that all current browsers that are capable of displaying high-res images already support srcset and sizes. The only exception is iOS Safari 8.4 (current is 9.0), so perhaps the JS can be added just there, if at all.

@peterwilsoncc

pull request in gh-188

@joemcgill

We've discussed this but I don't feel like we've made a final decision. If we're not going to include PF in WordPress we should also remove it from the plugin for consistency.

The main benefit of picturefill.js is support for <picture>

This is not quite accurate. Picturefill is a full JS implementation of the responsive image spec, which includes support for srcset in all it's permutations. If you check browser support at caniuse.com, you'll see native support is looking good for all modern browsers, but older devices would still benefit from the polyfill.

Seems that all current browsers that are capable of displaying high-res images already support srcset and sizes

Keep in mind that the most important user benefit of responsive images is the ability to deliver smaller images to users when possible, not the opposite.

Since this polyfill is maintained by the RICG, and is relatively small (~11 KB minified), it may be best to keep it in.

@azaozz
azaozz commented Sep 26, 2015

In my mind adding the image attributes is a "froward enhancement". It will benefit relatively smaller percent of the users now, but this number will increase (significantly?) in the future.

If you check browser support at caniuse.com, you'll see native support is looking good for all modern browsers, but older devices would still benefit from the polyfill.

Right, that's where I looked too. Generally about 5% to 20% of a site visitors will benefit from having the new image tag attributes (depending on region). From these, only about 1% to 8% may need to have picturefill.js: these are the Safari 8, iOS Safari 8.x and MS Edge users on devices with high-res screens. As both Safari and iOS Safari were recently updated to version 9 which includes full support for the image attributes, I'm expecting the above percentage to fall significantly by the time WordPress 4.4 is released in December.

Force-loading a polifill that will benefit only few percent of the users on all WordPress sites is not right imho :)

@peterwilsoncc

My preference is clear in the PR, feature test and load picturefill asynchronously if needs be.

I do agree retina screens are a side issue, the more important use case is displaying smaller images on smaller screens. Or saving content editors from themselves when uploading 12,000 pixel wide images.

It's worth mentioning picturefill includes the window.matchmedia pollyfill.

@robneu
robneu commented Sep 27, 2015

Could we please not add more front-end JS to WordPress core? I agree with @azaozz here. IMO, the benefit is nowhere near enough to justify adding a polyfill for this.

@tevko
Member
tevko commented Sep 27, 2015

Hey all, looks like some further clarification is needed here. First off it's worth stating that the polyfill is 7.53kb before GZIP'ing.

@azaozz the purpose of the polyfill is to ensure support for all browsers, down to IE9 and even earlier in some cases. This means that with the polyfill enabled, all visitors to a users site will benefit from having the polyfill loaded, regardless of their viewport dimensions / DPI.

@robneu The benefit of the pollyfill is saving a visitor from having to download an image that is bigger / heavier than they need.This can potentially save the user megabytes worth of data, which I think is a fair trade to make for 7k of javascript.

Overall what we're asking here is to asynchronously load an insignificantly small amount of javascript in order to save users (on old browsers) from having to download images that are too big. Yes the polyfill is extra, but it is a more than fair tradeoff to make (7k versus a potentially super heavy image).

I think feature testing is a fair compromise, and will ensure that no one misses out on responsive images

@Wilto
Member
Wilto commented Sep 28, 2015

As a Picturefill maintainer and partial author of the responsive images specs, I’ve gone both ways on this with my own projects. All these markup patterns have a really solid fallback pattern built in, so omitting Picturefill is just as valid an approach as using it. Worth keeping in mind, though, that Picturefill comes with tremendous byte savings and memory usage potential for users on older devices and browsers—well above and beyond the cost of transferring and parsing Picturefill itself.

The responsive images choose-your-own-adventure book usually breaks down to one of three approaches:

1. Omit Picturefill, Provide a Reasonably-Sized Fallback
I used this approach on HBR.org recently (not live just yet, I don’t think). This relies on the native src fallback pattern—which is bulletproof—using one of the mid-breakpoint image cuts as the fallback. This means a reasonably sized default for browsers without responsive images support, but also means larger and/or high-res users in non-respimg browsers will see an inappropriately-sized image. This doesn’t necessarily feel broken to them—a lot of the web looks this way, as things stand now—but it might be a little unpredictable for theme maintainers. I’ve seen a few “wrong size image loaded in [older browser]” QA tickets with this approach in the past.

2. Omit Picturefill, Provide Original Upload as Fallback
Without respimg support: users on large/high-resolution displays get large high-res images, users on small/low-res displays… also get large high-res images. As you can probably guess, there’s a pretty small overlap between “users on low-resolution browsers” and “users on browsers with responsive images support,” but this does still help deal with image size on smaller displays with modern browsers. This is likely the most seamless approach to rolling these features out, but means users only getting responsive images as the browser landscape gradually changes. It’s an optimization for the latest-and-greatest, not necessarily the users in underpowered browsing contexts who would need it the most.

3. Use Picturefill
This means loading more JavaScript—and I am no advocate of loading more JavaScript when less will do—but also means the benefits and byte/memory savings of native responsive images across the entire browser landscape. The trade-off is that we do give up some of the native fallback pattern in using Picturefill, which has to be worked around to avoid a double-download. Older browsers without native support for responsive images would be beholden to Picturefill for images, newer browsers with partial support for responsive images will have only the syntaxes they lack shimmed (and first-wave implementation browser bugs worked around or fixed), and browsers with full respimg support won’t run Picturefill at all.

Considering the number of users this feature will reach I’d lean heavily toward using Picturefill, with the reasonably-sized native fallback second.

@robneu
robneu commented Sep 28, 2015

@tevko I understand the reasoning, I just don't agree with it. I don't see why users on newer browsers should be forced to download extra, albeit fairly small, code to benefit a minority of users on old browsers. I'm not really a fan of the polyfill concept in general, but in this case I'm really against it because it would be the default in WordPress core and impact a huge amount of the web.

If people want to add support for old browsers in a theme or plugin, it's not like it would take a great amount of effort for them to load picturefill themselves...

Out of the choices mentioned by @Wilto IMO 1 makes the most sense as the default. If ya'll do decide to use picturefill as the default, eventually it will be totally pointless and would need to be deprecated... except this is WordPress so that will probably happen years after it should have.

@Wilto
Member
Wilto commented Sep 28, 2015

@robneu I may be misreading, but I think one of the current proposals is to asynchronously load Picturefill only in contexts where the browser has missing/partial native responsive image support—in that case, we’d effectively have a built-in deprecation strategy.

@peterwilsoncc

@robneu Are you happy with the feature detect approach? Keep in mind the compressed code will become:

i=new Image();
if(!('srcset' in i&&'sizes' in i&&'currentsrc' in i))k('path/to/picturefill.js')

should we take the approach I have in mind for the merge. That's all browsers with responsive image support would see.

(Pls excuse any typos, 3am and on phone)

Edit 1: fix to promised typos:

  • fix if condition, incorporating correction below
  • && were || originally
@peterwilsoncc

Correction, should be a !() wrapping the if conditions above.

Note: incorporated with edit to above comment

@robneu
robneu commented Sep 28, 2015

@peterwilsoncc I'm not sure if happy is the right word, but it's definitely preferable to any of the other choices where picturefill is loaded by default.

I realize that loading it async won't impact load times significantly, just as the the emoji code doesn't... but my major concern with this is where does it stop? How many things are we going to integrate into core like this? How much front-end JS is going to be force-loaded into every WordPress install?

IMO this is a slippery slope. It was started by the decision to include the emoji JS by default and this could easily solidify it as a go-to solution for shimming anything and everything into core.

In any event, if this does wind up being the way things go, could you please make it a bit more straightforward to disable than the emoji code? This seems super extreme to disable a feature that IMO should be optional in the first place:

function disable_emojis() {
    remove_action( 'wp_head', 'print_emoji_detection_script', 7 );
    remove_action( 'admin_print_scripts', 'print_emoji_detection_script' );
    remove_action( 'wp_print_styles', 'print_emoji_styles' );
    remove_action( 'admin_print_styles', 'print_emoji_styles' );
    remove_filter( 'the_content_feed', 'wp_staticize_emoji' );
    remove_filter( 'comment_text_rss', 'wp_staticize_emoji' );
    remove_filter( 'wp_mail', 'wp_staticize_emoji_for_email' );
    add_filter( 'tiny_mce_plugins', 'disable_emojis_tinymce' );
}
add_action( 'init', 'disable_emojis' );

/**
 * Filter function used to remove the tinymce emoji plugin.
 *
 * @param    array  $plugins
 * @return   array             Difference betwen the two arrays
 */
function disable_emojis_tinymce( $plugins ) {
    if ( is_array( $plugins ) ) {
        return array_diff( $plugins, array( 'wpemoji' ) );
    } else {
        return array();
    }
}

It's also worth noting that the Disable Emojis plugin (where the above code came from) is already up to 10,000 active installs, so I don't think I'm alone in the feeling that force-loading JS in core is not desirable.

@albell
albell commented Sep 28, 2015

Wilto’s 1 (use an intermediate src size) is pretty much guaranteed to generate support tickets when undersized images load on large desktops running old browsers. There are many layout contexts is which this is going to look just terrible visually.

Wilto’s 2 means loading oversize images on mobile. This is particular painful, and hits underpowered devices with slow connections (i.e. the developing world) the hardest.

Wilto’s 3 (PF on feature detect failure) avoids both of these, catering to both user classes, at the cost of a very small increase in default page weight. The full feature detect to handle all browsers and edge cases is more than just a one-liner, but it is very short:

https://github.com/scottjehl/picturefill/blob/3.0/src/picturefill.js#L886-L898

Ripping out the detect in a couple years (or rather making it optional) should be trivial.

Fully agreed that WP should not feature detect and polyfill everything under the sun. Perhaps there is a terrace somewhere on the “slippery slope.”

@joemcgill

I tend to agree with @albell's assessment of the options as presented. I don't see option 1 being tenable for WordPress because of all the assumptions people have around which intermediate size should show up as the src attribute when they choose a size from the attachment display settings in the insert media modal.

Not including PF at all saves very little bandwidth for users with new devices/browsers at the expense of the users who will benefit the most from a native responsive image solution in WordPress.

I'm inclined to include PF directly, through the normal enqueue process so it's easier to turn off or modify for folks who are so inclined, but adding a smaller feature test + loader would be an acceptable compromise if that's the preferred approach.

@azaozz
azaozz commented Sep 28, 2015

The benefit of the pollyfill is saving a visitor from having to download an image that is bigger / heavier than they need.

Yes, I understand. However I'm not sure if this applies to most WordPress sites. I wish there was a good way to check how many users prefer to insert "large" images vs. "thumbnail" or "medium". Perhaps somebody that manages many WP installs can run couple of DB searches for size-large vs. size-medium and size-thumbnail.

As far as I've seen most users tend to use either medium or thumbnail images. In most (nearly all?) WP themes the "main" area is somewhere between 450-650px wide, even on 2560px screens. The "large" images are typically 1024px, however many themes set $content_width which limits the "large" width.

So this benefit of using the polyfill will be only when all of these are true:

  • The users chose to insert images that are "too wide".
  • The device has a "low-res" screen narrower than the theme's "main area".
  • The theme doesn't set $content_width.
  • The theme has responsive CSS and the browser supports it.
  • The browser doesn't support srcset and sizes.

I'm sure there are sites that meet all points. Not sure how many there are and if it's worth it adding the polyfill to all older browsers for their benefit.

@peterwilsoncc

In any event, if this does wind up being the way things go, could you please make it a bit more straightforward to disable than the emoji code?

At present, the PR uses a single hook to load the polyfill. The particular hook may change but my aim will be to keep it on a single hook for the front end. At present picturefill is not loaded in the WordPress admin.

To share an anecdotal number: on my most recent post with a feature image, responsive images save more than 20K for the small screen user. It makes the polyfill worth it to my mind.

I really, really care about performance and not loading unnecessary JavaScript. Clear performance gains are my where I place my terrace on the slippery slope.

@robneu
robneu commented Sep 29, 2015

To share an anecdotal number: on my most recent post with a feature image, responsive images save more than 20K for the small screen user. It makes the polyfill worth it to my mind.

There's a pretty good chance those small screen users would have received the same benefit without the polyfill, plus the added benefit of not having to load any additional JS.

I don't want to beat a dead horse here, so unless some new information crops up this will be my last response to this thread. I'm honestly not trying to be difficult, so I hope it isn't coming across that way, I just really don't think including this in WP core by default is the right move.

@albell
albell commented Sep 29, 2015

@azaozz

So this benefit of using the polyfill will be only when all of these are true:

The users chose to insert images that are "too wide".

That is the modern workflow. Users will upload the highest resolution they have, targeted to the largest expected use case. Full screen background with fine detail on a 5K screen? You will need very large jpgs. "Too wide" for mobile can be "just right" on a high-DPR large display.

The device has a "low-res" screen narrower than the theme's "main area".

Mobile viewports will typically be narrower than any image intended to display large on desktop. On low end mobile this is especially true.

The theme doesn't set $content_width.

Different theme layouts do different things with images. It's hard to generalize here.

The theme has responsive CSS and the browser supports it.

Responsive CSS is all over the place. Media queries have been in place since IE9. These are now mainstream practices in the wild.

The browser doesn't support srcset and sizes.

Correct. This is the case Picturefill attempts to mitigate. Unloaded images and double image downloads both "break" the web in different ways. Picturefill is an attempt to soften the historical transition between src markup and srcset markup. You seem to be denying that srcset solves a problem at all, or has any benefit. I hope I'm misunderstanding you.

@robneu

There's a pretty good chance those small screen users would have received the same benefit without the polyfill, plus the added benefit of not having to load any additional JS.

A pretty good chance, absolutely. To my mind, that's not the point. I'm talking about the significant number of users who, without a src on an img and no polyfill, would see no image. That situation is onerous, a totally failed user experience, I'm sure we agree. But it is also onerous to force large image downloads on someone with an extremely basic mobile device, and a data plan they can barely afford. (That describes the situation of most in the developing world. It think it's really important to back away from some of our routine first world developer assumptions about hardware and connectivity.) What default-size src would you like to see used? I'm unclear on what you're suggesting, other than a general dislike for polyfills.

@azaozz
azaozz commented Sep 29, 2015

@albell

Users will upload the highest resolution they have...

Right, most users upload straight from their camera/phone. However when inserting images in a post, they mostly seem to insert medium size, followed by thumbnail, followed by large. Considering that most themes have $content_width, the large size is limited to the width of the theme's main area.

Full screen background with fine detail...

We are looking at images inserted in post_content. Most of them are not larger than the "comfortable to read" width, which is about 650 "CSS" pixels. Look at the width of this comment :)

Mobile viewports will typically be narrower than any image intended to display large on desktop. On low end mobile this is especially true.

Right. For example: an old iPhone 4 with 320px wide screen would need 640px image. The large size is lets say 660px wide as set by Twenty Fifteen, and medium size is around 400px. It would still need to use the 660px image whether the user inserted a large size or a medium size, as both images will be end-to-end and need a minimum of about 640px width.

Different theme layouts do different things with images. It's hard to generalize here.

Agreed, and they will continue doing so. There is no point in core trying to predict every user case a theme may come up with. As far as I see it, core has to support the most common cases of images inserted by the users in post_content.

Don't get me wrong. I'd love to have <picture> support in WP by default. I just can't justify it yet.

@getsource

I agree with @azaozz here. Unless there's a clear large percentage of users that we're missing by not providing a polyfill (I haven't seen stats that show this yet), I tend towards avoiding adding a dependency for WordPress Core. I definitely see the case for improving the experience for older mobile browsers, but don't think that the numbers I've seen so far make the case for users of WordPress as a whole.

I'd prefer not to change src value on display, since it seems like that has a good chance to accidentally cause surprise or edge cases, and is more of an intrusion on user intent/content than adding additional attributes (srcset, sizes) to match the user's chosen content.

@joemcgill

Thanks everyone for the great feedback. There seems to be two separate questions on the table here:

  1. Should sites use a polyfill to extend responsive image support to browsers that don't have native support?
  2. Should that polyfill be included in WordPress core and loaded for all sites by default.

On the first question, I think most of us agree that PF has a benefits for users on older/underpowered devices. However, there are a few implementation details which developers need to be mindful of when using Picturefill, which @Wilto listed in his earlier comment in this thread. It is probably best to leave those decisions to theme authors rather than forcing all sites into a single solution.

Additionally, when it comes to including an extra dependency to WordPress core, we have to be mindful of the long-term maintenance since back compatibility is such an important value to WP. To that end, I defer to the judgement of @azaozz and @getsource.

For now, I suggest we keep loading Picturefill via the plugin, but that we do not include it in WordPress core directly. We should also continue to educate theme authors on the benefits and best practices of including Picturefill in their themes.

We can always revisit this down the road.

@jaspermdegroot
Member

👍

@joemcgill joemcgill closed this Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment