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

Photon + Lazy Load doesn't load Retina versions #529

Closed
ChrisBegley opened this Issue May 10, 2014 · 22 comments

Comments

Projects
None yet
6 participants
@ChrisBegley

ChrisBegley commented May 10, 2014

When I have Photon enabled and the Lazy Load plugin from the VIP team, Retina versions of the lazy loaded images aren't being served.
http://wordpress.org/plugins/lazy-load/

The Lazy Load plugin changes the markup to look something like this:

<img src="http://example.com/wordpress/wp-content/plugins/lazy-load/images/1x1.trans.gif" data-lazy-src="http://i0.wp.com/example.com/wordpress/wp-content/uploads/2014/05/example.jpg" />

Could the issue be that the Photon script: http://s0.wp.com/wp-content/js/devicepx-jetpack.js is only looking at the "src" attribute and not the "data-lazy-src" attribute, where the actual image is?

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

@ChrisBegley how does the issue exactly show? Can you simply not see the images?

@ChrisBegley

This comment has been minimized.

ChrisBegley commented May 10, 2014

@rase- The images still show up, but Retina versions aren't being served to HiDPI screens when Lazy Load is active. So the images look a bit blurry. If I disable Lazy Load, then Photon will load in Retina versions as expected.

I believe this script is what Photon uses to decide if Retina versions should be loaded: http://s0.wp.com/wp-content/js/devicepx-jetpack.js

So my guess is that it's only looking at the <img> "src" attribute. With Lazy Load active, the markup gets changed and the image is actually in the "data-lazy-src" attribute.

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

@ChrisBegley thanks for the clarification! I'll try to look into it. :-)

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

@ChrisBegley your suspicion seems correct. I beutified devicepx-jetpack.js and took a peek. It does do scale detection, and does only look at the src attribute.

A recent commit in Jetpack delays the call that will enable scaling when using lazy loading (ffa1b1d). Have you by any chance tried the current master branch or Jetpack, and if you haven't do you have a development setup where you could?

I suspect that the mentioned change might also fix this issue, and it would be extremely helpful if you could report your findings regarding your specific situation.

@ChrisBegley

This comment has been minimized.

ChrisBegley commented May 10, 2014

@rase- Thanks for looking into it! I do have a development site where I can load the current master branch of Jetpack. Let me try that now and get back to you.

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

@ChrisBegley great! Thanks!

@ChrisBegley

This comment has been minimized.

ChrisBegley commented May 10, 2014

@rase- Unfortunately it's still an issue with the master branch.

This is my test site: http://techyourworld.com/

With Lazy Load active, it's still only serving up the normal images to Retina devices:
http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2013/08/american-hustle-bradley-cooper-christian-bale1.jpg?resize=340%2C220

Instead of a Retina version:
http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2013/08/american-hustle-bradley-cooper-christian-bale1.jpg?zoom=2&resize=340%2C220

When I deactivate Lazy Load, everything works as expected and Retina versions of images are served.

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

@ChrisBegley thanks for trying it out. I'll look further into it. I think I can imitate your experience with retina versions at least to some extent via zooming in my browser. I'll get back to you when I figure something out.

@ChrisBegley

This comment has been minimized.

ChrisBegley commented May 10, 2014

@rase- Thanks a lot!

@rase-

This comment has been minimized.

Contributor

rase- commented May 10, 2014

According my current understanding, the reason why the zoom parameter isn't properly set for you is that devicepx-jetpack.js will set scale="0" to all images, which indicates disabling scaling because it looks at the placeholder image instead of the actual source, which doesn't match any of the predefined formats to be handled.

The reason the placeholder image doesn't match any of the pre-existing rules is that the placeholder image doesn't get passed through Photon for you. When trying things out in my test environment, they do. Also for me everything will have the data-recalc-dims attribute. If these would appear for you too, then the simple solution suggested in #530 would be enough.

I will investigate Photon further tomorrow and try to find out more about the best way to deal with both of the cases.

@rase-

This comment has been minimized.

Contributor

