WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#22329 closed enhancement (fixed)

Retina Gravatars

Reported by: miqrogroove Owned by:
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: ui Cc:

Description

Mentioned by Matt in #21019

Should be a simple matter of changing get_avatar() in pluggable.php so that the requested size is twice the display size.

Attachments (9)

miqro-retina-gravatars.diff (1.4 KB) - added by miqrogroove 7 years ago.
HiDPI Gravatars, tested and working!
hidpi-gravatars.zip (2.3 KB) - added by miqrogroove 7 years ago.
Plugin solution involving Javascript to replace avatars after page load.
miqrogroove.com.png (86.4 KB) - added by andy 7 years ago.
screen shot with gravatar
image_3.jpg (45.2 KB) - added by iseulde 5 years ago.
22329.patch (1.4 KB) - added by iseulde 5 years ago.
22329.2.patch (963 bytes) - added by iseulde 5 years ago.
22329.3.patch (971 bytes) - added by iseulde 5 years ago.
fix-r31721.patch (513 bytes) - added by miqrogroove 5 years ago.
22329-test_get_avatar_fix.diff (621 bytes) - added by jipmoors 5 years ago.
Fixes failing unittest because of added 'srcset' parameter on img tag

Download all attachments as: .zip

Change History (75)

@miqrogroove
7 years ago

HiDPI Gravatars, tested and working!

#1 @alexvorn2
7 years ago

  • Keywords needs-testing added

I see that your patch just double the avatar size. And,
what is the point having HiDPI gravatars on non-retina devices?

#2 @alexvorn2
7 years ago

  • Keywords 2nd-opinion added; needs-testing removed

#3 follow-up: @miqrogroove
7 years ago

Serving HiDPI to all clients avoids the addition of client detection scripts. Since Gravatar doesn't offer a better solution, this seemed like the best way to go for now.

#4 in reply to: ↑ 3 @alexvorn2
7 years ago

retina support is made via css file not via php...

#5 follow-up: @miqrogroove
7 years ago

lol okay then show me the CSS for Retina Gravatars :)

#6 in reply to: ↑ 5 @alexvorn2
7 years ago

Replying to miqrogroove:

lol okay then show me the CSS for Retina Gravatars :)

I think a new ticket should be added: Twenty Twelve: Gravatar support for retina displays, and there maybe I will show an example. ;)

Last edited 7 years ago by alexvorn2 (previous) (diff)

#7 @miqrogroove
7 years ago

I propose that a second solution would be to generate two IMG elements for every Gravatar, with each non-Retina version having a common class, and each Retina version having both a common class as well as an inline STYLE attribute having display:none. This would be backward compatible with non-Retina themes, and would allow new themes to reset the display properties based on the class names.

The drawback would be the duplication of all of the IMG elements, resulting in a larger response entity.

#8 @toscho
7 years ago

  • Cc info@… added

That would make the site significantly slower without any benefit for 95% of all visitors. That’s plugin territory.

#9 @miqrogroove
7 years ago

Based on feedback in IRC, a Javascript solution may be ideal. I'll put it together in plugin form, and if not implemented I can release it that way.

@miqrogroove
7 years ago

Plugin solution involving Javascript to replace avatars after page load.

#10 @miqrogroove
7 years ago

Plugin attached. This is the Javascript strategy, resulting in more bandwidth used by Retina devices (they have to load the Gravatars twice), and less bandwidth (just the script) for standard resolution devices.

#11 @beaulebens
7 years ago

This solution makes a lot of assumptions that make it pretty fragile:

  • That s= is the last parameter on the URL
  • That all images with class="avatar" are Gravatars
  • That there are no other parameters ending in s (e.g. &monsters=something)

Unfortunately I think this is significantly more complex to do in a way that covers all variations in a cross-browser fashion. I'll see if I can round up some more details.

#12 @miqrogroove
7 years ago

First assumption is not true. The other two, yes.

#13 @miqrogroove
7 years ago

This will not get a 3.5 milestone according to Nacin in IRC. I'll leave this ticket open for brainstorming, and get the plugin version released.

beaulebens, regarding your third assumption, I should be able to parse on both ?s= and &s= rather than just s=. That would only leave your one concern about avatars that are not gravatars. Perhaps to make the plugin a bit less fragile, I can add an if statement to confirm the presence of the s= parameter before modifying any image attributes.

#14 @beaulebens
7 years ago

Since you already have the src of the image, why not just check for the string 'gravatar.com' in it?

