The Portrait Immersive Main Media Width Problem #2205

oliverlloyd · 2020-12-07T12:45:39Z

What does this change?

This is a proposal for discussion around a problem where immersive main media images are being chosen by the browser poorly on portrait devices. This happens because we size the main media content based on 100vh but for portrait aspect ratios this results in us loading much smaller images than we need.

Before	After

What was done in Frontend?

This problem has already been raised, discussed and solved 5 years ago, see:

guardian/frontend#12811
and then
guardian/frontend#12847

That work resulted in this excellent code:

@immersivePortraitSource(largestImage: model.ImageAsset, hidpi: Boolean = false) = {
    <source media="@if(hidpi) {
        (orientation: portrait) and (-webkit-min-device-pixel-ratio: 1.25), (orientation: portrait) and (min-resolution: 120dpi)
    } else {
        (orientation: portrait)
    }"

    sizes="@{100 * largestImage.ratioDouble}vh"
    @* This adds 15 src options to the srcset from 500 to the largest image width incrementing by 250px *@
    srcset="@{(500 to largestImage.width by 250)
        .toList
        .map(width => ImgSrc.srcsetForProfile(views.support.ImageProfile(width = Some(width)), picture, hidpi).asSrcSetString)
        .mkString(", ")}" />
}

where ratioDouble is

lazy val ratioDouble: Double = width.toDouble / height

Why are we not doing the same thing?

We must ensure that this work is not lost when transitioning to image-rendering but before we adopt the IR picture tag approach, this PR applies a simpler method via the img tag.

So instead of using an imageRatio based on an images height and width to calculate the correct width to set in sizes this PR is simply using the viewport height and an assumed image ration of 5:3 to decide what width to ask for.

Shouldn't we be more explicit?

There's a case for being more chill. The argument goes along the lines of: it's hard to get this right and when you aim for absolute perfection it's very easy to make mistakes because the logic is complex. Keeping it chill makes it easy to see what's going on.

Of course, we can do better and we should. Even small perf gains at scale are worth the extra complexity. But a staged approach to getting there, starting off simple, might be nice?

oliverlloyd · 2020-12-07T12:47:22Z

@paperboyo @sndrs @JamieB-gu @mxdvl thoughts?

github-actions · 2020-12-07T12:50:34Z

Size Change: +1.07 kB (0%)

Total Size: 717 kB

Filename	Size	Change
`dist/frontend.server.js`	223 kB	+987 B (0%)
`dist/react.js`	113 kB	+43 B (0%)
`dist/react.legacy.js`	116 kB	+44 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`dist/atomIframe.js`	1.21 kB	0 B
`dist/atomIframe.legacy.js`	1.21 kB	0 B
`dist/dynamicImport.js`	1.96 kB	0 B
`dist/dynamicImport.legacy.js`	2.01 kB	0 B
`dist/embedIframe.js`	1.21 kB	0 B
`dist/embedIframe.legacy.js`	1.22 kB	0 B
`dist/ga.js`	3.01 kB	0 B
`dist/ga.legacy.js`	3.47 kB	0 B
`dist/GetMatchStats.js`	3.23 kB	0 B
`dist/GetMatchStats.legacy.js`	3.32 kB	0 B
`dist/lotame.js`	1.14 kB	0 B
`dist/lotame.legacy.js`	1.15 kB	0 B
`dist/MostViewedFooterData.js`	5.63 kB	0 B
`dist/MostViewedFooterData.legacy.js`	5.84 kB	0 B
`dist/MostViewedRightWrapper.js`	4.08 kB	0 B
`dist/MostViewedRightWrapper.legacy.js`	4.3 kB	0 B
`dist/newsletterEmbedIframe.js`	1.14 kB	0 B
`dist/newsletterEmbedIframe.legacy.js`	1.15 kB	0 B
`dist/OnwardsLower.js`	8.8 kB	0 B
`dist/OnwardsLower.legacy.js`	9.06 kB	0 B
`dist/OnwardsUpper.js`	12.8 kB	0 B
`dist/OnwardsUpper.legacy.js`	13.2 kB	0 B
`dist/ophan.js`	6.94 kB	0 B
`dist/ophan.legacy.js`	6.93 kB	0 B
`dist/sentry.js`	22.8 kB	0 B
`dist/sentry.legacy.js`	22.7 kB	0 B
`dist/shimport.js`	3.21 kB	0 B
`dist/shimport.legacy.js`	3.21 kB	0 B
`dist/SignInGateMain.js`	1.87 kB	0 B
`dist/SignInGateMain.legacy.js`	1.89 kB	0 B
`dist/vendors~braze-web-sdk-core.js`	34.9 kB	0 B
`dist/vendors~braze-web-sdk-core.legacy.js`	34.9 kB	0 B
`dist/vendors~guardian-braze-components.js`	17.3 kB	0 B
`dist/vendors~guardian-braze-components.legacy.js`	17.4 kB	0 B

