ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Feature detect need for picturefill #188

Closed peterwilsoncc closed 9 years ago

peterwilsoncc commented 9 years ago

See gh-186

Feature detects the need for picturefill and loads asynchronously if needed. This operates in the same fashion as the Emoji pollyfill in core.

The feature detection is output directly in the HTML header, in the first push I have inserted the uncompressed code using a readfile command. In the patch for merge I'd suggest using grunt to insert the compressed code directly in the PHP file.

All this is very similar to the emoji-loader script, it could be worth repurposing the emoji-loader to a pollyfill loader.

It's worth reminding you all outputting the Emoji feature detection in the HTML header was controversial. The performance gains were not well understood at the time.

azaozz commented 9 years ago

The patch looks good. Even thinking it can be further simplified:

Maybe something like this:

function tevkori_get_picturefill() {
    $version = '2.3.1'; // picturefill.js version
    $picturefill_source = plugins_url( 'js/picturefill.min.js?ver=' . $version, __FILE__ );
    $picturefill_source = apply_filters( 'script_loader_src', $picturefill_source, 'picturefill' );
    ?>
    <script type='text/javascript'>
        ( function() {
            var image  = new Image(),
                script = document.createElement( 'script' ),
                firstScript = document.getElementsByTagName( 'script' )[0];

            // Adapted from picturefill 2.3.1
            if ( 'sizes' in image && 'currentSrc' in image && 'srcset' in image ) {
                return;
            }

            script.async = script.src = '<?php echo esc_url_raw( $picturefill_source ); ?>';
            firstScript.parentNode.insertBefore( script, firstScript );
        }());
    </script>
    <?php
}
azaozz commented 9 years ago

Hmm, actually the test for supporting srcset and sizes will also have to test if the browser and the device can use them. No need to load these on desktops or laptops unless they have "retina" displays, etc. So it should probably be:

if ( ! window.devicePixelRatio || window.devicePixelRatio === 1 || ( 'sizes' in image && 'currentSrc' in image && 'srcset' in image ) ) {
    return;
}
peterwilsoncc commented 9 years ago

That makes sense. Including uncompressed code may cause politics but I can ride it out.

Should picturefill be included, what are the chances of refactoring emoji-loader to a general purpose polyfill loader? Emoji-loader already includes a JS script loader.

joemcgill commented 9 years ago

No need to load these on desktops or laptops unless they have "retina" displays

I want to reiterate that this line of thinking is quite wrong. One of the main issues that responsive images solves is making sure smaller viewports and non-retina displays aren't forced to download image assets that have been optimized for larger/denser devices when they get no benefit from all those extra pixels.

In fact, I would argue that the main improvement we bring to WordPress by including support for srcset and sizes is the performance gains sites will get by loading smaller images when applicable. If we do add feature detection, we definitely should not require a browser have a devicePixelRatio > 1.

joemcgill commented 9 years ago

We might be able to get away with only detecting for native sizes support. However, we need to be aware of one edge case with Safari 8, where it can prefetch an image src before the polyfill loads, thus triggering a double download. See: https://github.com/scottjehl/picturefill/issues/479

albell commented 9 years ago

The srcset feature detect unfortunately needs to be a bit more thorough in order to handle all browsers and edge cases. We had issues with UC Browser. Please see:

https://github.com/scottjehl/picturefill/issues/564

Final code in Picturefill 3.0 at:

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

peterwilsoncc commented 9 years ago

Thanks @albell, for now I'll match the test with the version shipped with the plugin but I've started watching the picturefill repo to keep track of any changes.

Changes pushed to simplify the JavaScript. For the review process I am keeping in a separate file as text editor highlighting is super handy.

peterwilsoncc commented 9 years ago

Following discussion in gh-186 going to close this, it's a deviation from the merge-ready path.

If reconsidered, it can be reopened.