🐛 BUG: <Picture /> component passes props as attributes to the underlying picture element

[carlcs]

What version of astro are you using?

1.0.0-beta.65

Are you using an SSR adapter? If so, which one?

none

What package manager are you using?

yarn

What operating system are you using?

Mac

Describe the Bug

When passing props such as class or alt to the <Picture /> component, they are added as attributes to the underlying picture element. It would probably be better if they ended up being added to the child img element.

Issue with the current behaviour that I ran into (see repro):

• Lighthouse lists a missing alt attribute. It needs to be added to the img element.
• CSS property object-fit does nothing on the picture element, it works when added to img.

Other image components make it possible to add attributes to the img element via a separate prop.

<!-- Astro Imagetools -->
<Picture attributes={{ class: "my-class" }} />

<!-- Nuxt Image -->
<nuxt-picture :imgAttrs="{ class: 'my-class' }" />

But I don’t think that this is really necessary, and we should just pass all props to the img instead. In my experience I have yet come across a use case where I needed to add attributes to the picture element. If a container is required for styling purposes you can always wrap it in a div and add your styles there. If anything I think we should rather do it the other way around and support passing attributes to the picture element via a dedicated pictureAttrs prop.

All examples on MDN or in HTML specs that I came across use picture attribute-less, as a plain container to wrap the img and its additional sources.

They have some notes to prevent usage mistakes:

The picture element itself does not display anything; it merely provides a context for its contained img element that enables it to choose from multiple URLs
— https://html.spec.whatwg.org/multipage/embedded-content.html#the-picture-element

Use these properties on the child img element, not the picture element
— https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture#usage_notes

Link to Minimal Reproducible Example

https://codesandbox.io/s/nifty-wiles-cn6p8w?file=/src/pages/index.astro

Participation

• I am willing to submit a pull request for this issue.

[natemoo-re]

This suggestion makes a lot of sense, I'll leave this one in @tony-sull's capable hands!

[tony-sull]

💯 those props really should be passed down to the img rather than the picture, thanks for catching that oversight!

[rndexe]

Hello, is there a suggested workaround for the "object-fit" case?

[carlcs]

First of all, thanks @tony-sull and @natemoo-re for considering this change. The PR looks great, I’m really looking forward to the merge!

@rndexe I’m pretty sure once this is implemented you can do something like this:

<div class="banner">
  <Picture
    class="banner-img"
    src={imageUrl}
    widths={[530, 1060]}
    aspectRatio={1060 / 1081}
    sizes="100vw"
  />
</div>

<style>
  .banner {
    position: relative;
    height: 200px;
  }
  .banner-img {
    position: absolute;
    top: 0;
    width: 100%;
    height: 100%;
    object-fit: cover;
  }
</style>

[rndexe]

I think the PR only covers the alt attribute for accessibility concerns and reverted the change for other attributes to existing behaviour.

[carlcs]

Oh, I didn’t notice that this was reverted. That is sad.

[rndexe]

Does the picture element even support styling? I was trying create a scaling effect on hover and it isn't working in the current scenario

[carlcs]

At least for image specific styles (e.g. object-fit) you’ve got to apply them on the img element. So if you are relying on those you’ve got to use the <Image /> component currently, I guess.

@tony-sull can we please re-open the issue?

[natemoo-re]

Sorry, this issue should not have been marked as completed! This is still something we want to support, we just had to bump down the priority a bit since we have more pressing things to tackle.

If anyone wants to submit a PR to cover this use case in a more generic way, contributions are welcome!

[carlcs]

@rndexe I was thinking too much in the Tailwind way of applying classes directly to the elements. Which in case of the child img element isn’t possible right now when using the <Picture /> component. But you can for sure target the nested img element with CSS selectors like so:

<div class="banner">
  <Picture
    class="banner-img"
    src={imageUrl}
    widths={[530, 1060]}
    aspectRatio={1060 / 1081}
    sizes="100vw"
  />
</div>

<style>
  .banner {
    position: relative;
    height: 200px;
  }
  .banner-img > img {
    position: absolute;
    top: 0;
    width: 100%;
    height: 100%;
    object-fit: cover;
  }
</style>

[rndexe]

For some reason even this doesn't work, maybe I should start a support thread on discord instead of polluting this issue's comment thread.

Edit:
Found this workaround in the bugbash today

main > :global(picture > img) {
    width: 50%;
    height: auto;
    margin: auto;
    aspect-ratio: 4 / 3;
    object-fit: cover;
}

it needs global somehow

[Deanmv]

If you're using Tailwind (above 3.1) then you can achieve this by applying a an arbitrary variants such as:

<Picture
  class="[&>img]:rounded-full"
  ...
/>

[carlcs]

As @emmanuelchucks mentioned in his PR, the image component in Nextjs also passes all additional props to the <img> element. It’s in the current version and in the future one, where they are correcting some of the decisions they initially made.

https://nextjs.org/docs/api-reference/next/future/image#other-props
https://nextjs.org/docs/api-reference/next/image#other-props