fix: audit incorrectly flagging images as above the fold (#12993) (#1…

…2998) * fix: audit incorrectly flag images as above the fold (#12993) * chore: add changeset * Add additional tests for perf audit
withastro · Jan 20, 2025 · 9ce0038 · 9ce0038
1 parent 22eafff
commit 9ce0038
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 2 deletions.
diff --git a/.changeset/cuddly-rings-sing.md b/.changeset/cuddly-rings-sing.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes the issue that audit incorrectly flag images as above the fold when the scrolling container is not body
diff --git a/packages/astro/e2e/dev-toolbar-audits.test.js b/packages/astro/e2e/dev-toolbar-audits.test.js
@@ -44,6 +44,55 @@ test.describe('Dev Toolbar - Audits', () => {
 		await appButton.click();
 	});
 
+	test('does not warn about perf issue for below the fold image after mutation when body is unscrollable', async ({ page, astro }) => {
+		await page.goto(astro.resolveUrl('/audits-perf-body-unscrollable'));
+
+		const toolbar = page.locator('astro-dev-toolbar');
+		const appButton = toolbar.locator('button[data-app-id="astro:audit"]');
+		await appButton.click();
+
+		const auditCanvas = toolbar.locator('astro-dev-toolbar-app-canvas[data-app-id="astro:audit"]');
+		const auditHighlights = auditCanvas.locator('astro-dev-toolbar-highlight');
+
+		expect(auditHighlights).toHaveCount(1);
+
+		await page.click('body');
+
+		let consolePromise = page.waitForEvent('console');
+		await page.locator('#mutation-button').click();
+		await consolePromise;
+
+		await appButton.click();
+
+		expect(auditHighlights).toHaveCount(1);
+	});
+
+	test('does not warn about perf issue for below the fold image in relative container', async ({ page, astro }) => {
+		await page.goto(astro.resolveUrl('/audits-perf-relative'));
+
+		const toolbar = page.locator('astro-dev-toolbar');
+		const appButton = toolbar.locator('button[data-app-id="astro:audit"]');
+		await appButton.click();
+
+		const auditCanvas = toolbar.locator('astro-dev-toolbar-app-canvas[data-app-id="astro:audit"]');
+		const auditHighlights = auditCanvas.locator('astro-dev-toolbar-highlight');
+
+		expect(auditHighlights).toHaveCount(0);
+	});
+
+	test('can warn about perf issue for below the fold image in absolute container', async ({ page, astro }) => {
+		await page.goto(astro.resolveUrl('/audits-perf-absolute'));
+
+		const toolbar = page.locator('astro-dev-toolbar');
+		const appButton = toolbar.locator('button[data-app-id="astro:audit"]');
+		await appButton.click();
+
+		const auditCanvas = toolbar.locator('astro-dev-toolbar-app-canvas[data-app-id="astro:audit"]');
+		const auditHighlights = auditCanvas.locator('astro-dev-toolbar-highlight');
+
+		expect(auditHighlights).toHaveCount(1);
+	});
+
 	test('can handle mutations', async ({ page, astro }) => {
 		await page.goto(astro.resolveUrl('/audits-mutations'));
 

diff --git a/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-absolute.astro b/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-absolute.astro
@@ -0,0 +1,9 @@
+---
+import { Image } from "astro:assets";
+import walrus from "../light_walrus.avif";
+---
+<div style="height: 9000px;"></div>
+<div style="position:absolute;top:0;left:0">
+    <Image src={walrus} loading="lazy" alt="A walrus" />
+</div>
+<div style="height: 9000px;"></div>
diff --git a/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-body-unscrollable.astro b/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-body-unscrollable.astro
@@ -0,0 +1,38 @@
+---
+import { Image } from "astro:assets";
+import walrus from "../light_walrus.avif";
+---
+
+<body>
+    <div id="scroll-container">
+        <Image src={walrus} loading="lazy" alt="A walrus" />
+        <div style="height: 9000px;"></div>
+
+        <Image src={walrus} loading="lazy" alt="A walrus" />
+        <button id="mutation-button">Mutation</button>
+        <div style="height: 9000px;"></div>
+    </div>
+</body>
+
+<style>
+    body {
+        margin: 0;
+        padding: 0;
+        height: 100dvh;
+        overflow-y: hidden;
+    }
+    #scroll-container {
+        height: 100dvh;
+        overflow-y: auto;
+    }
+</style>
+
+<script>
+    const mutationButton = document.getElementById('mutation-button');
+    mutationButton?.addEventListener('click', () => {
+        const dummyElement = document.createElement('div');
+        document.body.appendChild(dummyElement);
+
+        console.log('mutation');
+    });
+</script>
diff --git a/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-relative.astro b/packages/astro/e2e/fixtures/dev-toolbar/src/pages/audits-perf-relative.astro
@@ -0,0 +1,9 @@
+---
+import { Image } from "astro:assets";
+import walrus from "../light_walrus.avif";
+---
+
+<div style="height: 9000px;"></div>
+<div style="position:relative">
+    <Image src={walrus} loading="lazy" alt="A walrus" />
+</div>
diff --git a/packages/astro/src/runtime/client/dev-toolbar/apps/audit/rules/perf.ts b/packages/astro/src/runtime/client/dev-toolbar/apps/audit/rules/perf.ts
@@ -35,8 +35,14 @@ export const perf: AuditRuleWithSelector[] = [
 			'img:not([loading]), img[loading="eager"], iframe:not([loading]), iframe[loading="eager"]',
 		match(element) {
 			const htmlElement = element as HTMLImageElement | HTMLIFrameElement;
+
 			// Ignore elements that are above the fold, they should be loaded eagerly
-			const elementYPosition = htmlElement.getBoundingClientRect().y + window.scrollY;
+			let currentElement = element as HTMLElement;
+			let elementYPosition = 0;
+			while (currentElement) {
+				elementYPosition += currentElement.offsetTop;
+				currentElement = currentElement.offsetParent as HTMLElement;
+			}
 			if (elementYPosition < window.innerHeight) return false;
 
 			// Ignore elements using `data:` URI, the `loading` attribute doesn't do anything for these
@@ -55,7 +61,12 @@ export const perf: AuditRuleWithSelector[] = [
 			const htmlElement = element as HTMLImageElement | HTMLIFrameElement;
 
 			// Ignore elements that are below the fold, they should be loaded lazily
-			const elementYPosition = htmlElement.getBoundingClientRect().y + window.scrollY;
+			let currentElement = element as HTMLElement;
+			let elementYPosition = 0;
+			while (currentElement) {
+				elementYPosition += currentElement.offsetTop;
+				currentElement = currentElement.offsetParent as HTMLElement;
+			}
 			if (elementYPosition > window.innerHeight) return false;
 
 			// Ignore elements using `data:` URI, the `loading` attribute doesn't do anything for these