_{compressed-size-action}

JamieB-gu · 2020-12-07T13:17:40Z

@paperboyo @sndrs @JamieB-gu @mxdvl thoughts?

Interestingly on AR it looks like we've come up with a similar solution to the frontend one you described above (i.e. using (imageRatio * 100)vh). If you go to the apps-rendering version of the article you're using above (available via Teleporter), you should see one of the larger images being loaded.

On a related note, I've also noticed that our HeaderImage component in question is still using the pre-IR implementation, we'll need to migrate that.

but before we adopt the IR picture tag approach, this PR applies a simpler, more arbitrary method

Are you saying this solution is temporary until you introduce IR into DCR?

liywjl

Really comprehensive! great work!

paperboyo · 2020-12-07T13:37:34Z

Related to (as Lightbox would be another portrait situation): guardian/frontend#12216 (comment)

but before we adopt the IR picture tag approach, this PR applies a simpler, more arbitrary method via the img tag

👏

Of course, we can do better and we should

👏 👏

Apart from using Picture (being able to prevent phones from downloading higher res for desktops is most important in immersive, as we currently satisfy quite high quality desktop displays), I wonder what more we could do. If we would to forego being able to resize the browser (and a picture following fluidly) only on phones and be OK with another request when readers change orientation (how often?), maybe we could use the resizer to actually cut off sides of these (with some leeway for differently ratioed phone displays)? That should shave a lot for phone-only downloads. Unless I haven’t thought of something?

Whenever we gave any thought to Grid supporting “mobile” crops explicitly, we always ended up thinking that setting point(s) of interest would be preferable to actually providing (and requiring users to set) two different physical crops (art direction-like). But whatever we will choose one day (if we ever do it), currently all these are just always cropped off equally relative to the image centre.

oliverlloyd · 2020-12-07T13:43:58Z

Are you saying this solution is temporary until you introduce IR into DCR?

Yes, we've taken the approach in DCR of seeting some short term goals of improving image quality to give readers a better experience - which we're doing by using an img tag and things like this PR. But we maintain our medium to long term goal of migrating to image-rendering

oliverlloyd · 2020-12-07T13:50:46Z

use the resizer to actually cut off sides of these

This sounds like a good trade off. Small images giving us a performance win vs. unexpected behaviour when resizing the screen. +1 for the perf. gain here, screen resizing must be very rare (outside the second Floor).

An automated solution like this would obviously be easier that asking people to take additional steps in Grid but there's possibly a lot of benefit to having a human choose where that portrait crop happens? I guess though that Viewer is the sanity check here so any strange mobile cropping will be seen at this stage.

paperboyo · 2020-12-07T14:15:23Z

screen resizing must be very rare

I think so. Especially on mobile!

But this is really unrelated directly to the second point (sorry for introducing confusion). Currently all images coming from Grid are the same, just of different size. DCR/AR/IR could unilaterally decide to cut off invisible sides (if only for mobile). And nobody would loose anything and only gain some pocket change from smaller phone bill (even orientation change probably fires off another request?).

Regarding us supporting aided multiple-ratio cropping for fluid scenarios one day: yes, users now sometimes crop source images in Grid several times, before they arrive at the satisfactory result on different ratios (doesn’t help that they all seem to think immersive must be 5:3 for some unknown reason!). I only mentioned it, because then we would need stop this arbitrary cropping off sides relative to the centre, described in the previous paragraph, and start doing it relative to these point(s) of interest that arrived through CAPI. I give us 5 years, though, so a non-issue.

oliverlloyd · 2020-12-07T14:23:26Z

I give us 5 years, though

👋 to anyone reading this in 5 years

oliverlloyd · 2020-12-07T16:08:09Z

Low quality images on DCR

mxdvl

@paperboyo @sndrs @JamieB-gu @mxdvl thoughts?

