CSS: Add a reset for image heights #30092
Conversation
scruffian
added
[Feature] Themes
scruffian
changed the title
|
Size Change: +33 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
| @@ -81,3 +81,7 @@ | |||
| .items-justified-space-between { | |||
| justify-content: space-between; | |||
| } | |||
|
|
|||
| img { | |||
| height: auto; | |||
There was a problem hiding this comment.
Not sure I understand the issue and fix here. I tried using an image and it didn't seem stretched for me. Maybe there are special cases? Anyway, it seems this is overriding something that comes from Core itself. Is that style broken? (inline image height) Should we remove it in Core itself directly.
cc @felixarntz as it seems related to your work on lazy loading images.
There was a problem hiding this comment.
It depends on the proportions of your image. You can see it in https://maylandblockstarter.wordpress.com/
There was a problem hiding this comment.
This is indeed not great, it seems we shouldn't be including that height in the first place, shouldn't we?
There was a problem hiding this comment.
I can reproduce this for image blocks when testing the emptytheme when:
- There's a wide or full alignment selected.
- The image has "Full Size" set as the image size.
When an image block is inserted, it gets some height and width by default. But when Gutenberg adjusts those, things get stretched. I'm not sure whether the height should be fixed by Gutenberg or by core then, but it probably shouldn't need to be fixed in the themes. 🙂
Default, Large Size (Things look fine in this scenario)
| Editor | Front |
|---|---|
 |
 |
Wide, Large Size
| Editor | Front |
|---|---|
 |
 |
Default, Full Size
| Editor | Front |
|---|---|
 |
 |
There was a problem hiding this comment.
but it probably shouldn't need to be fixed in the themes
I agree with this yes
There was a problem hiding this comment.
In principle, IMO this is the responsibility of themes to provide. Themes supporting the block editor have the responsibility of providing styles for .alignwide and .alignfull. As part of that (and as part of any usage that makes images span a certain width), it's necessary that any image has its height reset. So I'd argue, every theme should e.g. include something like
img.alignwide,
img.alignfull {
height: auto;
}I agree though that it makes sense to solve this in Gutenberg because a theme developer may not be aware of WordPress images potentially having size attributes, and it is a trivial fix. We're certainly solving this widely by including it in Gutenberg itself.
There was a problem hiding this comment.
In principle, IMO this is the responsibility of themes to provide.
This goest against the principles of Full Site Editing. In Full Site Editing, the theme doesn't have to provide anything for things to work properly and to have the editor match the frontend. You can start with an empty theme, go to the site editor and start building templates without having to provide any custom CSS.
Based on this principle, I don't think it makes sense anymore to have these inline attributes (the height at least) anymore in WordPress. There are just too many situations where things could go wrong. Thing just random images inside columns, inside a small "group" block. Images widths are not going to match the actual image width in a lot of situations, responsive mode is another one too.
So I'm not sure it makes sense to add these inline attributes if we override the styles later in a stylesheet.
There was a problem hiding this comment.
Themes supporting the block editor have the responsibility of providing styles for .alignwide and .alignfull.
This is no longer the case for themes that opt into the new alignments offered in FSE (#29335)
|
Simple fix that seems to do the job, and could potentially pass as a bandaid. But isn't this an issue that should be fixed at the source? |
|
yes, this basically undo what the core ticket does a little bit, so I'd like to have @felixarntz's thoughts on the best way forward here. I'm thinking personally that we should revert the addition of these inline width/heights because they don't take into consideration the context of where the image is inserted. |
|
@youknowriad I left some feedback in https://github.com/WordPress/gutenberg/pull/30092/files#r599874756. TLDR I do think it is a bandaid of somewhat broken theme styles, but it should be a reliable and trivial bandaid, so I'm not opposed to having it included in Gutenberg/WordPress. The case against it would be: Before there was Gutenberg, almost all images in WordPress already had size attributes on them. So if a theme had something like But Gutenberg does include that styling itself for the image block, so I think it's worth including it there specifically. |
There was a problem hiding this comment.
Instead of resetting the height for any images (which should remain responsibility of themes), I suggest to only reset height for images where Gutenberg itself defines the styles that would break it without reset, which is in https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/style.scss#L6.
|
@jasmussen This is also an issue in TT1 Blocks that I have also noticed. WordPress/theme-experiments#259 Strangely there, it only happens for the Media + Text block. |
|
Circling back to this issue. After some reading on the net, it seems we should do it. Basically all images should have height and width HTML attributes defined and wherever there's So what do you think about moving this rule to these places where we have the 100% widths instead of doing a global rule like that? |
That seems reasonable to me. 👍 |
scruffian
force-pushed
the
add/image-reset
branch
from
e0230fa to
6a912c5
Compare
I've done this in 55ad9e2, so this could use another review. Sorry it took so long! |
|
I didn't test but the changes look good to me |
|
Any idea how to get the performance tests to pass? |
|
@scruffian did you try rebasing, these tests can fail sometimes with old code. |
…max-width to 100%
scruffian
force-pushed
the
add/image-reset
branch
from
55ad9e2 to
3d29ca1
Compare
Description (updated 5/10/2021)
For all images that explicitly set widths or max-widths of 100% we should add a
height: autorule to prevent them from becoming stretched.Screenshots
Before:
After:
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: