Skip to content

feat(core): add warnings for oversized images and lazy-lcp #51846

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

Closed
wants to merge 1 commit into from

Conversation

atcastle
Copy link
Contributor

This PR adds two new warnings related to image performance to Angular. These warnings are based on existing warning in the NgOptimizedImage directive, but apply to all images, as opposed to just those generated with NgOptimizedImage. The rationale for this is that both of the image performance issues captured by these warnings apply equally to images that don't use the optimized image directive. By surfacing them as warnings, we can help improve Angular loading performance and Core Web Vitals.

The two warnings are:

  1. Lazy-loaded-LCP: Fires when the the scanner discovers that an image which is the LCP element has been rendered with loading="lazy'. This can have a strong negative impact on LCP.
  2. Oversized image: Fires when the scanner detects an image where the intrinsic height or width are more than 1200 pixels larger than the rendered height or width.

In both cases, the scanner runs once, on the page load event.

CC: @AndrewKushnir @kara @dgp1130

@pullapprove pullapprove bot requested review from crisbeto and dylhunn September 21, 2023 09:20
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Sep 21, 2023
@AndrewKushnir AndrewKushnir self-assigned this Sep 21, 2023
@AndrewKushnir AndrewKushnir requested review from kara, AndrewKushnir and pkozlowski-opensource and removed request for crisbeto and dylhunn September 21, 2023 15:52
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release area: common Issues related to APIs in the @angular/common package common: image directive labels Sep 21, 2023
@ngbot ngbot bot modified the milestone: Backlog Sep 21, 2023
@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Sep 21, 2023
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Mostly comments about docs and testing

After an application is loaded, Angular checks included `<img>` elements for performance-harming misconfiguration. This warning can be triggered two ways:

### Oversized images
When images are loaded, the **intrinsic size** of the downloaded file is checked against the **rendered size** of the image on the page. If the downloaded image is much larger (more than 1200px too large in either dimension), this warning is triggered. Downloading oversized images can slow down page loading and have a negative effect on [Core Web Vitals](https://web.dev/vitals/).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: may want to add a note about the pixel density being part of it to prevent confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point--this actually made me go back and change something else. In the existing oversized image check, we were using a fixed value of 2 to align with the top bounds of the generated srcsets. For non-NgOptimizedImage images, that's a little confusing, so I updated the logic to just multiply by window.devicePixelRatio directly. I changed the error page to read:

When images are loaded, the **intrinsic size** of the downloaded file is checked against the actual size of the image on the page. The actual size is calculated using the **rendered size** of the image with CSS applied, multiplied by the [pixel device ratio](https://web.dev/codelab-density-descriptors/#pixel-density).

@atcastle atcastle force-pushed the image-performance-warnings branch 3 times, most recently from d3b9e36 to 6e67dc5 Compare September 25, 2023 09:04
@AndrewKushnir AndrewKushnir modified the milestones: Backlog, v17, v17-candidates Sep 28, 2023
@AndrewKushnir
Copy link
Contributor

TGP.

@atcastle atcastle force-pushed the image-performance-warnings branch from 6e67dc5 to 6614353 Compare October 3, 2023 20:23
// Wait to avoid race conditions where LCP image triggers
// load event before it's recorded by the performance observer
const waitToScan = () => {
setTimeout(this.scanImages.bind(this), 200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep 200, let's may be move it to a const outside of the class and add a note on what it is and why it's 200.

@atcastle atcastle force-pushed the image-performance-warnings branch from 6614353 to d9d316d Compare October 4, 2023 17:23
@pullapprove pullapprove bot requested review from alxhub and dylhunn October 5, 2023 22:18
@atcastle atcastle force-pushed the image-performance-warnings branch 4 times, most recently from 82b47c7 to f5c3017 Compare October 5, 2023 23:47
@AndrewKushnir AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 6, 2023
@AndrewKushnir
Copy link
Contributor

TGP.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

reviewed-for: fw-core, fw-common, public-api

Add warnings for two image-related performance problems that apply beyond just apps using NgOptimizedImage.
@atcastle atcastle force-pushed the image-performance-warnings branch from f5c3017 to 6c10f7b Compare October 6, 2023 17:26
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: global presubmit The PR is in need of a google3 global presubmit labels Oct 6, 2023
@AndrewKushnir
Copy link
Contributor

Caretaker note: TGP is green, this PR is ready for merge.

@atscott
Copy link
Contributor

atscott commented Oct 6, 2023

This PR was merged into the repository by commit 048f400.

@atscott atscott closed this in 048f400 Oct 6, 2023
console.warn(formatRuntimeError(
RuntimeErrorCode.IMAGE_PERFORMANCE_WARNING,
`An image with src ${src} has intrinsic file dimensions much larger than its ` +
`rendered size. This can ` +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this sentence is still trailing off. Fix in a follow-up?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 9, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…1846)

Add warnings for two image-related performance problems that apply beyond just apps using NgOptimizedImage.

PR Close angular#51846
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime common: image directive detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants