Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#35030 closed defect (bug) (fixed)

Responsive Images: wrong source selected in iOS8

Reported by: tonysandwich's profile tonysandwich Owned by: joemcgill's profile joemcgill
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hey guys,

With the new release and the addition of the srcset attribute, the lower res 150px x 150px images are causing pretty hefty pixellation on some devices.

If for example you look at featured images using the post_thumbnail function on an iphone 6 they look really pixelated as it is grabbing the 150px image and upscaling it.

I have attached a screenshot, I basically think that the 150px on smaller devices may be a little bit harsh, and there should be a minimum size of perhaps 280 - 300px (around the width of most smartphones)

If you look at the product images on the this page - http://staging.eightarms.co.uk/tabcat/shop/ on an iPhone 6 you will see what I mean, be interested to hear your thoughts as I'm worried that on a lot of newer devices there are going to be some pretty chewy looking images.

I totally understand the need for quick loading times etc..but I think there may be a better compromise, or the ability to set the minimum size would be very useful rather than having no choice in the matter and having to accept 150px as the best size.

Look forward to hearing from you.

Matt

Attachments (7)

Screen Shot 2015-12-12 at 00.14.52.png (550.7 KB) - added by tonysandwich 8 years ago.
Pixellation on iPhone 6
Screen Shot 2015-12-12 at 01.38.46.png (756.8 KB) - added by tonysandwich 8 years ago.
os info
35030.patch (8.5 KB) - added by jaspermdegroot 8 years ago.
35030.1.patch (9.4 KB) - added by jaspermdegroot 8 years ago.
35030.2.patch (2.0 KB) - added by azaozz 8 years ago.
35030.3.patch (8.8 KB) - added by jaspermdegroot 8 years ago.
35030.4.patch (14.3 KB) - added by joemcgill 8 years ago.

Download all attachments as: .zip

Change History (43)

@tonysandwich
8 years ago

Pixellation on iPhone 6

#1 @joemcgill
8 years ago

  • Keywords reporter-feedback added
  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi @tonysandwich,

Thanks for the report. I opened the link to your staging site on my iPhone 6, and I'm seeing a very crisp image. Using Chrome inspector, it shows that the browser should be choosing the full-size 900px version (screenshot: https://cloudup.com/cMXottispDK). Can you tell me any more about the setup you are using when you experienced the 150px images being loaded? What version of iOS/Safari are you using and do you have anything loaded that would modify that browser's native behavior?

Thanks,
Joe

#2 @tonysandwich
8 years ago

Very strange, it was actually on a colleagues phone but I managed to replicate it in Browserstack. Screenshot uploaded with a bit more info, when I'm back in the office Monday I can send more info if required.

Thanks,

Matt

#3 @joemcgill
8 years ago

Thanks for the added info @tonysandwich.

I see that your BrowserStack demo is using iOS8, which didn't have support for srcset with w descriptors. So what you're probably seeing is the 400px image in the src shown on a retina screen. You should be able to add picturefill to polyfill this functionality for older browsers that don't support the new responsive image attributes, but I will warn you that doing so can lead to double downloads in iOS8 (see the "Any Drawbacks" section in this article).

A quick way to test this is to install the RICG Responsive Images plugin, which will add picturefill for you by default. Can you let me know this ends up being correct?

Thanks,
Joe

#4 follow-up: @tonysandwich
8 years ago

No problem.

Yep that seems to have worked, do you think that is something that could be included natively in the next update of wp rather than being something that requires a workaround?

Thanks loads for the assistance, I think the responsive images is a really nice touch to Wordpress.

Matt

#5 in reply to: ↑ 4 @swissspidy
8 years ago

  • Keywords close added; reporter-feedback removed

Replying to tonysandwich:

No problem.

Yep that seems to have worked, do you think that is something that could be included natively in the next update of wp rather than being something that requires a workaround?

Thanks loads for the assistance, I think the responsive images is a really nice touch to Wordpress.

Matt

From the merge proposal:

Based on community feedback, we are choosing not to include a polyfill for non-supporting browsers directly in core. However, we would recommend that themes make use of Picturefill in order to provide responsive image support to the broadest set of users.

I agree that this should be handled by themes or the RICG Responsive Images plugin.

#6 @joemcgill
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from reviewing to closed

Hi @tonysandwich,

@swissspidy is correct. We've chosen not to include the polyfill for this feature in by default based on the feedback we received during the process of bringing responsive images into WordPress. I would recommend loading Picturefill directly through your theme if you want to provide backwards compatibility to older browsers. If you choose not to use a polyfill, browsers will load your images exactly like they would have before this feature was introduced.

Joe

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


