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

Toolbar audit incorrectly flagging images as above the fold #10891

Open
1 task done
hfournier opened this issue Apr 26, 2024 · 3 comments
Open
1 task done

Toolbar audit incorrectly flagging images as above the fold #10891

hfournier opened this issue Apr 26, 2024 · 3 comments
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: toolbar Related to dev toolbar (scope)

Comments

@hfournier
Copy link

Astro Info

Astro                    v4.7.0
Node                     v20.10.0
System                   Windows (x64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             @astrojs/tailwind
                         astro-icon

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

The Toolbar Audit shows:

Unoptimized loading attribute
This IMG tag is above the fold and could be eagerly-loaded to improve performance.

for images that are clearly NOT above the fold.

What's the expected result?

The Toolbar Audit should not show warnings for images that are not above the fold

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ddfdpg?file=src%2Fpages%2Findex.astro

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Apr 26, 2024
@hfournier
Copy link
Author

hfournier commented Apr 26, 2024

The problem is with how the above/below the fold is detected. The following code:

// Ignore elements that are below the fold, they should be loaded lazily
if (htmlElement.offsetTop > window.innerHeight) return false;

does not work when the <img> is inside a <div class="relative">, because htmlElement.offsetTop is 0 (see the console logs in the StackBlitz). So you get a false positive.

I've found two options to correctly determine the offsetTop.
The first adds up all the offsetTop values up the hierarchy until no parent is found (i.e. body):

let element = htmlElement
let totalOffsetTop = 0
while (element) {
	totalOffsetTop += element.offsetTop
	element = element.offsetParent as HTMLElement
}
if (totalOffsetTop > window.innerHeight) return false;

or by adding scrollY to getBoundingClientRect().top

const totalOffsetTop = htmlElement.getBoundingClientRect().top + window.scrollY
if (totalOffsetTop > window.innerHeight) return false;

I'd be happy to submit a PR with either solution, although the latter seems cleaner and easier to understand.

@Princesseuh Princesseuh added - P3: minor bug An edge case that only affects very specific usage (priority) feat: toolbar Related to dev toolbar (scope) and removed needs triage Issue needs to be triaged labels Apr 30, 2024
@Princesseuh
Copy link
Member

Hmm, won't the second solution cause a problem when you arrive on the page scrolled?

@hfournier
Copy link
Author

Yes, you're right that getBoundingClientRect by itself would cause a problem, which is why I add window.scrollY to account for it.

Scroll down half the page on the StackBlitz, hit the StackBlitz refresh button, and have a look at the console output:

not relative img
img.offsetTop:  1621
totalOffsetTop:  1621
getBoundingClientRect:  921
getBoundingClientRect and scrollY:  1621

relative img
img.offsetTop:  0
totalOffsetTop:  2025
getBoundingClientRect:  1325
getBoundingClientRect and scrollY:  2025

A few things we can observe:

  • On the relative positioned image, img.offsetTop is always: 0
  • getBoundingClientRect by itself does not provide the correct top value when scrolled
  • You can scroll anywhere on the page and refresh... you can see that totalOffsetTop and getBoundingClientRect and scrollY are always the same, no matter where you scroll to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: toolbar Related to dev toolbar (scope)
Projects
None yet
Development

No branches or pull requests

2 participants