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

Custom Image sizes are not responsive in the editor #38381

Open
kjellr opened this issue Jan 31, 2022 · 21 comments
Open

Custom Image sizes are not responsive in the editor #38381

kjellr opened this issue Jan 31, 2022 · 21 comments
Labels
[Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended

Comments

@kjellr
Copy link
Contributor

kjellr commented Jan 31, 2022

In the editor, by default, image blocks are 100% the width of their container, regardless of their actual dimensions. As soon as the user adds a custom width to their image however, that responsive guardrail is gone, and the image will flow off-screen.

responsive-image.mp4

The max-width guardrail works on the frontend. The editor discrepancy appears to be related to the in-canvas resize handles (see also).


Tested with a clean install of WordPress 5.9.
First reported over in the Twenty Twenty-Two support forums.

@kjellr kjellr added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block labels Jan 31, 2022
@kjellr kjellr changed the title [Block Themes] Custom Image sizes are not responsive Custom Image sizes are not responsive Jan 31, 2022
@jasmussen
Copy link
Contributor

Good ticket, thank you. It looks like #38399 fixes this one at least on the frontend:
editor

I'm currently investigating what it will take to also fix it in the editor.

@jasmussen
Copy link
Contributor

jasmussen commented Feb 1, 2022

It looks like this issue will be harder to fix in the editor, because the img is wrapped in a ResizableBox component to add the resize handles. It looks like the component does support maxWidth, but it seems like that has some height related side effects:
responsive editor

There's some commentary on what looks like a past exploration in the code for the Image block:

		// With the current implementation of ResizableBox, an image needs an
		// explicit pixel value for the max-width. In absence of being able to
		// set the content-width, this max-width is currently dictated by the
		// vanilla editor style. The following variable adds a buffer to this
		// vanilla style, so 3rd party themes have some wiggleroom. This does,
		// in most cases, allow you to scale the image beyond the width of the
		// main column, though not infinitely.
		// @todo It would be good to revisit this once a content-width variable
		// becomes available.

So it seems like at worst this can only be fixed in the editor with a little bit of a refactor. But given that #38399 fixes it on the frontend, we might want to see if we can land that one, and repurpose this ticket to detail the bug in the editor. What do you think?

Meanwhile, I'll tinker a bit with the max-width and a percentage value, see whether I can get it working. Edit: yeah there doesn't seem to be a trivial fix to this one, per the commentary above.

@jasmussen jasmussen changed the title Custom Image sizes are not responsive Custom Image sizes are not responsive in the editor Feb 2, 2022
@jasmussen
Copy link
Contributor

With the merger of #38399, I took the liberty of updating this issue to reflect that the issue is now editor-only.

@annezazu
Copy link
Contributor

Jumping in here to pass along some feedback that was found as part of the FSE Outreach Program's All Things Media exploration since this is very much related to this ticket:

The default dimension is highlighted as 100% but in-fact if clicking on 100% again usually modifies the image size on the editor. It is a bit confusing

Of note, this only happens in the Post Editor but that might be due to some breakage in the site editor (#38557).

154404537-85c4ec68-37b7-4ade-945b-712f04dce90f.mov

@Robertght
Copy link

There's still going on here, but I think it could be also very specific to some cases. This is what I saw on a user's site with a Layout Grid & Columns structure. The issue also remained once I took the images out of the Layout Grid:

image

I can confirm this remained a problem on self-hosted.

Is it worth opening another report on this @jasmussen ?

@rickmgithub
Copy link

rickmgithub commented Apr 12, 2022

Adding an additional report.

I can replicate this on a test site too using both a blockbase theme and Twenty Twenty

Markup on 2022-04-12 at 12:02:06

@skorasaurus
Copy link
Member

related: #12168 and #12168

@jasmussen
Copy link
Contributor

@Robertght or @rickmgithub can you provide some example content that I can test? Using the latest plugin version testing against TwentyTwenty and TwentytTwentyTwo it's working as it should for me:

tt2

tt

@jasmussen
Copy link
Contributor

jasmussen commented Apr 13, 2022

As an update, I did find a way to reproduce it after all. If you just insert an image, without resizing it manually, it works like noted above.

As soon as you resize the image manually, however, the image gets an explicit size and stops being responsive:
resized

That issue is editor only: on the frontend the responsive catch-all rules still work and the image doesn't break out of its container. However it is technically difficult to address, as outlined in this comment.

@Robertght
Copy link

Thank you @jasmussen ! I referenced the above as it appears this issue creates a very misleading experience, especially when used with Columns and the Layout Grid block. If these are separate issues, I can open a new one instead.

@jasmussen
Copy link
Contributor

It's all the same issue, if an image has been resized manually, it loses the responsive guard rail in the editor view. I agree it'd be really good to fix, though!

@tomasztunik
Copy link
Contributor

tomasztunik commented Apr 14, 2022

It's possible to fix this fairly easy but it would affect what outcomes are possible with the resizeable box component if we did not change the unit system for the resizer from absolute (px) to relative units (%).

Currently, when width/height are changed they are changed proportionally. If I make width 20% smaller, height will get 20% smaller as well and they will be set as width/height CSS properties. This could have caused images to have non native aspect ratio causing the image to deform if max-width was set or leak beyond the container if it wasn't.

To get a simple fix (at a cost I'll get to later) assuming we are working with static/relative images we can override setting height and just force it to be always auto and force max-width to be 100%. Changing height would proportionally change the width of the image instead as it does now already and the proportions would stay thanks to height: auto.

This is easily achievable with something like this without touching any of the JS code for resizer.

.wp-block-image .components-resizable-box__container {
    height: auto !important;
    max-width: 100% !important;
}

Problem this introduces - and any other implementation around that has a couple of drawbacks.

  • if the change was made on a bigger screen/window - on a smaller window it would get squashed to 100% of current small screen and potentially confuse about it not being 100% but fixed size.
  • It would not be possible to extend the image beyond the container - sometimes this might be the expected outcome depending on the design where user wants to break the grid.
  • The fixed width/height is not responsive - should the fixed dimensions be even used for mobile or responsive designs?

We are now limiting them to 100% but most of the time the fixed dimensions make sense only within the screen space user fixed the width/height of the image. I'm thinking if the width/height set there shouldn't be relative and based on % of the container rather than on absolute value which won't scale and possible cause breaking of a grid whenever non desktop screen size (or browser window) is used where column in which image got resized suddenly get smaller than the fixed width. With the responsiveness of block themes this is guaranteed to be the case anytime this feature is used. If we replaced absolute units with relative units this should always work alright and make the images more responsive similar to how using the fixed % breakpoint sizes works. Even creating overflowing images could work fine on mobile to create subtle grid breaking layouts.

if we had agreement on direction I'd be happy to pick it up.

@jasmussen
Copy link
Contributor

@tomasztunik Thank you for looking! I did explore a CSS-only fix like what you've suggested above, though when I tried it didn't work as resiliently as your fix and resulted in garbled images.

Nevertheless, I think the downside you outline isn't worth it. While the responsive max-width + auto height is currently applied on the frontend, it is applied with low specificity so themes can easily override it. Specificity and behavior in the editor should ideally match that of the frontend, and adding !important modifiers, past experience has suggested, always causes headaches down the road.

More importantly, though, #38990 outlines how a pattern developer will be able to set specific dimensions on an image and apply masking (object-fit: cover;). Imagine an About page pattern with circular photos on the left, this would require square images and a high border-radius value. With masking applied, you'd be able to drag and drop any image, even landscape or portrait, to replace the source image, but still keep the square shape and the rounded treatment. The CSS to make that work might involve aspect-ratio to be responsive, but it works. Given that, we can't lock an image just to its intrinsic aspect ratio, we have to tame the component directly. Thankfully ResizableBox supports maxWidth, we're just not using it.

@tomasztunik
Copy link
Contributor

tomasztunik commented Apr 15, 2022

Ha! Using maxWidth is definitely going to make it simpler :)

Though it would still prevent creating grid breaking designs which oftentimes are valid and desired so I'm not sure if it's an ideal solution.

I'm thinking though as I mentioned the units now are absolute (px) and I think this is a bigger issue. If someone designs the page on a smaller or larger screen/window and then previews it on different screen size or changes the container to be full width/wide the fixed nature of the unit wouldn't cause the content to properly scale to retain proportions and designs harmony.

I'm not sure if this issue is the right place to discuss this change but I think a lot of that overflowing images, and images not fitting in on mobile etc. comes from the fact that once the user touches the manual resize of the image it stops being responsive despite the block that hosts it and all other elements of the page around it are.

@jasmussen
Copy link
Contributor

Though it would still prevent creating grid breaking designs which oftentimes are valid and desired so I'm not sure if it's an ideal solution.

Indeed, do we choose between basic responsive guard-rails out of the box, or do we leave that to themes to handle?

I have a feeling that when we're able to tackle #38990, the object-fit toggle that is output when manually typing in non-intrinsic aspect-ratio dimensions might afford us a space to change how that responsiveness works. That refactor seems like it might provide an avenue for such boundary breaking sizes.

In the mean time, I think it would be useful to apply a max-width equal to the container size as described here, mainly because that is already the case on the frontend.

I'm thinking though as I mentioned the units now are absolute (px) and I think this is a bigger issue. If someone designs the page on a smaller or larger screen/window and then previews it on different screen size or changes the container to be full width/wide the fixed nature of the unit wouldn't cause the content to properly scale to retain proportions and designs harmony.

This would only ever be the case if they set explicit dimensions on an image, right? If you insert an image by default, it will naturally be size limited by its container. The px vs. % conversation is definitely valid, but in my mind it is an addon conversation that can happen regardless of where we end on this one. What do you think?

@Mamaduka
Copy link
Member

Mamaduka commented Apr 15, 2022

I experimented with relative maxWidth for #39678, and it works pretty well. Happy to push the current code if anyone wants to test it.

Relative units weren't used initially because ResizableBox didn't support them. Feature PR - bokuweb/re-resizable#526.

@tomasztunik, feel free to spin up new PR so we can test your fix as well.

@tomasztunik
Copy link
Contributor

@jasmussen - yes, by default image fills the width of the container and if % breakpoints (25, 50, 75, 100%) are used the images are responsive. It's only when the user puts in values by hand or when they drag the resizable box the image stops being responsive and the problem occurs.

I agree, with how the frontend now has max-width: 100% it makes sense to make it consistent.

I can try spinning up a try PR so that we have a better feel of what it could mean from a responsiveness perspective - it would require some changes on the front-end side as well I think. I think relative units would still work great with the planned aspect ratio and object-fit updates.

@jasmussen
Copy link
Contributor

I can try spinning up a try PR so that we have a better feel of what it could mean from a responsiveness perspective - it would require some changes on the front-end side as well I think. I think relative units would still work great with the planned aspect ratio and object-fit updates.

👍 👍 — another reason to keep that separate is that relative units aren't always obvious in their behavior, and it depends what they are relative to. Is this image 50% of the width of the column it's in? Or is it 50% width of the viewport.

@tomasztunik
Copy link
Contributor

@Mamaduka I had a look at the PR with maxWidth being based on the layout's wide width and thinking if this would produce expected results from the user's perspective. For images put as children of columns and nested deeper in the layouts most of the time it wouldn't be true and would still produce the glitchy states that were shared in the report.

I think using 100% of the container width might be better here to make it reflect frontend behaviour. Assuming the user would design the page at max width it would give him a full range of possible sizes within a given container.

Such a small thing but so many edge cases... I don't even want to get into how it should behave with absolute values when moved around to a different column or when columns get resized... :)

@annezazu
Copy link
Contributor

annezazu commented May 2, 2022

Just noting that this came up with the Avatar block (another image related block) in the comments query loop when you manually resize (as noted above):

Screen Shot 2022-05-02 at 12 26 20 PM

This was noted during group testing for the fourteenth call for testing for the FSE Outreach Program. To me, this is the same problem so noting it here but happy to open a new issue if that seems more advantageous.

@sinanisler
Copy link

related to this issue lets continue here
#55619

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
None yet
Development

No branches or pull requests

9 participants