WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#26221 closed defect (bug) (fixed)

Twenty Fourteen: wp_is_mobile() shouldn't be used

Reported by: dimadin Owned by: lancewillett
Milestone: 3.8 Priority: high
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

[26026] introduced usage of wp_is_mobile() function for determining size of thumbnail that should be displayed (see #25758).

Since wp_is_mobile() checks current visitor, it should be used only on dynamic pages, how it was done in core before (used on admin pages only).

Although frontend is dynamic by default, when page cache is used it isn't. This can for example lead to situation where page view from mobile gets to cache which is then served to all visitors until cache is deleted/expires.

In this case, until browser supports for responsive images becomes standard, solution is to load full-width image or use some Javascript solution.

Attachments (1)

26221.patch (1.5 KB) - added by kwight 3 years ago.

Download all attachments as: .zip

Change History (11)

#1 @lancewillett
3 years ago

Let's discuss this more, I think the case of using a plugin with aggressive caching is valid. But, I think the tradeoffs are worth it.

For example, if the first visitor to populate the page cache is on a large monitor, the images will load too large for mobile visitors, but it'll still work just fine.

#2 @iamtakashi
3 years ago

  • Cc takashi@… added

#3 @nacin
3 years ago

I think I agree with dimadin here. I discussed wp_is_mobile() usage with iandstewart before it landed, but this didn't occur to me. I wouldn't call this "aggressive" caching, just any kind of caching. Batcache, WP Super Cache, W3 Total Cache, any of them.

In the end, I don't think this is something the theme should be messing with. I totally get that we owe it to the world to serve smaller images to mobile devices, but this just isn't the right way to do it. We need to be dropping a cookie based on bandwidth and/or device. This is not something a theme, certainly not a default one, should be doing.

I'm also not in love with yet another image size, though I can't fault you for it. It would be nice to somehow figure out dynamic sizing at one point.

#4 @nacin
3 years ago

  • Priority changed from normal to high

#5 @jartes
3 years ago

  • Cc joan@… added

@kwight
3 years ago

#6 @kwight
3 years ago

26221.patch keeps the full-width image size for layouts without sidebars, while removing the wp_is_mobile check.

#7 @kwight
3 years ago

  • Cc kwight@… added
  • Keywords has-patch added

#8 @lancewillett
3 years ago

  • Milestone changed from Awaiting Review to 3.8

#9 @lancewillett
3 years ago

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

In 26599:

Twenty Fourteen: remove wp_is_mobile() call from post thumbnail size logic as it's not intended for front-end use. Props kwight, fixes #26221.

This ticket was mentioned in Slack in #themereview by emiluzelac. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.