Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image Block: Use CSS style width/height instead of attributes #51929

Closed
ajlende opened this issue Jun 26, 2023 · 10 comments · Fixed by #52286
Closed

Image Block: Use CSS style width/height instead of attributes #51929

ajlende opened this issue Jun 26, 2023 · 10 comments · Fixed by #52286
Assignees
Labels
[Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended

Comments

@ajlende
Copy link
Contributor

ajlende commented Jun 26, 2023

Description

The width and height <img> attributes don't work well with object-fit CSS. The image block needs to be updated to use CSS width and height instead. This can be added in the style attribute so it can have the same effect of holding space on the page when the attributes are set.

Step-by-step reproduction instructions

  1. In the post editor add an image block with width and height different than the natural aspect ratio of the image. (Aspect ratio should say 'Custom' with both width and height set.)
  2. Ensure that 'Cover' is selected in the 'Size' select.
  3. View the post and see that the image is not cropped to the set width and height as it is supposed to.

Screenshots, screen recording, code snippet

In this example, the image should be square on the frontend with the content filling the entire space (object-fit: cover), but it does not respect the set width and height.

<!-- wp:image {"id":460,"width":100,"height":100,"scale":"cover","sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large is-resized"><img src="http://wp.local/wp-content/uploads/2023/06/test-image-rotated-90ccw-1024x683.jpg" alt="" class="wp-image-460" style="object-fit:cover" width="100" height="100"/></figure>
<!-- /wp:image -->

Screen Shot 2023-06-28 at 11 23 02
Screen Shot 2023-06-28 at 11 23 04

Environment info

Reproduced in both latest Firefox and Chromium.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ajlende ajlende added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block labels Jun 26, 2023
@ajlende ajlende self-assigned this Jun 26, 2023
@cbirdsong
Copy link

Functionally, there should be no difference between width/height set using attributes and CSS, and I can't replicate this behavior. Can you post a snippet of HTML that can be pasted into the editor and demonstrates this problem?

@ajlende ajlende moved this from Todo to In Progress in Automattic team Ignite's project board Jun 28, 2023
@ajlende
Copy link
Contributor Author

ajlende commented Jun 28, 2023

@cbirdsong I updated the description with a code snippet and some screenshots. It was a bug that I introduced, and I'm working on a fix for it now.

@felixarntz
Copy link
Member

@ajlende @cbirdsong Images without width and height attribute are a major problem for user experience as they lead to layout shifts. Unless there is an alternative approach that ensures the lack of the attributes does not cause layout shifts, we need to maintain the width and height attribute present on every image. This was intentionally fixed in WordPress core a few years ago, to even backfill the attributes on post content with images that don't have them.

See also https://web.dev/optimize-cls/#images-without-dimensions

@cbirdsong
Copy link

cbirdsong commented Jun 30, 2023

The aspect ratio/sizing options in the featured image block work as expected:
CleanShot 2023-06-30 at 08 59 35

The resulting markup of the two:

<figure class="wp-block-image size-large is-resized">
  <img
    decoding="async"
    loading="lazy"
    src="http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4-1024x683.jpg"
    alt=""
    class="wp-image-96"
    style="object-fit: cover"
    width="100"
    height="100"
  />
</figure>

<figure style="aspect-ratio: auto; width: 100px; height: 100px" class="wp-block-post-featured-image">
  <img
    width="1251"
    height="834"
    src="http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4.jpg"
    class="attachment-post-thumbnail size-post-thumbnail wp-post-image"
    alt=""
    decoding="async"
    loading="lazy"
    style="width: 100%; height: 100%; object-fit: cover"
    srcset="
      http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4.jpg          1251w,
      http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4-300x200.jpg   300w,
      http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4-1024x683.jpg 1024w,
      http://stock.local/wp-content/uploads/2022/01/9f8208c1-2172-35e9-b7fa-3817d14801d4-768x512.jpg   768w
    "
    sizes="(max-width: 1251px) 100vw, 1251px"
  />
</figure>

The root problem is img { height: auto } being set in a couple places.

/* wp-block-library-inline-css */

html :where(img[class*=wp-image-]) {
    height: auto;
    max-width: 100%
}

/* wp-block-image-inline-css */

.wp-block-image img {
    box-sizing: border-box;
    height: auto;
    max-width: 100%;
    vertical-align: bottom
}

Removing the width/height attributes would be a massive regression. Are you planning to just add the inline styles to mimic the featured image block?

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 2, 2023
@ajlende
Copy link
Contributor Author

ajlende commented Jul 2, 2023

Images without width and height attribute are a major problem for user experience as they lead to layout shifts. Unless there is an alternative approach that ensures the lack of the attributes does not cause layout shifts, we need to maintain the width and height attribute present on every image.

Having a width (or height) + aspect-ratio in the CSS should be enough for the layout engine to reserve space for the image preventing layout shifts. The challenge is when there isn't a width OR height specified. As it is now, there's still a layout shift when using the HTML attributes with aspect-ratio specified in CSS that I'm trying to fix as well. The rendering engine seems to reserve the attribute specified width and height and then recalculates when the image loads and the CSS is applied. Seems like a browser bug to me—I would expect it to figure the final size out with all that information—but it seemed to only work when I added one of the dimensions in CSS.

Removing the attributes is mostly for efficiency and shipping less HTML. I expect it would still work fine with the attributes and the CSS, but the CSS should be enough on its own.

Removing the width/height attributes would be a massive regression. Are you planning to just add the inline styles to mimic the featured image block?

I can't add them to the figure because the image block includes a caption inside the figure that shouldn't be included in the aspect ratio calculation. Whereas, the featured image block has a wrapper with only the image inside, so it can be applied to the wrapper.


I'm out this week, so I won't be making any progress until I'm back. I've pushed some things up to #52220 that I'll continue working on when I return.

@cbirdsong
Copy link

Is this change conditional and only applied when you modify the aspect ratio, or is it applied univerally? Using inline CSS instead of HTML attributes is unnecessary for 99% of cases, and the addition of inline CSS is a major problem if you want to use a strict content security policy that doesn't include unsafe-inline.

@scruffian
Copy link
Contributor

@cbirdsong I think it's applied in all cases, but note that the CSS isn't instead of the HTML attributes, it is as well as them. Is that still an issue?

@ajlende
Copy link
Contributor Author

ajlende commented Jul 10, 2023

the addition of inline CSS is a major problem if you want to use a strict content security policy that doesn't include unsafe-inline.

@cbirdsong There are lots of places where inline CSS is used within Gutenberg—we have an entire styles block attribute that is dedicated to adding inline styles using many of the core block controls—so I would expect this to be a widespread issue with many of the core controls.

Would generating a stylesheet with classnames for these styles work around the CSP? I worked on a system for generating stylesheets for a different feature, and I wonder if that might be needed for all of these places that use inline CSS if the CSP is that big of a problem.

@cbirdsong
Copy link

cbirdsong commented Jul 10, 2023

I think it's applied in all cases, but note that the CSS isn't instead of the HTML attributes, it is as well as them. Is that still an issue?

Yeah, if it's supplementary that should be fine. Shipping duplicated info like that isn't optimal, but that's preferable to it not working if CSS is disabled for whatever reason.

There are lots of places where inline CSS is used within Gutenberg—we have an entire styles block attribute that is dedicated to adding inline styles using many of the core block controls—so I would expect this to be a widespread issue with many of the core controls.

Yes, making an issue detailing the problems we've run into and workarounds we've had to employ to use a CSP has been on my to-do list for a while. For this issue I wanted to make sure an extremely fundamental thing like image sizing was not going to move to relying exclusively on inline CSS.

Would generating a stylesheet with classnames for these styles work around the CSP? I worked on a system for generating stylesheets for a different feature, and I wonder if that might be needed for all of these places that use inline CSS if the CSP is that big of a problem.

This would definitely be a good solution, and it would probably also help with making each page's CSS cacheable!

@ajlende
Copy link
Contributor Author

ajlende commented Jul 10, 2023

This would definitely be a good solution, and it would probably also help with making each page's CSS cacheable!

Since these are per-block customization, I don't think it would be very cacheable, but we would be able to prevent some unused CSS from ever needing to be loaded on the page.

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended
Projects
5 participants