vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.95k stars 26.87k forks source link

Enable styling of next/image outer wrapper #18585

Closed g-elwell closed 3 years ago

g-elwell commented 3 years ago

Styles added to the Image component get applied directly to the inner img element. However some of the most common styles you would usually apply to a regular img are position related (eg. width, margin, etc).

Because the img element in this component is absolutely positioned, these styles become useless.

There is no method to work around this other than placing your own additional wrapper outside the Image component and using CSS selectors to target the element you want to style as it's immediate child, eg:

// jsx
<div className="my-wrapper">
  <Image ... />
</div>

// outputted html
<div class="my-wrapper">
  <div style="max-width:100%;width:1200px">
    <div style="position:relative;padding-bottom:100%">
      <img style="visibility: visible; height: 100%; left: 0px; position: absolute; top: 0px; width: 100%;" ... >
    </div>
  </div>
</div>

// css
.my-wrapper > div {
  margin-right: 20px;
}

Suggested Fix: Apply component classnames and styles to the outer div instead of the img element

If styles applied to the Image component were applied to the outer div instead, you'd be able to use margin and padding without the need of an additional wrapper. This would be closer to a drop-in replacement for img for most people.

It would still be possible to style the img element directly as a descendant so you don't lose this functionality, eg:

// jsx
<Image className="my-image" ... />

// css
.my-image {
  margin-right: 20px;
}
.my-image img {
    border-radius: 10px;
}
uinstinct commented 3 years ago

Yeah, this is sure of a trouble. Neither classNames nor styles are passed down to the NextImage component.

daneden commented 3 years ago

+1! There also seems to be a lack of support for <style jsx> className styles passed to next/image:

import Image from "next/image"

export default function Example() {
  return (
    <>
      {/* This Image's inner `img` tag gets `class="image"` but not the additional/transformed jsx class name(s) */}
      <Image src="/image.jpg" className="image" width={128} height={128} />
      <style jsx>{`
        .image {
          width: 64px;
        }
      `}</style>
    </>
  )
}

Failing better support for styles or classNames, better documentation for how passed className is handled and/or recommendations for styling next/image (i.e. styling wrappers or using :global(img)) in Next.js documentation would be great.

lachlanjc commented 3 years ago

+1 The nested divs have caused a number of layout issues on https://scrapbook.hackclub.com (https://github.com/hackclub/summer-scrapbook/issues/115) that required ugly CSS (> div:first-child etc) to work around (e.g. styling CSS Grid children works differently from an image inside a div), & we’re still getting to the bottom of some (https://github.com/hackclub/summer-scrapbook/issues/117). Would love more flexibility here—I’m fine handling the sizing myself & would rather skip Next’s divs entirely in some cases.

g-elwell commented 3 years ago

@lachlanjc just FYI the overflow/layout issue seems to have been fixed in canary, so it's coming! I was having similar issues with next/image inside a flexbox child.

lachlanjc commented 3 years ago

Thanks so much @g-elwell! Just knocked out that issue, appreciate the tip.

TheThirdRace commented 3 years ago

I'm still having issues with this.

Using style is a big offense since you can't override without using !important which is an absolute "no no" for many people.

But there are so many styles too, I can disable 17 styles out of 22 for the 3 elements (div, div, img) and still get the same result without most of the current limitations...

I found this thread because I have a container that has margins and I want the image inside to fit flush with its borders. Applying negative margins should have been easy, but the component use style to position itself with margin: 0 so now I need to resort to what I consider very bad hacks to get the job done...

Margin is a positioning property, a component should >>> never <<< take care of its own positioning, it should only take care of whatever is downward itself, not upward. Going against this principle will only bring you big troubles.

The lack of handles on wrappers is even worst with CSS-in-JS where in some frameworks you simply can't use :first-child selector because of how styling is injected in the DOM. So you have to resort to target a div, which:

  1. comes from a third party library (Next/Image)
  2. meaning it can change at any time
  3. meaning it's not a solid handle and could break easily in the future

