Toolbar audit incorrectly flagging images as above the fold

hfournier commented 2 months ago

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

[X] I am willing to submit a pull request for this issue.

hfournier commented 2 months ago

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 commented 2 months ago

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

hfournier commented 2 months ago

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.

withastro / astro