#15 @miqrogroove
7 years ago

I agree. I am adding a check on the domain name as well as the extra hooks for admin pages and the admin bar. :)

#16 @miqrogroove
7 years ago

I've got the above issues ironed out. New plugin files will be at these URLs

http://www.miqrogroove.com/pro/software/
http://wordpress.org/extend/plugins/hidpi-gravatars/

#17 @alexvorn2
7 years ago

  • Cc alexvornoffice@… added

#18 @andy
7 years ago

Prior art: http://wordpress.com/wp-content/js/devicepx.js

A version of this script is included in all pages while Jetpack is activated.

It polls the browser zoom level and updates gravatars and other image URLs with known height/width parameters. This has been in operation for several months with no reported issues.

Detecting zoom level is more complex than detecting device pixel ratio. The script actually tries to map the image resolution directly onto device pixels. One of the benefits of this approach is that it also improves visuals on all devices (even LoDPI) when the browser is zoomed in. Try zooming in on any wordpress.com blog with gravatars. For another example, when viewing on an iPhone with 2x pixels, in landscape mode mobile safari will zoom to 1.5x for a logical device pixel ratio of 3x. This devicepx.js detects this and loads the 3x resolution.

You are welcome to integrate any part of this code into your plugin or core patch.

#19 follow-up: @miqrogroove
7 years ago

Don't users expect images to get blurry when zoomed?

When you say the iPhone zooms to 1.5x in landscape, is that automatic? For example, if you went to my website would you be able to find a blurry Gravatar and take a screen shot of it?

#20 in reply to: ↑ 19 @andy
7 years ago

Replying to miqrogroove:

Don't users expect images to get blurry when zoomed?

Yes, in the same way they expect their blender to get stuck with a bubble when making a milkshake. It's the kind of expectation you break if possible. Just as text is re-rendered at the zoomed resolution, images with a hi-res src are re-rendered. All we're doing is upgrading the src when we detect the zoom is > 1. That's a win.

Note that this only operates on image URLs that match the patterns in the script. They either exist on a known image resizing service (gravatar, files.wordpress.com) or they have 1x in the right place in the filename. Other images are left alone. So it's hard to imagine a case where this would be undesirable.

When you say the iPhone zooms to 1.5x in landscape, is that automatic? For example, if you went to my website would you be able to find a blurry Gravatar and take a screen shot of it?

Sure.

#21 follow-up: @miqrogroove
7 years ago

I'm finding this unintuitive because on my website if I don't specify a zoom level 1.0, iPad gives me a pixel ratio of 2.09 in landscape and 1.57 in portrait. So 2x Gravatars work great there. I might just have to get my hands on a friend's iPhone to see what's going on with a 3x ratio.

@andy
7 years ago

screen shot with gravatar

#22 in reply to: ↑ 21 @andy
7 years ago

Replying to miqrogroove:

I'm finding this unintuitive because on my website if I don't specify a zoom level 1.0, iPad gives me a pixel ratio of 2.09 in landscape and 1.57 in portrait. So 2x Gravatars work great there. I might just have to get my hands on a friend's iPhone to see what's going on with a 3x ratio.

devicepx rounds up so you'd get 2x for 1.56 and 3x for 2.09.

#23 @miqrogroove
7 years ago

devesine on IRC pointed this out http://webdesignerwall.com/tutorials/iphone-safari-viewport-scaling-bug

I switched my site's viewport to device-width, so I have an idea of what's going on with the "3x" issue.

I'm satisfied that the devicePixelRatio and the zoom level are still two separate issues. In other words, loading 2x Gravatars doesn't hurt anything. Whether or not to reload images during zooms would be a design decision.

#24 @miqrogroove
7 years ago

A note on the direction the plugin is taking: The pure Javascript solution doesn't catch Gravatars added by ajax actions. This problem is less obvious in the devicepx.js solution because the script gets executed every second after page load. Still, devicepx.js is not needed. I was able to implement server-side Gravatar filtering by setting a cookie in Javascript on the first visit. This strategy is compatible with admin pages only. Admin pages don't get cached. If anyone is using ajax on the front end, I'll have to figure out how to hook into those hits without affecting the cached pages.

#25 @miqrogroove
7 years ago

I did some research into whether this could be adapted to HiDPI printer output. The closest Javascript facility I found was the matchMedia.addListener() method. In Chrome, this got me as far as actually loading the 2x Gravatars, but the print preview became so unstable that all other output was effectively corrupt. It looks like that side of HiDPI technology isn't ready for use in the wild.