8 years ago

#8 @joemcgill
8 years ago

  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

After seeing another report of this behavior on iOS 8, I did a bit more digging and found that Safari in iOS 8 contains a bug that will cause the first src to always be selected. The bug was fixed in webkit here but seems to have never included in mobile Safari in iOS 8.

According to the global stat counter iOS 8 accounts for ~1.5% of all views, so I'm not sure if this is something we need to fix account for in Core or if we wait for browsers to catch up.

#9 @joemcgill
8 years ago

  • Summary changed from New responsive images to Responsive Images: wrong source selected in iOS8

#10 @joemcgill
8 years ago

#35311 was marked as a duplicate.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#12 @joemcgill
8 years ago

  • Milestone changed from Awaiting Review to 4.5

Let's take a look at whether we can ensure the image file in the source src attribute is listed first in srcset in order to fix this issue.

#13 @joemcgill
8 years ago

  • Component changed from Post Thumbnails to Media

#14 @joemcgill
8 years ago

  • Keywords needs-patch added; close removed

#15 @jaspermdegroot
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

In 35030.patch:

  • Made sure that wp_calculate_image_srcset() puts the src image at first position in the srcset.
  • Made the loop in wp_calculate_image_srcset() only check once if the current image is the src image, instead of twice.
  • Updated unit tests: Expected srcsets now have the src image at the first position.
  • Added a unit test to test if the src image is at the first position in the srcset.

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


8 years ago

This ticket was mentioned in Slack in #core-images by jaspermdegroot. View the logs.


8 years ago

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


8 years ago

#19 @joemcgill
8 years ago

@jaspermdegroot looks like the patch needs a refresh, but generally, this looks good. Wondering if we could use an array_unshift() to prepend a source when $is_src is true to avoid using two separate array. We should also add some inline docs explaining why we're forcing the src to the front of the srcset list.

Last edited 8 years ago by joemcgill (previous) (diff)

#20 @jaspermdegroot
8 years ago

In 35030.1.patch: Refreshed the patch and added a bit more information to the inline comment.

@joemcgill - We can't use array_unshift() because we use numerical keys in our array (the image width) and that function will modify them to start counting from zero.

#21 @azaozz
8 years ago

How about we do that only for iOS and "pluck" that URL out of the array after the loop and unshift? Will make a patch this afternoon. The tests may have to be updated too to reflect the different array ordering.

#22 follow-up: @jaspermdegroot
8 years ago

@azaozz

You mean after the filter so we don't have to preserve the keys anymore? That's also an option.
Only thing to keep in mind is that we have to find the src image in the $sources array by its URL. The width from $size_array doesn't match with the image width used as key in $sources when an image is constrained to the content width.

In my patch I added $is_src so we only check once if current image is the src image. I suggest we keep that part to improve performance, even if we don't need that variable anymore for to fix this issue.

I guess we don't have to make any changes in existing tests if we only change the array order for iOS. The test that I added for this ticket could be updated so it sets global $is_iphone (includes iPads) to true.

@azaozz
8 years ago

#23 in reply to: ↑ 22 @azaozz
8 years ago

Replying to jaspermdegroot:

Yes, probably better to do it right in the loop. A simple array_merge() would do it, no need of anything more? Also no need of specific tests for it in that case? It's like testing if array_merge() works.

In 35030.2.patch: simplify 35030.1.patch and use array_merge() to ensure the src URL is first in the srcset only in iOS.

#24 follow-up: @johnbillion
8 years ago

We can't have server side code that changes based on the client, as it's not compatible with page caching. Is there anything we can do client-side?

#25 in reply to: ↑ 24 ; follow-up: @azaozz
8 years ago

Replying to johnbillion:

Right, keep forgetting about page caching. Can drop the $is_iOS stuff and do that for all srcset then, like the other patches. As far as I gather, all other browsers would still work properly. It's pity to have to mess with the order just to avoid an old iOS bug, but...

That would mean some of the test will have to be corrected too.

Last edited 8 years ago by azaozz (previous) (diff)

#26 in reply to: ↑ 25 @joemcgill
8 years ago

Replying to azaozz & johnbillion:

I agree about the page caching. If we do mess with the order, it will need to happen on the server side because anything we do in JS on the client side would cause a double download due to the preloader beginning to fetch the resource before JS is ready.

Alternately, we could leave things as since iOS Safari 8 usage is below 1%, according to StatCounter GlobalStats. Anyone who needs to support older versions of iOS can add a polyfill for srcset. It may not be worth the tradeoff of adding more overhead to a display filter to support such a small user base.

#27 @jaspermdegroot
8 years ago

Replying to @azaozz, @johnbillion, and @joemcgill:

