WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 11 months ago

Last modified 10 months ago

#38395 closed defect (bug) (fixed)

Twenty Seventeen: iPad mini: Images on the front page badly pixelated

Reported by: alex27 Owned by: laurelfulford
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: Cc:

Description

On iPad mini 2 retina in landscape mode both custom header image and panel images appear to be enlarged and badly pixelated:
https://cloud.githubusercontent.com/assets/4550351/19464532/cc75e8aa-9548-11e6-9b37-1372d638361c.png

https://cloud.githubusercontent.com/assets/4550351/19464535/d0a07954-9548-11e6-9a68-4a35dc02ef34.png

In portrait mode images display correctly:
https://cloud.githubusercontent.com/assets/4550351/19464542/e407c6f0-9548-11e6-9488-37c329ff3478.png

Original size of featured image above is 2400x1600.

Featured images on single posts/pages look good as well.

Attachments (6)

38395.patch (2.4 KB) - added by laurelfulford 12 months ago.
38395.1.patch (711 bytes) - added by laurelfulford 12 months ago.
Increasing screen size where background-attachment fixed is applied.
38395.2.patch (2.0 KB) - added by laurelfulford 11 months ago.
38395.3.patch (1.9 KB) - added by joemcgill 11 months ago.
38395.4.patch (2.0 KB) - added by joemcgill 11 months ago.
38395.5.patch (2.4 KB) - added by laurelfulford 11 months ago.

Download all attachments as: .zip

Change History (35)

#1 @karmatosed
12 months ago

Discussion over here: https://github.com/WordPress/twentyseventeen/issues/450 - ticket being brought over as part of merge.

#2 @laurelfulford
12 months ago

  • Keywords has-patch added; needs-patch removed

background-attachment: fixed is causing this weird issue.

In 38395.patch, I added a check to see if the theme is being loaded on an iOS device, and used that to change a CSS class. The applied CSS class is then used to change the background-attachment styles.

#3 @davidakennedy
12 months ago

  • Keywords needs-refresh added; has-patch removed

Nice work around here @laurelfulford! I'd like to avoid user agent sniffing. It's a slippery slope.

It seems this is disabled in some mobile browsers for performance reasons. See: https://twitter.com/paul_irish/status/306818591196602368

I'm wondering if there's another solution? Maybe with the pseudo element, using it to be the thing that's fixed?

#4 @davidakennedy
12 months ago

  • Owner set to laurelfulford
  • Status changed from new to assigned

@laurelfulford
12 months ago

Increasing screen size where background-attachment fixed is applied.

#5 @laurelfulford
12 months ago

This is trickier to fix than I expected!

38395.1.patch isn't ideal, but it increases the breakpoint where the background-attachment: fixed is applied to the front page images. Doing this removes the fixed backgrounds and the issue from all iOS devices, but the downside is that it also removes the fixed backgrounds from smaller laptops.

This ticket was mentioned in Slack in #core-themes by davidakennedy. View the logs.


12 months ago

#7 @LittleBigThing
12 months ago

Hi,

Raising the min-width is indeed necessary, but it needs to be relatively high to cover the 12.9 inch iPad Pro. This mean that a lot of users with narrowor desktops will experience the theme without the fixed background image.

I was thinking of having an additional CSS rule that is specific for iOS devices or at least Apple devices. I could only think of the resolution (in dpi).

Couldn't we throw in an additional test for the media feature resolution? This is simple CSS but it could narrow down the use of background-attachment: scroll...

Something like:

@media only screen and (max-width: 85.45em) and ( (resolution: 264dpi) or (resolution: 326) or (resolution: 401) ) {
    .panel-image {
        background-attachment: scroll; 
    }
}

I am not sure how well this CSS feature is supported and if it needs the min or max prefix. It seems that we could also use only the prefixed (-webkit-) version and maybe do something with the non-standard rule min/max-device-pixel-ratio (?).
But I could be totally wrong, sorry... (did not have the time to test it yet).

Cheers,
Csaba

#8 @laurelfulford
11 months ago

Thanks for the suggestion, @LittleBigThing!

I tested adding a resolution to the media query, and the one downside I found is that it's still not possible to distinguish between iOS devices and retina screens that way. So it makes it possible to keep the fixed backgrounds on some smaller laptops, but not all.

I did a bit more searching in this vein, and found a post about targeting iPad Pros using really specific device height and width queries: https://medium.com/connect-the-dots/css-media-queries-for-ipad-pro-8cad10e17106

In 38395.2.patch, I lowered the screen size where background-attachment: fixed is applied, and added an iPad Pro-sized media query to override it. This exact setup still feels a bit hacky to me, but less heavy handed than the user agent sniffing. Suggestions for improvements welcome!

#9 @laurelfulford
11 months ago

  • Keywords has-patch added; needs-refresh removed

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


11 months ago

This ticket was mentioned in Slack in #core-themes by davidakennedy. View the logs.


11 months ago

#12 @davidakennedy
11 months ago

We talked about this ticket in the bug scrub today and in Core Themes in Slack. @laurelfulford and I decided to go with 38395.1.patch because it's the simplest and less targeted. Meaning, it's remotely possible this could be changed in the future should the landscape change.

See: https://wordpress.slack.com/archives/core-themes/p1478651088000264

Last edited 11 months ago by davidakennedy (previous) (diff)

#13 @davidakennedy
11 months ago

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