At the end of the day, I would also ask why does the image actually style itself at all? All I really need from the Image component is the mechanic behind it (lazy loading, placeholder, srcset, etc.), not the styling. By trying to do it all in the same component, Next/Image is severely limiting itself in what people can accomplish with it. The component became a monolithic block that can't be manipulated at will and in doing so, can't fit every design you can dream of.

I hope someone will take a look at this rather sooner than later, because the Image component solves a lot of troubles, but the current downsides are more than problematic at the moment.

maxcutie commented 3 years ago

Trying to add some padding and having a problem as styles are not supported with next/image. Hope this gets addressed soon :(

ashalfarhan commented 3 years ago

I'm also having this issue, there are some issues that i hope will be available soon. Can't apply border radius although its wrapped with a div, and can't hide the overflow image

lachlanjc commented 3 years ago

For border-radius specifically, I recommend using whatever child selectors your styling solution has to target the inner img element. But yeah, this should be way easier.

ashalfarhan commented 3 years ago

@lachlanjc On me, it works with target the Image 's className, but still wonder why can't hide the overflow image from it's parent, any suggest?

lachlanjc commented 3 years ago

@lachlanjc On me, it works with target the Image 's className, but still wonder why can't hide the overflow image from it's parent, any suggest?

Sorry, not sure what you mean. Can you link me to code or clarify?

ashalfarhan commented 3 years ago

@lachlanjc I'm using Chakra UI for styling my app, here's the code:

<Container maxW="90vw" overflow="hidden">
        <Box
          h="100vh"
          w="70vw"
          position="absolute"
          zIndex={-1}
          right="-72"
          style={{
            transform: "rotate(180deg)",
          }}
        >
          <Image src="/service-bg.png" layout="fill" />
        </Box>
</Container>

but it still can't hide the Image or the Box component

lachlanjc commented 3 years ago

I’ve never used Chakra, but it looks like since it’s using Emotion, you can use @emotion/styled or set up their JSX pragma: https://github.com/chakra-ui/chakra-ui/issues/287#issuecomment-563083168

TheThirdRace commented 3 years ago

I confirm for Chakra, it works if you add a global style and work your way there with css selectors on element types... But I don't consider this a robust solution as anything and everything can break it. Definitely not something I'd want to put in production because of how brittle it is.

The solution should come from NextJs, not other frameworks. We need a class handle at every level of the component or not so many divs on top of each other.

The styling issues are real, most of the limitations are completely arbitrary. As mentioned in my previous post:

But there are so many styles too, I can disable 17 styles out of 22 for the 3 elements (div, div, img) and still get the same result without most of the current limitations...

The problem is right there. I can disable 77% of all the inline styles and still get the same exact result. They've put style inline and forced so many limitations that cannot be easily overwritten for absolutely no valid reason. "Positioning" limitations I might add, which should always be taken care of by the parent, not the component itself.

We could use a wrapper around the whole thing and avoid the styling issues altogether, but then you need 4 elements to display 1 image. The DOM is already bloated with the current solution when you have 20 images (20 3 elements = 60 DOM nodes), it gets worst if you need another wrapper on top of it (20 4 = 80). In general, I should be able to get away with just the <img> tab, which would be 20 DOM nodes for 20 images.

NextJs needs to address this problem, not everyone needs a solution that auto-magically position the image in their custom layout by itself. Most people simply need the nuts and bolts, all they really need from the Image component is the mechanic behind it (lazy loading, placeholder, srcset, etc.), not the styling.

TheThirdRace commented 3 years ago

Just found another issue with the current styling solution of Next/Image.

Since it uses inline styles, you can't get rid of 'inline-unsafe' in your Content-Security-Policy header.

As with pretty much anything named inline-unsafe, we can guess pretty accurately the implications... ;)

rubenpoppe commented 3 years ago

The following works for setting a width and height.

<style>
.img {
    object-fit: contain;
}
</style>

<div style={{ width: '90%', height: '90%', position: 'relative' }}>
  <Image
    src={'/img/path'}
    layout={'fill'}
    className={'img'}
  />
