Fix resizing to max width in classic themes #64819
Conversation
kevin940726
added
[Type] Bug
kevin940726
requested review from
noisysocks,
talldan,
ramonjd,
andrewserong and
aaronrobertshaw
|
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. |
|
Size Change: +521 B (+0.03%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the PR @kevin940726 👍
I'm missing some of the recent history around the image resizing feature so take this review with a grain of salt.
Regarding the main thrust of the PR, it does fix the issue with top-level images in classic themes being able to be resized larger than their container.
While testing though I ran into weird behaviour trying to resize images, regardless of whether I was using a classic or block theme.
Some of the issues encountered were:
- If the image block was aligned (top-level or within another container like columns), any resizing of the image doesn't stick. Looks ok on trunk.
- Once aligned, the drag handles are clunky in that you can't drag them wider once you've dragged the handle in a bit.
Block Theme
Screen.Recording.2024-08-27.at.5.45.14.PM.mp4
Classic Theme
Screen.Recording.2024-08-27.at.5.46.29.PM.mp4
I can confirm. From what I can see, the combination of Kapture.2024-08-28.at.10.14.18.mp4 |
kevin940726
force-pushed
the
fix/image-max-width-classic
branch
from
d9e9642
to
bb5621b
Compare
|
Flaky tests detected in 33b11ac. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10768071519
|
There was a problem hiding this comment.
Nice follow-up @kevin940726!
In manual testing, I think I might have found another case where 100% might not work for the maximum value? If I have an image at the root of a post it works fine. But if I add a full width Group block, give it a child Image block, and then set the Group block to use a custom content size, then the 100% appears to allow the dragging to still go beyond the size of the container.
It was a little unreliable for me in reproducing this while testing, but here's a screengrab in case it helps:
2024-08-29.16.13.33.mp4
Since we're using the content resize listener for the align mode now with cloneElement, would it be worth trying using it when no alignment is applied, too?
kevin940726
force-pushed
the
fix/image-max-width-classic
branch
from
bb5621b
to
7493311
Compare
|
I pushed some updates, here's the result: Block themes (twentytwentyfour): Kapture.2024-08-29.at.16.37.37.mp4Classic themes (twentytwentyone): Kapture.2024-08-29.at.16.33.36.mp4The image block is complicated though, so I'd not be surprised if I missed some gotchas 😅. Appreciate any further testing 🙇 ! |
There was a problem hiding this comment.
This is feeling close to me now and addresses the issue in the Group block I encountered yesterday. The new bug I've encountered is when attempting to resize within a Row block. It doesn't seem to allow me to resize an image down — it winds up treating any resize as though resetting to auto for some reason 🤔
2024-08-30.15.02.16.mp4
Are you able to reproduce that?
I had a go at logging out what maxWidth is set to, and it seems that when the image block is a child of the Row block, when the onResizeStop fires, the maxWidth winds up being set to the same value as the width that the user dragged to, so I think it's being treated as within the threshold for resetting to Auto? Here's a screenshot of logging out maxWidth values in case it helps:
I'm guessing it might have something to do with the figure element (elt.parentElement) being a child of a flex element?
Apologies if I'm way off there!
|
Yeah, flexbox makes it a bit more complicated 😅. I pushed a commit and hopefully could solve that. I'm concerned about the Another concern is the |
|
The recent commit still fails when resizing an image to its max size in a flexbox. It seems like |
|
Nice progress here!
Just confirming the issue you're running into — is this only present when the Row block is set to not allow items to wrap? I found while testing that if I allowed the Row block to wrap items it behaves as expected, where dragging to the full width of the container sets it to Auto and the image fills the area as expected: 2024-09-03.16.33.27.mp4Whereas if the Row is set to not allow Wrap, then we can technically drag the image larger, but upon releasing the mouse button, the image will snap back to the size that flexbox believes it should have? It's not perfect since it's not WYSIWYG, but personally I also didn't mind it as it allows dragging all the way out to effectively perform a reset. Here's what I'm seeing: 2024-09-03.16.32.03.mp4Separately to that, though, I noticed an odd issue when selecting images within my test post. For some reason when I select these image blocks that didn't have any width values set, upon selecting them, they render smaller than the container size, and the max width doesn't seem quite right. These are root level image blocks within a post in case that helps: 2024-09-03.16.45.30.mp4Could the global margins / padding rules used for TT4 be getting factored in, in some way to the content resize listener? I.e. in this case is the parent style's padding being included when it shouldn't be? 🤔 |
kevin940726
force-pushed
the
fix/image-max-width-classic
branch
from
a85927d
to
b98be96
Compare
kevin940726
force-pushed
the
fix/image-max-width-classic
branch
from
345dca5
to
ecbd668
Compare
|
I updated the PR to only set max width for "simple" layouts ( Let's see if we can fix simple layouts for both classic and block themes. 🤞 |
|
@kevin940726 in retesting I'm still getting the odd issue where the padding appears to be incorrectly included when selecting an image block at the root level of a post using TT4 theme (the last video in #64819 (comment)). It seems that in this case the padding shouldn't be subtracted from the max width? I.e. if I comment out / swap 2024-09-03.16.45.30.mp4In which cases do we need to factor in the padding? |
There was a problem hiding this comment.
This is looking very close @kevin940726. Just found a regression for the max-width in the Column block, but I think it might be a fairly simple fix 🤞
| const isMaxWidthContainerWidth = | ||
| parentLayout?.type === 'default' || | ||
| parentLayout?.type === 'constrained'; |
There was a problem hiding this comment.
I think this might need to also include an absence of a parent layout type. Or to put it another way, what if we checked that the parent layout exists (is truthy) and isn't flex or grid?
I ran into this issue because in a post with a Columns block where a user hasn't yet interacted with the layout settings for an individual Column, the parentLayout will be an object but won't have an explicit type property set, as it's implicitly a flow / default layout rather than that being explicit:
In that case the parentLayout object was: {contentSize: '620px', wideSize: '1280px'}
The problem I ran into is that with the current check, an Image in the Column block no longer gets the max-width behaviour:
2024-09-10.12.32.36.mp4
Note: if a user has toggled the layout on the individual Column block, then they might get the max-width behaviour, because the type will be set explicitly: {contentSize: '620px', wideSize: '1280px', type: 'default'}. So this issue is to do with Column blocks where the layout hasn't been touched since the block was added to the post.
There was a problem hiding this comment.
Good catch! I think this makes sense! I didn't know about this detail about layouts 😅 (a proof that TypeScript could be useful here?). Committed in 5e1e199.
There was a problem hiding this comment.
Found another couple of issues again, sorry @kevin940726!!
Column block in Classic themes
I think the parent layout check needs a tiny adjustment to handle Classic themes where the parentLayout might not always be available, as the handling within a Column block in Twenty Twenty One theme didn't work for me in re-testing. I've added a separate comment.
Selecting an image block that follows a left or right aligned block
This issue has me stumped, as I'm not really sure what's going on. But I found that if I left or right align an image block, then go to select a non-aligned image block that follows that block (even if there are paragraph in between), then the image block jumps in position, overlapping other blocks.
It's a little tricky to describe, but hopefully the following video will help show what's happening:
2024-09-10.16.25.52.mp4
In the above video, the second block's resizer appears to be beneath the left aligned block, which is kind of correct. However the image displays lower down than the resizer, and unexpectedly overlaps with the Columns block that follows.
Let me know if you need any more info to reproduce!
| const isMaxWidthContainerWidth = | ||
| !! parentLayout && | ||
| parentLayout.type !== 'flex' && | ||
| parentLayout.type !== 'grid'; |
There was a problem hiding this comment.
In testing a Classic theme, I just discovered that parentLayout may not even be present, so the image block can be extended beyond the Column container again:
So, to handle that, would it work to update this check to something like:
const isMaxWidthContainerWidth =
! parentLayout ||
( parentLayout?.type !== 'flex' &&
parentLayout?.type !== 'grid' );
(In the above screenshot the flex layout in the logs is just to confirm that the Row block does have the parentLayout object present with a flex layout, as expected)
I think left or right aligned images are broken in trunk too 😅. I think that's something we can fix in another PR as this can get out of control really quick 😞. I think #63341 also broke left and right aligned images in some way too 🤔. |
There was a problem hiding this comment.
Thanks for all the back and forth here @kevin940726, aside from the left/right alignment this is all testing well for me now!
I think left or right aligned images are broken in trunk too 😅. I think that's something we can fix in another PR as this can get out of control really quick 😞. I think #63341 also broke left and right aligned images in some way too 🤔.
Yes, I see it's broken on trunk now, too. Let's merge this PR and look into that bug next 🙂
|
Thanks for the thorough review! We can work on improving this in follow-up PRs! 🤞 |
What?
Continued from #63341. Fix resizing beyond max-width in classic themes. See #63341 (review).
Note
Flex layouts are not supported yet.
Why?
It should work in classic themes too.
How?
Turns out we can just use
100%width in the resizable box. I'm not sure if there's any unexpected side effect I didn't notice though!Testing Instructions
Follow the same instructions in #63341 but with classic themes.
Screenshots or screencast
Kapture.2024-08-27.at.16.31.58.mp4