In 39176:

Twenty Seventeen: Fix badly pixelated images on iOS devices

The CSS property background-attachment: fixed is disabled in some mobile browsers for performance reasons. So here, the fix increases the breakpoint where the background-attachment: fixed is applied to the front page images. Doing this removes the fixed backgrounds and the issue from all smaller screens, but the downside is that it also removes the fixed backgrounds from smaller laptops. It's also possible future devices could fall into this breakpoint, but this seems to be the most practical solution.

Props laurelfulford.

Fixes #38395.

#14 @helen
11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The fixed background effect is lost for a lot of normal desktop sizes (1367px and under), which is a real shame because it's one of the things that's really initially attractive about Twenty Seventeen. Since the committed approach is already a workaround, it probably makes sense to dial in the media queries to be really specific about devices targeted (with explanatory inline comments, naturally).

#15 @LittleBigThing
11 months ago

Hi,
Maybe give it a try combining it with pixel density (dpi) based media queries? (comment #7)
Macbooks' and iOS devices' screens have different screen pixel densities (https://www.tekrevue.com/retina-display-comparison/).
Greetings,
Csaba

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


11 months ago

#17 @karmatosed
11 months ago

It would be great to get a more targeted patch - is anyone able to rustle that up? If not, I can investigate tomorrow a little more myself before RC. I agree with @helen that we should avoid losing the effect on so many desktops.

#18 @joemcgill
11 months ago

  • Keywords needs-testing added

I'm not sure if feature detection will work here, but 38395.3.patch adds a test to see if background-attachment: fixed is supported before setting the style.

@joemcgill
11 months ago

#19 @joemcgill
11 months ago

Was able to do some testing of 38395.3.patch on an iOS simulator, and it looks like this type of feature detection won't work because Safari still reports that 'background-attachment: fixed' is supported, even though it's being disabled under the hood. I'm usually against UA sniffing, but in this case, I think adding UA sniffing like what is in 38395.patch is preferable over trying to use a media query as a proxy for filtering out a specific browser/OS issue.

@joemcgill
11 months ago

#20 @joemcgill
11 months ago

38395.4.patch sticks with the feature test approach from 38395.3.patch but includes the checkiOS() test from 38395.patch when determining support for fixed backgrounds. I also returned the media query for applying this style to min-width: 55em which was the value before the [39176] workaround.

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


11 months ago

#22 @celloexpressions
11 months ago

UA sniffing seems like the clear choice here since we can't reliably do feature detection, and the alternative is a major design regression for a much broader set of devices.

#23 follow-up: @davidakennedy
11 months ago

Thanks everyone for the continued work and thoughts on this. I'm glad to have another chance to make this better.

@joemcgill I tested the patch heavily in current desktop browsers, plus devices in Browserstack. It works well overall, and I think this approach is better than just adjusting media queries. It doesn't make sense to punish other devices that may support this feature.

One thing I did notice: in testing in Browserstack on a Nexus 7 tablet with Chrome 49 – the function returned true, which I wouldn't expect it would. Is that what false reporting the Stack Overflow issue talked about?

Even so, I'm still up for going this route since it seems like the best option.

#24 in reply to: ↑ 23 @joemcgill
11 months ago

Replying to davidakennedy:

One thing I did notice: in testing in Browserstack on a Nexus 7 tablet with Chrome 49 – the function returned true, which I wouldn't expect it would. Is that what false reporting the Stack Overflow issue talked about?

I'm not sure if the Nexus 7 issue you're seeing is specific to Browserstack or not since I don't have a device to test on, but I'm seeing the same behavior in Browserstack. I'm not sure if it's because the media query isn't applying or if there's something else going on with the test, but I'm not seeing any visual problems with the behavior.

If we do find that there are other browser quirks that need to be addressed, those could be handled much like the UA check for iOS devices within supportsFixedBackground().

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


11 months ago

#26 @helen
11 months ago

I tested the patch + trunk + trunk before the original commit on both a Nexus 7 and an iPad Mini (in real life), patch works correctly for me in practical user terms. My only question left is if the media query needs to stay so wide at 55em given feature detection, and if there should be a comment there indicating the reasoning behind whatever value that becomes.

#27 @laurelfulford
11 months ago

In 38395.5.patch I updated the patch to change the breakpoint where background-attachment: fixed is applied from 55em to 48em. That's the same breakpoint where the panel image is set to be 100% of the screen height, along with most of the other 'large screen' styles. It seemed like a natural fit for the effect to me, but it could go lower if that makes sense.

Also included a comment explaining the current location :)

#28 @davidakennedy
11 months ago

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

In 39297:

Twenty Seventeen: Adds background-attachment: fixed; to devices that should support it

iOS disables this feature under the hood, but it also distorts the images – unlike other mobile devices that don't support it. So this adds a check for both background-attachment: fixed support or whether it’s an iOS device - passing it adds the class background-fixed which is used to add the proper styles.

It also lowers the media query so the parallax-like style is present on a wider range of screens since this bug can be better targeted and avoided. In this way, screens that aren't the offending devices aren't punished merely based on screen size.

Props joemcgill, laurelfulford, helen.

Fixes #38395.

#29 @mapk
10 months ago

I still get the distorted image on iOS devices here: https://make.wordpress.org/core/features/ when I preview the theme on my iOS device.

Note: See TracTickets for help on using tickets.