From ed2dfdae5bea93746be883bc528c1fb6407af6eb Mon Sep 17 00:00:00 2001
From: Emmanuel Chucks <31349069+emmanuelchucks@users.noreply.github.com>
Date: Thu, 20 Oct 2022 19:42:36 +0000
Subject: [PATCH] fix: updates <Picture /> to pass HTML attributes down to the
 <img /> element (#5038)

* fix: also pass attrs to underlying img

`class` for direct img styling, `width` & `height` to prevent layout shift, etc need to be passed to the underlying img tag to work.

* remove redundant attr from picture tag

* revert to previous commit

- seemed to break some tests
- can't deal with fixing that right now, maybe later

* only passing attributes to the img

* adding a note to the README

* chore: add changeset

Co-authored-by: Tony Sullivan <tony.f.sullivan@outlook.com>
---
 .changeset/wicked-laws-heal.md                | 11 ++++++++
 packages/integrations/image/README.md         |  4 +++
 .../image/components/Picture.astro            |  4 +--
 .../basic-picture/src/pages/index.astro       |  2 +-
 .../image/test/picture-ssg.test.js            | 28 +++++++++++--------
 .../image/test/picture-ssr-build.test.js      | 12 ++++----
 .../image/test/picture-ssr-dev.test.js        | 12 ++++----
 7 files changed, 46 insertions(+), 27 deletions(-)
 create mode 100644 .changeset/wicked-laws-heal.md

diff --git a/.changeset/wicked-laws-heal.md b/.changeset/wicked-laws-heal.md
new file mode 100644
index 000000000..bcaac6132
--- /dev/null
+++ b/.changeset/wicked-laws-heal.md
@@ -0,0 +1,11 @@
+---
+'@astrojs/image': minor
+---
+
+HTML attributes included on the `<Picture />` component are now passed down to the underlying `<img />` element.
+
+**Why?** 
+
+- when styling a `<picture>` the `class` and `style` attributes belong on the `<img>` itself
+- `<picture>` elements [should not](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture#attributes) actually provide any `aria-` attributes
+- `width` and `height` can be added to the `<img>` to help prevent layout shift
diff --git a/packages/integrations/image/README.md b/packages/integrations/image/README.md
index e57331124..70c6bbcd0 100644
--- a/packages/integrations/image/README.md
+++ b/packages/integrations/image/README.md
@@ -260,6 +260,10 @@ Position of the crop when fit is `cover` or `contain`.
 
 ### `<Picture />`
 
+The built-in `<Picture />` component is used to create an optimized `<picture />` for both remote images hosted on other domains as well as local images imported from your project's `src` directory.
+
+In addition to the component-specific properties, any valid HTML attribute for the `<img />` included in the `<Picture />` component will be included in the built `<img />`.
+
 #### src
 
 <p>
diff --git a/packages/integrations/image/components/Picture.astro b/packages/integrations/image/components/Picture.astro
index e86ccc65d..7cd71d600 100644
--- a/packages/integrations/image/components/Picture.astro
+++ b/packages/integrations/image/components/Picture.astro
@@ -65,9 +65,9 @@ delete image.width;
 delete image.height;
 ---
 
-<picture {...attrs}>
+<picture>
 	{sources.map((attrs) => <source {...attrs} {sizes} />)}
-	<img {...image} {loading} {decoding} {alt} />
+	<img {...image} {loading} {decoding} {alt} {...attrs} />
 </picture>
 
 <style>
diff --git a/packages/integrations/image/test/fixtures/basic-picture/src/pages/index.astro b/packages/integrations/image/test/fixtures/basic-picture/src/pages/index.astro
index 7cc20d65f..e05f878d4 100644
--- a/packages/integrations/image/test/fixtures/basic-picture/src/pages/index.astro
+++ b/packages/integrations/image/test/fixtures/basic-picture/src/pages/index.astro
@@ -20,7 +20,7 @@ import { Picture } from '@astrojs/image/components';
 		<Picture id='inline' src={import('../assets/social.jpg')} sizes="(min-width: 640px) 50vw, 100vw" widths={[253, 506]} alt="Inline social image" />
 		<br />
 		<Picture id="bg-color" src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png" sizes="(min-width: 640px) 50vw, 100vw" widths={[272, 544]} aspectRatio={544/184} alt="Google logo" background="rgb(51, 51, 51)" formats={['avif', 'jpeg']} />
-    <br />
+        <br />
 		<Picture id="ipsum" src="https://dummyimage.com/200x300" sizes="100vw" widths={[100, 200]} aspectRatio={2/3} formats={["avif", "webp", "jpg"]} alt="ipsum" />
 		<br />
 		<Picture id="query" src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png?token=abc" sizes="100vw" widths={[544]} aspectRatio={544/184} alt="query" />
diff --git a/packages/integrations/image/test/picture-ssg.test.js b/packages/integrations/image/test/picture-ssg.test.js
index 55134a154..9f0074b17 100644
--- a/packages/integrations/image/test/picture-ssg.test.js
+++ b/packages/integrations/image/test/picture-ssg.test.js
@@ -78,10 +78,11 @@ describe('SSG pictures - dev', function () {
 		},
 	].forEach(({ title, id, url, query, alt }) => {
 		it(title, () => {
-			const sources = $(`${id} source`);
-			expect(sources.length).to.equal(3);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
-			const image = $(`${id} img`);
+			const sources = picture.children('source');
+			expect(sources.length).to.equal(3);
 
 			const src = image.attr('src');
 			const [route, params] = src.split('?');
@@ -158,10 +159,11 @@ describe('SSG pictures with subpath - dev', function () {
 		},
 	].forEach(({ title, id, url, query, alt }) => {
 		it(title, () => {
-			const sources = $(`${id} source`);
-			expect(sources.length).to.equal(3);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
-			const image = $(`${id} img`);
+			const sources = picture.children('source');
+			expect(sources.length).to.equal(3);
 
 			const src = image.attr('src');
 			const [route, params] = src.split('?');
@@ -250,10 +252,11 @@ describe('SSG pictures - build', function () {
 		},
 	].forEach(({ title, id, regex, size, alt }) => {
 		it(title, () => {
-			const sources = $(`${id} source`);
-			expect(sources.length).to.equal(3);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
-			const image = $(`${id} img`);
+			const sources = picture.children('source');
+			expect(sources.length).to.equal(3);
 
 			expect(image.attr('src')).to.match(regex);
 			expect(image.attr('alt')).to.equal(alt);
@@ -349,10 +352,11 @@ describe('SSG pictures with subpath - build', function () {
 		},
 	].forEach(({ title, id, regex, size, alt }) => {
 		it(title, () => {
-			const sources = $(`${id} source`);
-			expect(sources.length).to.equal(3);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
-			const image = $(`${id} img`);
+			const sources = picture.children('source');
+			expect(sources.length).to.equal(3);
 
 			expect(image.attr('src')).to.match(regex);
 			expect(image.attr('alt')).to.equal(alt);
diff --git a/packages/integrations/image/test/picture-ssr-build.test.js b/packages/integrations/image/test/picture-ssr-build.test.js
index b76585240..aa3bdfc3e 100644
--- a/packages/integrations/image/test/picture-ssr-build.test.js
+++ b/packages/integrations/image/test/picture-ssr-build.test.js
@@ -89,12 +89,12 @@ describe('SSR pictures - build', function () {
 			const html = await response.text();
 			const $ = cheerio.load(html);
 
-			const sources = $(`${id} source`);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
+			const sources = picture.children('source');
 			expect(sources.length).to.equal(3);
 
-			const image = $(`${id} img`);
-
 			const src = image.attr('src');
 			const [route, params] = src.split('?');
 
@@ -200,12 +200,12 @@ describe('SSR pictures with subpath - build', function () {
 			const html = await response.text();
 			const $ = cheerio.load(html);
 
-			const sources = $(`${id} source`);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
+			const sources = picture.children('source');
 			expect(sources.length).to.equal(3);
 
-			const image = $(`${id} img`);
-
 			const src = image.attr('src');
 			const [route, params] = src.split('?');
 
diff --git a/packages/integrations/image/test/picture-ssr-dev.test.js b/packages/integrations/image/test/picture-ssr-dev.test.js
index 10d8f65cd..b21ee202a 100644
--- a/packages/integrations/image/test/picture-ssr-dev.test.js
+++ b/packages/integrations/image/test/picture-ssr-dev.test.js
@@ -104,12 +104,12 @@ describe('SSR pictures - dev', function () {
 		},
 	].forEach(({ title, id, url, query, alt, contentType }) => {
 		it(title, async () => {
-			const sources = $(`${id} source`);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
+			const sources = picture.children('source');
 			expect(sources.length).to.equal(3);
 
-			const image = $(`${id} img`);
-
 			const src = image.attr('src');
 			const [route, params] = src.split('?');
 
@@ -220,12 +220,12 @@ describe('SSR pictures with subpath - dev', function () {
 		},
 	].forEach(({ title, id, url, query, alt, contentType }) => {
 		it(title, async () => {
-			const sources = $(`${id} source`);
+			const image = $(`${id}`);
+			const picture = image.closest('picture');
 
+			const sources = picture.children('source');
 			expect(sources.length).to.equal(3);
 
-			const image = $(`${id} img`);
-
 			const src = image.attr('src');
 			const [route, params] = src.split('?');