Could we use the same (orientation: portrait) and use that set the size to 167vh, assuming 5:3 is the most common ratio? It seems that this change would limit the resolution of images to 1300px even though they stretch to the whole viewport on large devices.

(orientation: portrait) 167vw,
100vw

mxdvl · 2020-12-07T20:46:32Z

src/web/components/Img.tsx

+/**
+ *       mobile: 320
+ *       mobileMedium: 375
+ *       mobileLandscape: 480


how accurate is that, nowadays?

Probably not very, the name is certainly confusing for me. We design to it though so it would be hard to remove.

The names are somewhat arbitrary. We need to specify breakpoints somewhere - we could probably name them 1-7 if we wanted to 🤷‍♂️.

mxdvl · 2020-12-07T20:53:36Z

src/web/components/Img.tsx

+                ? `
+                    (min-width: ${breakpoints.leftCol}px) 1600px,
+                    (min-width: ${breakpoints.desktop}px) 1900px,
+                    (min-width: ${breakpoints.tablet}px) 1900px,
+                    (min-width: ${breakpoints.phablet}px) 1600px,
+                    (min-width: ${breakpoints.mobileLandscape}px) 1300px,
+                    (min-width: ${breakpoints.mobileMedium}px) 900px,
+                    1300px
+                `


Suggested change

? `

(min-width: ${breakpoints.leftCol}px) 1600px,

(min-width: ${breakpoints.desktop}px) 1900px,

(min-width: ${breakpoints.tablet}px) 1900px,

(min-width: ${breakpoints.phablet}px) 1600px,

(min-width: ${breakpoints.mobileLandscape}px) 1300px,

(min-width: ${breakpoints.mobileMedium}px) 900px,

1300px

`

? `

(orientation: portrait) 167vw,

100vw

`

Just to be clear, I think (orientation: portrait) 167vw above should read as (orientation: portrait) 167vh (vh) so I'm leaving this commit suggestion alone and I'll push this change up manually

Just to note that while it’s definitely true that most main medias are 5:3, there is nothing stopping them being any ratio. As I understand, this (167vh) will control the number of steps, so if it’s gonna be off, nothing terrible will happen. Still, curious what Kylie’s gonna look like.

oliverlloyd · 2020-12-07T22:05:26Z

It seems that this change would limit the resolution of images to 1300px even though they stretch to the whole viewport on large devices

I think that the default to 1300px is an oversight, that should indeed be 100vw. Great spot!

oliverlloyd · 2020-12-07T23:00:01Z

Could we use the same (orientation: portrait) and use that set the size to 167vh

This is an inspired idea. It's so simple but does exactly what we want! 👏

The reason that I used varying sizes at different breakpoints is because device screen ratios actually vary quite a bit but by utilizing the viewport height to set the desired width you nicely sidestep that problem. I also ❤️ how the code is so much simpler using this approach.

However, I tried it out locally and it seems to be returning the expected images in Chrome but Firefox is stubonly set on always returning the largest image so I want to investigate this some more.

paperboyo · 2020-12-07T23:02:29Z

Firefox is stubonly set

Firefox again? Maybe it’s somehow related to this?

oliverlloyd · 2020-12-07T23:15:01Z

Firefox again? Maybe it’s somehow related to this?

I think you're right. I think it was using an image from it's cache rather than fetching a new one. Seems to have fixed itself now though after I disabled the cache 🎉 . I've read about these kind of problems when testing images like this. Each browser has implemented the spec in their own way and there are some factors which can make things unpredicable (for instance, I think Chrome will choose a different image based on network conditions).

paperboyo · 2020-12-07T23:19:33Z

Oh, boo. I thought there is something in common to both behaviours of Firefox. The other one still consistently chooses 445px in place of 620px cache or not. Nevermind, here was hoping, this is unrelated.

mxdvl

I like the clarity around portrait mode triggering a large image 😉

src/web/components/Img.tsx

Co-authored-by: Gareth Trufitt <[email protected]>

JamieB-gu · 2020-12-08T14:11:22Z

maybe we could use the resizer to actually cut off sides of these

Is this the "Art Direction" problem?

Could we use the same (orientation: portrait)

How is orientation: portrait defined? What happens if the user is on a desktop browser using a narrow window? Or a landscape iPad in split screen?

paperboyo · 2020-12-08T14:23:00Z

Is this the "Art Direction" problem?

Not AFAIU. Kinda opposite of it. Art Direction means you are explicitly providing a different image for mobile. Here, knowing upfront that you are displaying this image on mobile, you just blindly cutting off the parts you know aren’t going to show up. And you keep quiet about it in front of your Art Director, coz you know he won’t even notice. IOW, the image will look exactly the same but will weigh less. Needs some leeway to satisfy different phone ratios. And probs mobile only, coz on desktop Art Director may accidentally resize the window and notice you not telling him.

Don't choose immersive main media widths based on viewport

8ea77fc

oliverlloyd requested review from gtrufitt, jlieb10, liywjl and shtukas as code owners December 7, 2020 12:45

oliverlloyd changed the title ~~Don't choose immersive main media widths based on viewport~~ The Portrait Immersive Main Media Width Problem Dec 7, 2020

liywjl approved these changes Dec 7, 2020

View reviewed changes

mxdvl reviewed Dec 7, 2020

View reviewed changes

Use orientation portrait and vh to work out width

8c99690

mxdvl approved these changes Dec 8, 2020

View reviewed changes

gtrufitt reviewed Dec 8, 2020

View reviewed changes

src/web/components/Img.tsx Outdated Show resolved Hide resolved

gtrufitt approved these changes Dec 8, 2020

View reviewed changes

oliverlloyd and others added 4 commits December 8, 2020 11:13

Update src/web/components/Img.tsx

6cf9201

Co-authored-by: Gareth Trufitt <[email protected]>

Updated comment for 167vh approach

6aaf1d5

Reduce Chromatic check threashold for these image stories

28520b8

Increase threshold for these images

e39fa8a

oliverlloyd merged commit c9b84e3 into main Dec 8, 2020

oliverlloyd deleted the oliver/immersive-image-refactor branch December 8, 2020 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Portrait Immersive Main Media Width Problem #2205

The Portrait Immersive Main Media Width Problem #2205

oliverlloyd commented Dec 7, 2020 •

edited

Loading

oliverlloyd commented Dec 7, 2020

github-actions bot commented Dec 7, 2020 •

edited

Loading

JamieB-gu commented Dec 7, 2020 •

edited

Loading

liywjl left a comment

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

mxdvl left a comment •

edited

Loading

mxdvl Dec 7, 2020

oliverlloyd Dec 7, 2020

JamieB-gu Dec 8, 2020

mxdvl Dec 7, 2020

oliverlloyd Dec 7, 2020

paperboyo Dec 8, 2020

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020 •

edited by mxdvl

Loading

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

paperboyo commented Dec 7, 2020

mxdvl left a comment

JamieB-gu commented Dec 8, 2020

paperboyo commented Dec 8, 2020 •

edited

Loading

The Portrait Immersive Main Media Width Problem #2205

The Portrait Immersive Main Media Width Problem #2205

Conversation

oliverlloyd commented Dec 7, 2020 • edited Loading

What does this change?

What was done in Frontend?

Why are we not doing the same thing?

Shouldn't we be more explicit?

oliverlloyd commented Dec 7, 2020

github-actions bot commented Dec 7, 2020 • edited Loading

JamieB-gu commented Dec 7, 2020 • edited Loading

liywjl left a comment

Choose a reason for hiding this comment

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

mxdvl left a comment • edited Loading

Choose a reason for hiding this comment

mxdvl Dec 7, 2020

Choose a reason for hiding this comment

oliverlloyd Dec 7, 2020

Choose a reason for hiding this comment

JamieB-gu Dec 8, 2020

Choose a reason for hiding this comment

mxdvl Dec 7, 2020

Choose a reason for hiding this comment

oliverlloyd Dec 7, 2020

Choose a reason for hiding this comment

paperboyo Dec 8, 2020

Choose a reason for hiding this comment

oliverlloyd commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020 • edited by mxdvl Loading

paperboyo commented Dec 7, 2020

oliverlloyd commented Dec 7, 2020

paperboyo commented Dec 7, 2020

mxdvl left a comment

Choose a reason for hiding this comment

JamieB-gu commented Dec 8, 2020

paperboyo commented Dec 8, 2020 • edited Loading

oliverlloyd commented Dec 7, 2020 •

edited

Loading

github-actions bot commented Dec 7, 2020 •

edited

Loading

JamieB-gu commented Dec 7, 2020 •

edited

Loading

mxdvl left a comment •

edited

Loading

oliverlloyd commented Dec 7, 2020 •

edited by mxdvl

Loading

paperboyo commented Dec 8, 2020 •

edited

Loading