Good point about page caching. So let's indeed not do something for iOS(8) specific.

Just to put things in perspective: 1% of ~3.5 billion internet users is still 35 million people. It's a bigger market share than whole Windows Phone. These are probably mostly people that have an iThing that can't be upgraded to iOS9 so I think the number won't decrease very fast either. If it was about iOS8 users not benefiting from an enhancement it would be a different story, but here we are breaking something that used to be ok. So I really think we should fix this.

In 35030.3.patch: Instead of using two arrays, prepend the src image to the $sources array. Compared to @azaozz his patch I used the + operator instead array_merge() because of the difference in how duplicate keys are handled.

#28 @jaspermdegroot
8 years ago

There is a problem with the _get_image_size_array_from_name() helper function in the test. Working on a fix for that now and will submit an updated patch soon.

@joemcgill
8 years ago

#29 @joemcgill
8 years ago

35030.4.patch fixes the failing tests after 35030.3.patch by replacing _get_image_size_array_from_name() with _get_image_size_array_from_meta(), passing the same meta that's being used to calculate srcset attributes, instead of hard coding assumed image widths into our tests. This also cleans up a few formatting issues.

#30 @jaspermdegroot
8 years ago

Patch 35030.4 looks good to me.

#31 @azaozz
8 years ago

Yep, the changes in 35030.4.patch look good, and array() + array() is better there than array_merge().

However the changes to the tests need more.

  1. Not sure what is being tested with test_wp_calculate_image_srcset_src_first(). It is not needed to specifically test if the image src is first in srcset. That is being tested by all other srcset tests already.

Another big problem there is that some of the test data is randomized. This means it may introduce intermittent failures. As we are testing array order it may fail because of shuffle(). That's really bad for testing :) If there are 10 cases, all need to be tested every time so we can see which one fails.

  1. There is a lot of repetition in most of the srcset testing functions when generating comparison strings. It would be good to "DRY" that and include the functionality from the new _src_first() helper. This is pretty much the same in 4 places:
    $year_month = date('Y/m');
    $uploads_dir = 'http://' . WP_TESTS_DOMAIN . '/wp-content/uploads/';
    
    // Set up test cases for all expected size names.
    $intermediates = array( 'medium', 'medium_large', 'large', 'full' );
    
    foreach ( $_wp_additional_image_sizes as $name => $additional_size ) {
    	if ( ! $_wp_additional_image_sizes[$name]['crop'] || 0 === $_wp_additional_image_sizes[$name]['height'] ) {
    		$intermediates[] = $name;
    	}
    }
    
    $expected = "";
    
    foreach ( $image_meta['sizes'] as $name => $size ) {
    	// Whitelist the sizes that should be included so we pick up 'medium_large' in 4.4.
    	if ( in_array( $name, $intermediates ) ) {
    		$expected .= $uploads_dir . $year_month . '/' . $size['file'] . ' ' . $size['width'] . 'w, ';
    	}
    }
    
    $expected .= $uploads_dir . $image_meta['file'] . ' ' . $image_meta['width'] .'w';
    

Of course, that is a test change so it can be added in RC too.

Last edited 8 years ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-images by azaozz. View the logs.


8 years ago

#33 @azaozz
8 years ago

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

In 37034:

Responsive Images: the src of the image has to be first in the srcset, because of a bug in iOS8. Update the unit tests to reflect the changes.

Props jaspermdegroot, joemcgill, azaozz.
Fixes #35030.

#34 @jaspermdegroot
8 years ago

@azaozz

Yeah, you're completely right about test_wp_calculate_image_srcset_src_first(). I wrote it right after fixing the bug not thinking about the fact that most existing tests would fail and that fixing them results in testing this. And I also see the problems with using that shuffle now. Although I think it wouldn't cause random failure but more a false positive.
Anyways, it's a bad/useless test. Thanks for the feedback! I learned something :)

#35 @ocean90
8 years ago

In 38257:

About Page: Enhance responsive images.

  • Add srcset and sizes to the mobile image for streamlined updates.
  • Modify the order of image candidate strings in each srcset to address a bug in iOS8 where the first candidate will always be selected when using w descriptors, see #35030.

Props joemcgill.
Fixes #37246.

#36 @ocean90
8 years ago

In 38258:

About Page: Enhance responsive images.

  • Add srcset and sizes to the mobile image for streamlined updates.
  • Modify the order of image candidate strings in each srcset to address a bug in iOS8 where the first candidate will always be selected when using w descriptors, see #35030.

Merge of [38257] to the 4.6 branch.

Props joemcgill.
See #37246.

Note: See TracTickets for help on using tickets.