Limit the max width of image to its container size #63341
Conversation
kevin940726
added
[Type] Bug
|
Size Change: -41 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
It seems close and I like the solution, but I think the main issue is that the image resized to the max bounds loses its responsive because it's set to an exact pixel value instead of 'auto'. I think it'd be better to set it back to 'auto' like an image is when first uploaded to an image block. Some steps to highlight the issue:
Now compare that behavior to this:
|
|
Another observation is that left/right aligned images can no longer be resized to the same degree they were before (it seems to be limited to content width). That's an interesting one, I'm not sure what the preferred behavior is. This is also a little bit theme dependent, some themes like TT4 show aligned blocks offset from the content which would allow them to be resized further, while others don't. That makes it tricky to know dynamically what the right limit is. |
mtias
added
the
[Feature] Media
|
Nice work. Just chiming in to say that I tested this and it fixes the issue 😀 |
+1, nice progress here, it feels much better to use to me with the constrained maximum.
Great idea, that sounds like a decent way to handle it. Would that be a matter of setting to Also just pinging @Mamaduka for visibility on this one since he commented on the linked issue 🙂 |
|
Nice. I've not thought of using |
I've been thinking about this. I think it's hard to distinguish the user's intention just by observing resizing. Maybe the user wants it to be a certain exact pixel wide. I think in most cases the user wants it to fit the max width though 😅. I wonder if some kind of "snapping" makes it clearer. Perhaps @WordPress/gutenberg-design has some ideas? 🙇
Another thing to note is that I think it also depends on the original image's size. If the image width is smaller than the max width, then resizing it to the max width (the user intention) should probably mean "width: 100%" rather than "width: auto"? 🤔 |
Good point. In this case, if folks want to set an exact pixel size, they can still do that in the sidebar. I like the idea of some kind of snapping at the max width that sets it to
Also a good point! The idea of defaulting to |
Possibly, I haven't checked the CSS. When I say 'auto', it's what the UI shows in the width field. Sorry if that wasn't clear. |
I'd echo fixed width vs. "unbounded" is one of the primary headaches to solve as part of this. And if we can do snapping to unbounded ("Auto" as Dan refers to), that would be great. We've had separate explorations into snapping into wide and full-wide alignments too, it's very difficult to build. But it would definitely be valuable, and in this case would account for snapping to content-width. |
|
I found this old issue that might be closed by this PR - #12168. |
|
Should we include the snapping feature in this PR, or could it be implemented separately? Is the implementation in this PR without snapping worth merging? |
|
Is there a basic version that could be attempted? Something like this pseudocode onResizeStop( { actualWidth } ) {
if ( Math.abs( containerWidth - actualWidth ) < 10 ) {
setWidth( '100%' );
} else {
setWidth( actualWidth )
}
} |
Committed in 858d8a2! |
There was a problem hiding this comment.
Thanks for the update @kevin940726! In general this is testing pretty well for me, but I notice that explicitly setting to auto results in a bug that stretches images upon resizing the viewport. I've left a comment, but I'm wondering if instead of setting auto explicitly, we can treat drags within the 10px threshold of the container as a reset instead. Would that work?
Here's a quick demo of what happens when both width and height are explicitly set to auto: (seems fine at first until we resize the viewport)
2024-08-21.10.17.13.mp4
| resizedWidth = 'auto'; | ||
| elt.style.width = 'auto'; |
There was a problem hiding this comment.
The default state of an image when it's inserted is that the controls display auto, but there's no explicit style output. Instead of injecting width: 'auto' and height: auto, could we instead treat this as a kind of "reset" and an early return, with something like:
setAttributes( {
width: undefined,
height: undefined,
aspectRatio: undefined, // (OR possibly re-use the ratio === naturalRatio logic below)
} );
return;If we go with what we currently have, I notice that the markup winds up including the auto rules:
<!-- wp:image {"id":4711,"width":"auto","height":"auto","sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large is-resized"><img src="http://a-toule-site.local/wp-content/uploads/2024/01/IMG_3906-768x1024.jpg" alt="" class="wp-image-4711" style="width:auto;height:auto"/></figure>
Whereas an image that is freshly inserted doesn't have any of those rules applied:
<!-- wp:image {"id":4711,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2024/01/IMG_3906-768x1024.jpg" alt="" class="wp-image-4711"/></figure>
There was a problem hiding this comment.
Good idea unsetting to undefined!
I don't know if it's possible to set an image to 100% of the container's width, so I added a check to fallback to explicit pixel values if the image is smaller than the container.
There was a problem hiding this comment.
Thanks Kai, this is feeling really close to me!
I don't know if it's possible to set an image to 100% of the container's width, so I added a check to fallback to explicit pixel values if the image is smaller than the container.
That sounds good to me for now — we could always look at it in a follow-up, potentially alongside an explicit way of locking to full width alignment as I imagine the two concepts could be quite related. E.g. when would we want to lock to "auto" vs setting the alignment 🤔
For now, though, I think unsetting and only doing so if the image is smaller than the container is a good solution 👍
The one issue I ran into in retesting is that I think I was wrong about explicitly setting aspectRatio to undefined. It looks like we might need to either not update the aspectRatio property at all in this call, or reuse the logic from the setAttributes call below:
aspectRatio:
ratio === naturalRatio
? undefined
: String( ratio ),
Otherwise, when adjusting the size within the threshold of the container area, we lose an aspect ratio if one has been set:
2024-08-22.11.24.08.mp4
What do you think? From my perspective, once that bug's resolved I think this PR will be in good shape to switch over to ready for review and get wider testing / feedback from designers, etc.
There was a problem hiding this comment.
I think that makes sense! Maybe we could just not override the aspectRatio in this case? I pushed an update in c1635bd.
There was a problem hiding this comment.
Nice, that's testing well for me now! (Also while re-testing, I think avoiding explicitly setting auto for width and height helps avoid potential conflicts with aspect ratios, too). Let's see what some designers think 🙂
Before landing, we'll likely want to manually test the controls in a variety of browsers and themes (including Classic ones), but overall this is feeling pretty solid to me!
kevin940726
force-pushed
the
fix/image-column-max-size
branch
from
858d8a2
to
b7bc071
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
This is testing great for me across browsers (Chrome, Safari, FF, Edge) and in a variety of scenarios with various blocks. Overall I think this is a really big improvement. There are still some outstanding bugs or things that would be good to polish in separate PRs, but for now I think the overall improvement here makes it very worthwhile 👍
I ran into one tricky issue (not a blocker) with Classic themes, with image blocks that are added to the root of the document — I think I've identified the issue, but I'm not 100% sure what the right fix might be.
In block themes, the max width of blocks within the document is determined by layout rules that apply to any element injected in the document. So the div added via the contentResizeListener gets the correct width assigned to it:
In the above screenshot, the listener div correctly gets the max-width of 620px, so when we go to drag the handle on the image block, it can't go past 620px, which is what we expected.
However on Classic themes like Twenty Twenty One, blocks get a max width based on the classname of wp-block, and the div used by the resize listener doesn't have that classname. As a result, the max width of the listener is much larger than expected:
This means that in Classic themes when we go to drag beyond the container it still lets us do it:
2024-08-26.15.19.44.mp4
Whereas in block themes, the listener is the correct size, so it works just as we'd expect:
2024-08-26.14.52.46.mp4
I found that in Classic themes, I could hack a fix in Dev Tools by inserting a wp-block class name into the div output by the content resize listener, which appears to prove out the above (though I'm not sure if that would be the right fix 🤔):
Longer-term, I wonder if it would be worth exploring whether we can let the resize observer accept a custom classname to output to the resize listener? If it supported that, then we could (potentially) pass it wp-block in order to fix this particular case. We'd then output it somewhere around here:
gutenberg/packages/compose/src/hooks/use-resize-observer/index.tsx
Lines 345 to 359 in ca2ee4d
In any case, I think that too could be a separate PR to look into. What do you think?
For now this PR resolves the situation of resizing within a Column block as reported in #63326, and also resolves dragging within the editor canvas in block themes, so I reckon the above could be looked at separately.
Thanks for reading this longer-than-expected brain dump! 😄
LGTM! 🚀
That's an interesting idea! I think we can do that today with |
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: jasmussen <[email protected]> * Limit the max width of image to its container size * Try fixing e2e tests * Snap to max width after resize * Set to undefined instead * Do not override aspect ratio
What?
Attempt to fix #63326.
Limit the max width of image to its container size.
Why?
See #63326. Images in columns or other container can be resized to be bigger than its container, which is not expected.
How?
Limit the
maxWidthof the image to its container's size. Not sure about this approach and potential side effects though.Testing Instructions
Screenshots or screencast
Kapture.2024-07-10.at.15.54.58.mp4