rase- commented May 11, 2014

If we change the zoomImages function in devicepx-jetpack.js to be the following

    zoomImages: function () {
        var t = this;
        var scale = t.getScale();
        if (!t.shouldZoom(scale)) {
            return;
        }
        t.zoomed = true;
        var imgs = document.getElementsByTagName("img");
        for (var i = 0; i < imgs.length; i++) {
            if ("complete" in imgs[i] && !imgs[i].complete)
                continue;
            var imgScale = imgs[i].getAttribute("scale");
            if (imgScale == scale || imgScale == "0")
                continue;
            var scaleFail = imgs[i].getAttribute("scale-fail");
            if (scaleFail && scaleFail <= scale)
                continue;
            if (!(imgs[i].width && imgs[i].height))
                continue;
            if (!imgScale && imgs[i].getAttribute("data-lazy-src") && (imgs[i].getAttribute("data-lazy-src") !== imgs[i].getAttribute("src")))
                continue;
            if (t.scaleImage(imgs[i], scale)) {
                imgs[i].setAttribute("scale", scale);
            } else {
                imgs[i].setAttribute("scale", "0");
            }
        }
    }

we avoid both this problem, and the problem presented in #530. The above code example has added an additional check: if no scale has been set yet, and we have a data-lazy-src attribute indicating some form of Photon supported lazy loading, let's postpone the scaling until the src attribute has the same content as the data-lazy-src attribute.

@rase-

This comment has been minimized.

Contributor

rase- commented May 11, 2014

I imitated the reported situation by zooming in in my browser, and hosting a version of the script where the zoomImages function is replaced with the above code, and changed the enqueue to the following

function devicepx() {
        if ( Jetpack::is_active() ) {
            //wp_enqueue_script( 'devicepx', set_url_scheme( 'http://s0.wp.com/wp-content/js/devicepx-jetpack.js' ), array(), gmdate( 'oW' ), true );
            wp_enqueue_script( 'devicepx', 'http://cs.helsinki.fi/u/tonykova/devicepx-jetpack.js', array(), null, true );
        }
    }

in class.jetpack.php.

This fixes issue #530 and I believe it will fix the situation reported here too.

@rase-

This comment has been minimized.

Contributor

rase- commented May 11, 2014

@ChrisBegley if you want, you could try if this change fixes your particular situation by replacing the devicepx function in class.jetpack.php with the above code. :-)

@jeherve jeherve added Photon labels May 11, 2014

@jamo

This comment has been minimized.

jamo commented May 11, 2014

I tried this with @rase-'s version of the script and now the scale attribute and zoom url parameter look correct on my retina mac

@rase-

This comment has been minimized.

Contributor

rase- commented May 11, 2014

@jamox thanks for trying it out. :-)

@ChrisBegley

This comment has been minimized.

ChrisBegley commented May 12, 2014

@rase- Yes! I just loaded up your changes to "devicepx" and now the Retina versions are loading, even with Lazy Load enabled. Awesome, thanks so much!

@rase-

This comment has been minimized.

Contributor

rase- commented May 12, 2014

@ChrisBegley cool! Thanks for your efforts :-)

Does someone know where to properly request a change to devicepx-jetpack.js. @jeherve do you have any idea about this?

@jeherve

This comment has been minimized.

Member

jeherve commented May 12, 2014

@rase- I reported this internally, and we'll take a look at your patch!

Thanks!

@rase-

This comment has been minimized.

Contributor

rase- commented May 12, 2014

@jeherve ok thanks! Could you let me know how it goes? I can also adjust the patch if needed. :-)

@ethitter

This comment has been minimized.

Contributor

ethitter commented May 12, 2014

@rase-, a version of your change was deployed today. Thanks for your efforts!

@ethitter ethitter closed this May 12, 2014

@rase-

This comment has been minimized.

Contributor

rase- commented May 12, 2014

@ethitter awesome! Thanks for getting back to me! :)

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented Nov 18, 2017

There is a new Lazy Load module included with Jetpack which works well with Photon: #8093

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