#26 @johnbillion
7 years ago

  • Cc johnbillion added

#27 @johnbillion
6 years ago

#24887 was marked as a duplicate.

#28 @DrewAPicture
5 years ago

  • Component changed from General to Users
  • Focuses ui added

@iseulde
5 years ago

#29 @iseulde
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Here's what this looks like on mobile... :(

@iseulde
5 years ago

#31 follow-up: @iseulde
5 years ago

  • Keywords has-patch added; 2nd-opinion removed

What about making this a progressive enhancement? This works in most browsers now, even iOS. http://caniuse.com/#search=srcset

#32 @iseulde
5 years ago

  • Component changed from Users to General

#33 in reply to: ↑ 31 @ocean90
5 years ago

Replying to iseulde:

What about making this a progressive enhancement? This works in most browsers now, even iOS. http://caniuse.com/#search=srcset

See #31004.

#34 @miqrogroove
5 years ago

"Most browsers" is not how I would characterize the srcset support. I researched this recently and found that it was not worth pursuing in the plugin due to bloat in code and complexity. It may be a year or two yet before the browser market has mature support for that.

#35 follow-up: @iseulde
5 years ago

Chrome supports it, Safari, iOS Safari, Opera, Android. Firefox will soon support it. The only one is IE.
And this only takes a few lines of code and won't break anything.

Last edited 5 years ago by iseulde (previous) (diff)

#36 in reply to: ↑ 35 ; follow-up: @miqrogroove
5 years ago

Replying to iseulde:

And this only takes a few lines of code and won't break anything.

With regard to the plugin, this would break the plugin in Firefox and IE. So I'm not going to do that. If you're saying we should add this to core and have it just not work for some people, that sounds like a new feature that isn't ready for core.

#37 follow-up: @georgestephanis
5 years ago

When looking at the browsers that support it, you also need to subset that against browsers that can realistically be run on devices that actually are HiDPI.

IE can be largely disregarded as the majority of HiDPI devices out in the world are MacBooks, iOS devices, and Android devices. The majority of which aren't running Firefox or IE.

Simply adding in a srcset attribute won't break anything, it's simply progressive enhancement in a standards compliant way.

+1 to doing it.

#38 @iseulde
5 years ago

With regard to the plugin, this would break the plugin in Firefox and IE. So I'm not going to do that.

Not saying you should change your plugin, just suggested something like the patch above.

If you're saying we should add this to core and have it just not work for some people, that sounds like a new feature that isn't ready for core.

Why? :)

#39 in reply to: ↑ 36 ; follow-up: @georgestephanis
5 years ago

Replying to miqrogroove:

Replying to iseulde:

And this only takes a few lines of code and won't break anything.

With regard to the plugin, this would break the plugin in Firefox and IE. So I'm not going to do that. If you're saying we should add this to core and have it just not work for some people, that sounds like a new feature that isn't ready for core.

I believe you mean that compared to forcing all avatars to download at 4x the necessary pixels, this is less effective.

That's correct.

And I'm 100% okay with that.

It's also better for folks on mobile devices as they won't need to download images with 4x the number of pixels.

This change will also support HiDPI screens that are @3x like some newer generation iPhones -- and not lock into a @2x dependency.

Last edited 5 years ago by georgestephanis (previous) (diff)

#40 @georgestephanis
5 years ago

