Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preload the Hero Image #202

Closed
wants to merge 1 commit into from
Closed

Conversation

bstopp
Copy link

@bstopp bstopp commented Mar 31, 2023

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 31, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 31, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: Something went wrong. PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 31, 2023

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@bstopp
Copy link
Author

bstopp commented Mar 31, 2023

If anyone knows a better way to force the browser to process the picture element, without it being visible, i'm all for it. But i couldn't figure out how to make that event take place.

@ramboz
Copy link
Collaborator

ramboz commented Apr 4, 2023

You could create a specific preload Link header for the pages via the global headers spreadsheet. But ideally, we should even have the pipeline do that for us based on the image metadata for the page.

@davidnuescheler @tripodsan @kptdobe what do you guys think?

@bstopp
Copy link
Author

bstopp commented Apr 4, 2023

That assumes the image metadata is the same as what needs to be preloaded for hero.

Also the preload image need for mobile is diff than desktop, and diff from the metadata.

Take for example the PR URL, this is the og:image metadata:

https://feat-preload-hero--helix-project-boilerplate--bstopp.hlx.page/media_12637fbb67cddc5d293b20975e89028d919270ac0.jpeg?width=1200&format=pjpg&optimize=medium

But this is what needs to be pre-loaded on mobile:

https://feat-preload-hero--helix-project-boilerplate--bstopp.hlx.page/media_12637fbb67cddc5d293b20975e89028d919270ac0.jpeg?width=750&format=webply&optimize=medium

The format and width are different than the default that is out of the pipeline.

@ramboz
Copy link
Collaborator

ramboz commented Apr 5, 2023

The Link headers seem to support media queries, if I understand the discussion here correctly w3c/preload#18 and the spec https://html.spec.whatwg.org/multipage/links.html#link-type-preload. But it does require that those links be manually set in the spreadsheet.

If this does not work, then we'd be indeed stuck with only supporting a fixed URL. The <link> meta prop does support the media prop for sure, though, but I don't think the pipeline would let you set this from the document metadata directly. And if you have to do it in JS, we are back to your solution.

Now the complaint is mostly on the mobile PSI score, so in theory, we could have it hardcoded in the metadata, or the document, manually for now. You would likely get a warning on desktop that you asked to preload something that wasn't used in the first 3s though… but since bandwidth is usually better on desktop, this is less of an issue. It's a trade-off in the end if you really need to "over-optimize" for mobile.

But before even trying further, I would try to set it manually once to see the PSI score difference, maybe using @davidnuescheler 's https://thinktanked.org/tools/deep-psi/deep-psi.html so you get a good average. I'm not sure it would really change the score much anyways.

@bstopp
Copy link
Author

bstopp commented Apr 5, 2023

I think that there's some potentially incorrect assumptions with using the pipeline to pick the image:

First possible option: the pipeline picks either the first image, or the metadata defined image for the og:image tag. Picking only this value doesn't necessarily correlate to the image that needs to be pre-loaded (Hero or otherwise).

So moving to an option where we find the first image on the page to set it to be preloaded. Assuming the pipeline performs the same logic and generates a list of srcset variations to set them to all to be preloaded, with appropriate media query definitions. This also, does not necessarily correlate to the image that needs to be preloaded (hero or otherwise)

It's only the image that is either in the LCP block, be it the Hero or otherwise needs to be preloaded.

In the proposed solution, we're providing a function as a way to allow for arbitrary preloading of a desired image. Instead of assuming/presuming that the first image or the metadata image needs to be preloaded.

I suppose the PR is improperly named as i'm not limiting only the Hero to be pre-loaded. But as the Boilerplate's hero is what needs to be pre-loaded, i went with that description.

@ramboz
Copy link
Collaborator

ramboz commented Apr 11, 2023

I think the Link headers, <link> tags in the <head> and fetchpriority on the image are the only options we have to control the loading sequence. Each approach will have pros and cons, and it will likely be a project-specific implementation detail to squeeze the last few ms out of the LCP.

I also remember from discussions with @davidnuescheler that those kind of optimizations tend to work well for desktop, but ended impacting mobile negatively on several test projects as the bandwidth is more limited and the overhead of handling several requests in parallel was larger than the gain from preloading earlier. At least on "lab" measurements we did via PSI. It would need to be confirmed to "field" data from CrUX to be sure.

@rofe
Copy link
Contributor

rofe commented Apr 27, 2023

Unfortunately PSI recommendations aren't always correct or useful...

@bstopp Have you seen this becoming an actual issue in a real life scenario? In the boilerplate project, I can't see it moving the needle at all, on the contrary:

main

image

fork

image

@davidnuescheler
Copy link
Contributor

@bstopp i usually force the load of hidden image is usually done by adding loading="eager".

@davidnuescheler
Copy link
Contributor

i am not clear what the og:image has anything to do with this...

@bstopp
Copy link
Author

bstopp commented Apr 27, 2023

Maybe I misunderstand how it works, but this is what i thought was the case (all my assumptions/understandings, again which may be wrong):

Because the selection of the LCP image is a dynamic selection by the browser from a srcset within a picture, it doesn't start loading the image until it picks the correct one based on supported types (webp/jpg) and media query.

This happens when the LCP is finally 'visible' to the browser and in the DOM ("loaded").

But you can force the image to download early, thereby being available sooner, by telling the browser to preload it. But you have to be specific about which image. You have to pick the same one the browser will pick.

i usually force the load of hidden image is usually done by adding loading="eager".

I don't think that the OOTB Hero block does this? Either way, i consistently get the error message as shown on the referenced issue. Once i force the browser to preload the image, sometimes i get the error, sometimes i dont. (Though i suspect this is more to do with an oddity in the Lab test)

In the boilerplate project, I can't see it moving the needle at all, on the contrary:

No, I haven't seen it have any meaningful impact on the score itself. But that the report says "You should fix this", to me in something to consider.

@bstopp
Copy link
Author

bstopp commented Apr 27, 2023

No, I haven't seen it have any meaningful impact on the score itself. But that the report says "You should fix this", to me in something to consider.

I should clarify : i haven't seen it show any impact on the Boilerplate score. I'd have to remove it from my projects to test those contexts.

@davidnuescheler
Copy link
Contributor

davidnuescheler commented Apr 27, 2023

This happens when the LCP is finally 'visible' to the browser and in the DOM ("loaded").

no, that's not how it works. the body actually only get unhidden when the LCP candidate is loaded. so it is being loaded when it is display: none and the loading starts with the LCP being set to loading="eager" (which happens super early in the decoration phase).
we originally had the loading="eager" on the server, but that means that you have to guess your LCP on the server, which can be wrong, especially if you load the actual LCP element from a JSON (eg. an article feed), and moving it to the client made everything much more flexible and simple without any impact on performance.

@davidnuescheler
Copy link
Contributor

@bstopp i am going to close this one, please feel free to reopen if you still think that this is an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preload Hero Image
4 participants