Image block: unwrap img element in editor #68666
Conversation
stokesman
added
[Type] Bug
|
Size Change: +74 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @rickmgithub. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
Hey @stokesman, Thanks for the PR. This is testing well for me and covering multiple issues. ✅ The flicker issue is resolved Screen.Recording.2025-01-16.at.14.11.44.mov✅ The image disappearing when cropping inside a row/stack is also resolved Screen.Recording.2025-01-16.at.14.14.12.mov |
|
Hi @stokesman, Thank you for the PR, I have tested for disappearing image when editing (crop/rotate) auto-sized images inside flex layouts and this PR seems to solve the issue: Screencast:Screen.Recording.2025-01-16.at.2.44.28.PM.mov |
| // The only supported unit is px, so we can parseInt to strip the px here. | ||
| const numericWidth = width ? parseInt( width, 10 ) : undefined; | ||
| const numericHeight = height ? parseInt( height, 10 ) : undefined; |
There was a problem hiding this comment.
These became unneeded with the introduction of pixelSize that stays in sync with the displayed size of the img.
| const imageRef = useRef(); | ||
| const [ imageElement, setImageElement ] = useState(); |
There was a problem hiding this comment.
Switching to state instead of a ref for the img. I suppose this isn’t strictly necessary but because the value is read during render putting it in state follows the guidance of not reading refs in render.
| function onResizeStart() { | ||
| toggleSelection( false ); | ||
| } | ||
|
|
||
| function onResizeStop() { | ||
| toggleSelection( true ); | ||
| } |
There was a problem hiding this comment.
Moved into the bodies of the handlers at the actual ResizableBox JSX.
| width={ naturalWidth } | ||
| height={ naturalHeight } |
There was a problem hiding this comment.
Adding these img attributes is congruent with the front end and I'm pretty sure helped in some layout context though I forget the specifics.
| const dimensionsControl = ( | ||
| <DimensionsTool | ||
| value={ { width, height, scale, aspectRatio } } | ||
| onChange={ ( { | ||
| width: newWidth, | ||
| height: newHeight, | ||
| scale: newScale, | ||
| aspectRatio: newAspectRatio, | ||
| } ) => { | ||
| // Rebuilding the object forces setting `undefined` | ||
| // for values that are removed since setAttributes | ||
| // doesn't do anything with keys that aren't set. | ||
| setAttributes( { | ||
| // CSS includes `height: auto`, but we need | ||
| // `width: auto` to fix the aspect ratio when | ||
| // only height is set due to the width and | ||
| // height attributes set via the server. | ||
| width: ! newWidth && newHeight ? 'auto' : newWidth, | ||
| const dimensionsControl = | ||
| isResizable && | ||
| ( SIZED_LAYOUTS.includes( parentLayoutType ) ? ( |
There was a problem hiding this comment.
This is extraneous but it bugged me that the condition for which dimension controls would be visible was repeated in a couple places and both versions of the controls were being invoked regardless. If it helps to see it was done atomically in 6b28210.
|
Fantastic work, @stokesman! I'm going to add this to the WP 6.8 project. I think this is an excellent improvement for one of the most used blocks. |
|
I have manually tested this in different combinations of image sizes, with or without links, with borders, shadows, aspect ratios etc. |
|
Thank you for testing Carolina 🙇
I looked into this and found the same behavior here as on trunk. I think that the position is technically correct but is likely to be confusing. The actual Screenshot of Image block scale set to "Contain" with actual
|
|
What I am not finding, is a conversation about why the element was needed in the first place, and if everything that the element once solved, is now fixed. For example, this line in the removed CSS: Is this scenario resolved? |
|
And I think that issues should be created before pull requests. If the div created problems, they are not listed, and it makes it more difficult to review. |
I’ve looked into it and haven’t found it either. For me it’s reasonable to believe the wrapper element was introduced merely for easing the implementation of the in-canvas resizing. To create a resizable element with handles the most obvious way is to have the handles be children of the element. Yet an
I believe I tested that well enough but I can recheck. Assuming it is resolved, I’ll get back with more detail on how it can be tested. For reference, it comes from #7721.
I'm with you and it’s part of the guidelines for non-trivial PRs. Yet there are issues that were created before this PR and are linked here so I’m not certain what I can do better. I appreciate your feedback and will consider any suggestions on how to improve this. |
|
Thanks for the ping for review @carolinan, unfortunately, I don't have the bandwidth to help review or test here in the near future. I'll remove myself from the reviewers list, but it might pay to try pinging other contributors for additional eyes 🤞 |
| // This is necessary for the editor resize handles to accurately work on a non-floated, non-resized, small image. | ||
| .wp-block-image .components-resizable-box__container { | ||
| // Using "display: table" because: | ||
| // - it visually hides empty white space in between elements | ||
| // - it allows the element to be as wide as its contents (instead of 100% width, as it would be with `display: block`) | ||
| display: table; |
There was a problem hiding this comment.
I'm adding some commentary here since it was asked about
Currently the comment and style are outdated because display: table is overridden by inline styles. For reference, according to #44965 (comment) it was added to hide some empty space below the image (that made the selected block outline look wrong) while also avoiding a regression of broken center alignments. In trunk, it’s overridden with display: block from inline styles that were added in #51545 and I couldn’t find discussion on that particular change but it seems to have been okay and did not regress anything.
In regards to this PR, this selector and rule applied to the wrapper around the img and now there is no wrapper. Since there is no wrapper it’s worth considering the the display of the img itself and that is not specified. From my testing everything around this works. That is, resizing "non-floated, non-resized small images" works fine. Better than on trunk since the resize handles are initially out of place on small images (less than the container’s content width) there. Also:
- center aligned images work as expected
- there is no empty space below the image between the selected block outline
Some markup that can be used to test such images.
<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|20","bottom":"var:preset|spacing|20","left":"var:preset|spacing|20","right":"var:preset|spacing|20"}},"color":{"background":"#f3d5f1"}},"fontSize":"xx-large","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background has-xx-large-font-size" style="background-color:#f3d5f1;padding-top:var(--wp--preset--spacing--20);padding-right:var(--wp--preset--spacing--20);padding-bottom:var(--wp--preset--spacing--20);padding-left:var(--wp--preset--spacing--20)"><!-- wp:image {"width":"32px","sizeSlug":"large","linkDestination":"none","style":{"spacing":{"margin":{"left":"var:preset|spacing|20","right":"0"}}}} -->
<figure class="wp-block-image size-large is-resized" style="margin-right:0;margin-left:var(--wp--preset--spacing--20)"><img src="https://placehold.co/10x10/crimson/white?text=%23" alt="" style="width:32px"/></figure>
<!-- /wp:image --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|20","bottom":"var:preset|spacing|40","left":"var:preset|spacing|20","right":"var:preset|spacing|20"}},"typography":{"letterSpacing":"21px"},"color":{"background":"#f3d5f1"}},"fontSize":"xx-large","layout":{"type":"constrained","justifyContent":"center"}} -->
<div class="wp-block-group has-background has-xx-large-font-size" style="background-color:#f3d5f1;padding-top:var(--wp--preset--spacing--20);padding-right:var(--wp--preset--spacing--20);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--20);letter-spacing:21px"><!-- wp:image {"width":"32px","height":"auto","sizeSlug":"large","linkDestination":"none","align":"left","style":{"spacing":{"margin":{"left":"var:preset|spacing|20","right":"var:preset|spacing|20"}}}} -->
<figure class="wp-block-image alignleft size-large is-resized" style="margin-right:var(--wp--preset--spacing--20);margin-left:var(--wp--preset--spacing--20)"><img src="https://placehold.co/10x10/crimson/white?text=%23" alt="" style="width:32px;height:auto" title="fub"/></figure>
<!-- /wp:image -->
<!-- wp:paragraph -->
<p>SOMETHING</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
|
It turns out that this PR also resolves issue #69316. Use the following markup to apply a border to an image: <!-- wp:image {"sizeSlug":"large","linkDestination":"none","style":{"border":{"width":"0px","style":"none"}}} -->
<figure class="wp-block-image size-large has-custom-border"><img src="https://placehold.jp/1000x400.png" alt="" style="border-style:none;border-width:0px"/></figure>
<!-- /wp:image -->trunkImage height and resizable box height don't match: Detailsd62e2ae4922568d134f21e1ba4ec261d.mp4This PRThere is a slight delay, but the resizable box fits the actual image size: Details8dc3e6b511536d1f7bcc5aab241d4143.mp4 |
|
Thanks everybody for working on this. Here is how this PR tests for me - ✅ Issue #38381 - Images maintain proper responsive behavior and stay within their containers, even after manual resizing. 38381.mov✅ Issue #52219 - Images with aspect ratio display at their natural size without incorrectly stretching to fill the container width. 52219.mov✅ Issue #67506 - Images maintain consistent size when selected/deselected in various layout contexts. 67506.mov🟨 Issue #68057 - Images inside Row/Stack blocks maintain consistent sizing between editor and frontend views. However, as can be seen, if a very large paragraph is pasted, the image shrinks almost completely which might be unexpected but I think this is a separate issue to take care of. 68057.mov✅ Issue #68668 - Images no longer disappear when cropping in Row/Stack blocks or when aligned without specified size. 68668.mov✅ Issue #69316 - Resize container outline properly aligns with image borders when borders are applied. 69316.mov |
There was a problem hiding this comment.
Thank you, @stokesman!
Could you rebase this on top of the latest trunk? Then, I think it's ready to merge.
- The code looks good.
- It tests well for me and others.
- If we missed something, we could follow up with a fix.
stokesman
force-pushed
the
try/image-block-unwrapped
branch
from
54a80cd to
b4356d2
Compare
|
Flaky tests detected in b4356d2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13569907912
|
|
Thanks for the review George 🙇. I’ve rebased on trunk.
I have one fix in mind. The issue is technically not introduced here but I’m bringing it up because it could seem like it was. It’s a bug that manifests differently on this branch but is present on trunk. It may not be an issue on every theme but is on TT5, TT4, TT3 and TT2. It pertains to unsized images that are aligned left or right and whose innate width is larger than the content width. Thus, It’s fairly obscure. Moreover, for an image that’s aligned left or right it’s expected that the user will size it narrower than the content width and then there is no issue.
The cause is the UPDATE: I’ve opened #69364 for this. |
|
Folks, please pardon me. I failed to paste the props. |
What?
An attempt to fix several issues with the Image block in the editor by:
divwrapper aroundimg).I don’t think there’s as strong a reason to do this but I tried it because the size controls seem likely to be confusing in flex layouts.
There is also a few minor changes related to code quality.
Why?
Most generally this seems like a good idea because the markup becomes more similar to that of the front end.
How?
imgin the editor markup.ResizableBoxcomponent is made a sibling (instead of a parent).ResizableBoxon theimg.ResizableBoximgResizableBoxand width/height inspector controls.Testing Instructions
Markup I used in testing these issues. You’ll have to replace the images with your own though.
Screenshots or screencast
Editor/Front-end incongruities
Wrong size in flex and grid layouts (#68057 is open for this but is Row specific)
Image incorrectly filling the content-width #52219
An image block with an image that is smaller than the width of the post container with a specified aspect ratio and no specified size is incorrectly filling the width of the container.
Layout incorrect for images that are shorter than line-height (no open issue, I think)
If the image is shorter than the line-height so too is the figure and vertical alignment is incongruent with the front end
Editor-only issues
Resize handles out of place (I think there’s no open issue)
It affects images that are narrower than the content width with no size specified.
Disappearing image when editing (crop/rotate) auto-sized images inside flex layouts #68668
image-disappears-when-edited-in-flex.mp4
Layout shifts when editing (crop/rotate) auto-sized images
In trunk, an image without any specified size doesn’t maintain its displayed size when cropping/rotating. In this branch the image size will be maintained. This is an incidental change but might be hard to separate from the fix for the preceding issue. Besides it seems like a nice thing for consistency with how it works in most other scenarios (no layout shift).