(And just as I realized the other patch from #31004 hadn't been cross-uploaded here: https://core.trac.wordpress.org/attachment/ticket/31004/add-srcset-to-avatars.diff -- that will also handle other arbitrary HiDPI resolutions via srcset, not merely @2x)

#41 in reply to: ↑ 39 ; follow-up: @miqrogroove
5 years ago

Replying to georgestephanis:

I believe you mean that compared to forcing all avatars to download at 4x the necessary pixels, this is less effective.

No. I'm comparing the srcset feature to the existing plugin which only downloads hidpi images when the device pixel ratio is >= 1.5.

#43 in reply to: ↑ 41 @georgestephanis
5 years ago

Replying to miqrogroove:

Replying to georgestephanis:

I believe you mean that compared to forcing all avatars to download at 4x the necessary pixels, this is less effective.

No. I'm comparing the srcset feature to the existing plugin which only downloads hidpi images when the device pixel ratio is >= 1.5.

Ah, so a JS polyfill. That could easily test for browser support of srcset -- no? It's a bit of a stretch to say "You're breaking the plugin!" by implementing something standards compliant in core that lets browsers natively handle responsive images as they're meant to without forcing images to re-download twice (as your current plugin does)

#44 @miqrogroove
5 years ago

Well I'm not saying any of the solutions so far are optimal. When I have some free time, I will try to figure out the exact impact of using srcset in Firefox and IE. How does it affect desktops, how does it affect Surface users, etc.

@iseulde
5 years ago

#45 @miqrogroove
5 years ago

I will also attempt to test this in JS for a practical fallback:

http://www.webkit.org/demos/srcset/

If all the supported browsers default to the srcset value then that should be easy.

This ticket was mentioned in Slack in #feature-respimg by melchoyce. View the logs.


5 years ago

#47 @joemcgill
5 years ago

+1 for using the srcset attribute for this. If a polyfill is desirable, Picturefill is tracking pretty closely to the W3C spec (but may be more than what's needed/wanted for now). Polyfill or not, I think a progressive enhancement solution is the best option because it won't force extra downloads and won't break anything if a browser/device doesn't yet support srcset.

#48 @iseulde
5 years ago

  • Milestone changed from Future Release to 4.2

Let's consider this.

#49 @miqrogroove
5 years ago

Found a related problem in [31107]. The args list includes 'size' but then uses it as both a URL parameter as well as the height and width attributes of the image. And there are no extra args for things like srcset. Unless more args get added to core, the plugin will still have to filter the whole element rather than tweaking args.

#50 @miqrogroove
5 years ago

Plugin version 1.4 just committed. After several uglier drafts, I was able to get reliable detection of srcset support using a little DOM hack and added the appropriate filters to insert the attributes. Rather than breaking in Firefox, IE, etc, the plugin falls back to JS. It also falls back to server-side URL filtering for all admin pages to avoid problems with ajax.

Most users probably wont notice any difference from 1.3, but you are welcome to surf the new code for ideas.

https://wordpress.org/plugins/hidpi-gravatars/

#51 in reply to: ↑ 37 @miqrogroove
5 years ago

Replying to georgestephanis:

When looking at the browsers that support it, you also need to subset that against browsers that can realistically be run on devices that actually are HiDPI.

IE can be largely disregarded as the majority of HiDPI devices out in the world are MacBooks, iOS devices, and Android devices. The majority of which aren't running Firefox or IE.

I've been using a device-independent strategy because pixel ratio can be influenced by accessibility features. The 2x view can be configured very easily in Firefox and IE.

#52 @helen
5 years ago

I'm good with 22329.2.patch - I get that there are cases where browsers that don't (yet) support srcset would look better with a larger provided image, but I don't find those to be common enough to warrant a JS-based polyfill in core, especially given that get_avatar() can be and is used quite frequently on the front end.

#53 @helen
5 years ago

  • Keywords needs-refresh added

@iseulde
5 years ago

#54 @iseulde
5 years ago

  • Keywords needs-refresh removed

#55 @helen
5 years ago

In 31721:

Gravatars: Enable HiDPI versions for browsers that support srcset.

props iseulde.
see #22329.

#56 @miqrogroove
5 years ago

@helen, the 1x URL does nothing in this situation, except bloat the code.

esc_attr( "$url2x 2x" )

... would be more appropriate.

#57 follow-up: @iseulde
5 years ago

I just added that because most examples I've seen add it, including the demo you linked. Not sure if it's really necessary though.

#58 @miqrogroove
5 years ago

Patch added.

#59 in reply to: ↑ 57 @miqrogroove
5 years ago

Replying to iseulde:

I just added that because most examples I've seen add it, including the demo you linked. Not sure if it's really necessary though.

It's only used to set a different URL on browsers that support srcset. When the URLs are identical, it does nothing. :)

This ticket was mentioned in Slack in #feature-respimg by iseulde. View the logs.


5 years ago

#62 @helen
5 years ago

In 31722:

Gravatars: Remove redundant 1x srcset.

props miqrogroove.
see #22329.

#63 @helen
5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Actually, I'll call this fixed for now. Plugins are a great way to further expand HiDPI support for Gravatars, IMO.

This ticket was mentioned in Slack in #core by helen. View the logs.


5 years ago

@jipmoors
5 years ago

Fixes failing unittest because of added 'srcset' parameter on img tag

#65 @helen
5 years ago

In 31728:

Fix a unit test after [31721].

props jipmoors.
see #22329.

#66 @miqrogroove
4 years ago

#31004 was marked as a duplicate.

Note: See TracTickets for help on using tickets.