</div>
TheThirdRace commented 3 years ago

@rubenpoppe

It does indeed work, but you're now having 4 DOM nodes per image. (3 wrappers + 1 image)

Now imagine if I have 40 images in my page. The DOM alone just got 160 new nodes instead of 40.

Given the limit Google suggest is 250 (source: Lighthouse), I'd wager you would get penalized in Google rankings for excessive DOM size. And in practice, staying under 250 nodes when 160 nodes are taken by images is basically impossible unless you have pretty much nothing else (no nav, no headings, etc.).

So while it works, I'd say it's not recommended.

jschmidtnj commented 3 years ago

this works for tailwind (top-20 for navbar):

      <div className="absolute top-20 w-full h-full">
        <Image
          src="/assets/img/homepage_background.png"
          layout="fill"
          objectFit="cover"
        />
      </div>
zraineri commented 3 years ago

Can there be a param which enables not having all the extras and just have the optimized img tag?

The optimizations are nice, but this implementation makes basic image styling a headache

TheThirdRace commented 3 years ago

I've fiddled a lot with the next/image component in the last 2 weeks.

Given all the problems I encountered, I decided I would rewrite the whole component myself. So opened the source code available on github and copied it in my project.

My first goal was to strip all the wrappers and only keep <img> in the DOM. And it actually worked wonderfully.

Then I ran Lighthouse in Chrome dev tools... I was getting some Cumulative Layout Shift (CLS). Not much mind you, but still higher than zero.

That's when I understood why they used a wrapper. Even if you pass the freaking width and height, Chrome can't manage it correctly and you do get a slight CLS.

To fix this, I decided to use a <picture> wrapper to implement the padding-top hack.

Yes, I use a <picture> wrapper for something other than art direction. If you check the HTML specification carefully, <picture> acts a container for <img>. It's usually used for art direction, but it's not mandatory either. The styles that target the image needs to be on <img>, but it's not against semantics to put other styles like the padding-top hack on <picture>.

After this change, I was getting zero CLS in Lighthouse. Huge win, but now I have a wrapper...

Then I tried to reproduce all the layouts in next/image and it turns out I could only do 3 out of 4 without resorting to an extra wrapper.

At this point, I understand why next/image is like it is right now. I don't agree with all their decisions, but I understand the trade-offs they had to make.

At the end of the day, I replaced next/image by my own Image component. Here is the entire code: https://gist.github.com/TheThirdRace/7f270a786629f119b57d1b2227a4b113

I make trade-offs too, but I think it's where NextJs should have traced the line in the sand and not crossed it. I put more responsibilities in the hands of the designers to keep the component more flexible. It's less newbie-friendly, but on the other hand it's much more flexible.

You can read in the gist a big block of comments that explains the pros & cons of my component.

Fair warnings:

After all my experiments, I think NextJs should fix these points to make it more usable by their customers:

There's also the whole discussion around the caching that can't be overridden. I understand their caching strategy and I think it's a good one, but I see no valid reason to purposefully prevent the users from overriding the defaults.

rubenpoppe commented 3 years ago

@TheThirdRace first of all, nice work! Would setting contain-intrinsic-size on layImage not solve the layout shifting (without a wrapper)? (I haven't tried it myself yet. Also never really noticed the jumpiness you're talking about in demos.) Based on a HTTP 203 video and the demo (cutting down load time on the HTML spec).

TheThirdRace commented 3 years ago

@rubenpoppe Maybe, I haven't tried with contain-intrinsic-size. But I don't think so.

contain-intrinsic-size is supposed to be used with content-visibility, which is not what's causing this problem.

Normally, I should be able to pass a heigth & width and have 0 CLS in Chrome. That's the whole point Chrome is trying to solve.

But from my experience with Lighthouse, there's still a small CLS, like 0.03 using it this way.

At this point, I would consider this a bug in Chrome. It shouldn't have any CLS at all if we're passing height & width.

If it were working correctly, we could easily remove all the wrappers and let people use only <img>. Unfortunately, for the time being it's not a valid option if you want no CLS at all.