Image Block - Fix scaling image outside block parent in editor.

[Addison-Stavlo]

Description

This component will now change the maxWidth property given to the ResizableBox component based on the width of the block parent containing it. This will prevent the image from being scaled beyond the size of the parent block.

Addresses issue #19424 (some more explanation is contained here) and issue #12168.

Previously maxWidthBuffer was used as the maxWidth property of the ResizableBox component. This buffer was in place to ensure 3rd party themes would be able to resize images larger than the standard column size if necessary. The side effect of this buffer enabled the resizer to resize the image larger than the parent block. Now instead of using this buffer we replace it with a specific value as soon as we are able to, allowing the image to be resized to whatever the width of the block parent may be (and not any further).

How has this been tested?

Tested on local docker setup with standard image block as well as image block nested in a smaller column.

Screenshots

Before

Standard Size

In a smaller column

After

Standard Size

In a smaller column

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[mapk]

From a UX perspective, this works exactly as it should. The image never exceeds the block's border. As long as this doesn't effect themes negatively, I'm good with the functionality and UX. 👍

We should probably get some technical feedback on implementation before merging though.

cc @youknowriad @gziolo

[youknowriad]

We tried this a long time ago but we had to revert because we didn't find the best solution. I'm having trouble remembering the exact problem we had.

One that comes to mind is that in floats, the image size dictates the block size and not the opposite but I'm not sure that's the only issue we had. Maybe @jasmussen remembers.

[jasmussen]

One that comes to mind is that in floats, the image size dictates the block size and not the opposite but I'm not sure that's the only issue we had. Maybe @jasmussen remembers.

Floats are definitely something to keep an eye on, but in this branch they seem to be working alright.

I think the issue we had was with the max-width implementation leveraging the default $content-width variable that was defined in variables.scss, and which might be different for themes like TwentyNineteen which overrides and changes the main column width to be entirely different. So I made sure to test this in other themes that style the editor, and it seems to work as intended. As such, this is a better fix.

However, there are issues with it, and I'm not sure how to best address them. First problem is that the max-width is set based on the viewport width, and never updated:

• Use a theme that has a variable width main content column, such as TwentyNineteen. (Note that there are some margin bugs in that theme which will be fixed with 5.4, but it still works)
• Insert an image with a small viewport
• Resize the viewport

Two issues with that:

1. The image by default fills the width of the main column. It doesn't in this branch, until you reload the page.
2. Until you reload the page, you can also not size up the image to fit.

Note that until an image has been resized, it does not have any width or height attributes, so it should behave according to CSS rules which are commonly something like `img { max-width: 100; height: auto; }

There's also a glitch with the sizing:

An existing issue in the master branch is that the sidebar percentage/direct input resizing tools do not work well with liquid width themes like twentynineteen. The following is from master:

Yes, not good. However due to the fixed max-width it gets now, it's actually made a little worse.

So what do we do?

Well, maybe this is not an issue that should be "fixed" in the traditional sense. Here's what TinyMCE does:

As you can see — you can size the image as big as you want, limited only by the size of your screen.

Why does this work well? Because what you are doing when sizing an image is applying explicit width and height dimensions. By jumping through hoops to arbitrarily limit these to the dimensions of the block, we are trying our best to chase a good user experience, but due to the added complexity of responsive CSS that a theme can write, this may be a wild goose chase.

So zooming out a little bit, perhaps it's this border that's the primary issue:

That gray block border clearly makes this seem all wrong. Even if it isn't, because the image can be as big as shown, it's just HTML — if you make it bigger than the main column and there's no CSS to keep it there, well this is what you get.

This is another reason why design efforts #18667 lets go of those gray "block is being edited" borders, in favor of focus styles instead. Here's an early work-in-progress branch that shows how that might look:

Yeah, you made the image bigger than there's room for. But if that's what you want, maybe that's okay? Perhaps a "reset size" button would be nice, but that's also a separate exploration.

So as impressive as this PR is, I'm not sure it's the right approach.

[Addison-Stavlo]

Thanks for the detailed response @jasmussen .

First problem is that the max-width is set based on the viewport width, and never updated:

I have updated this, the resizer max-width should now change along with the parent block when the viewport is resized. I had a restriction to only set this once after loading, and have just removed that.

There's also a glitch with the sizing:

I believe this was also a result of the above, this should no longer be an issue in this way.

That gray block border clearly makes this seem all wrong. Even if it isn't, because the image can be as big as shown, it's just HTML — if you make it bigger than the main column and there's no CSS to keep it there, well this is what you get.

I don't think this is necessarily true, there is CSS to keep it there on the front-end. Even if you expand the image larger than the borders and save it, the front end will still render the image's size limited by its parent block. This is a clear disconnect between what a user sees in the editor vs what is shown on the site. In that regard, I think limiting the images size to the parent block in the editor is the correct way to go ( and that seems to be the way the discussion on this was going ). If it needs to be larger, there are always the wide and full width options.

An existing issue in the master branch is that the sidebar percentage/direct input resizing tools do not work well with liquid width themes like twentynineteen.

Hrm, yes this does seem like an issue here too. On Master it has the downside of allowing us to size it larger than the site will actually render, and on this PR it will force a skewed aspect ratio in the editor when trying to do so. Maybe this sidebar tool needs to be revisited, so 100% is relative to the actual parent block in question?

I think there is an intrinsic issue with using the resizer on 'fluid' width themes, and that is that the image is set to an explicit pixel value and is not able to scale up. If user edits their site on a viewport width of 1200, they may stretch their image to match the full width of the main column as they see it. The user might then expect that their site always renders this image at the full width of the main column. However, if someone then loads the site with a wider viewport, the column width will increase and the image itself will have been set to a value smaller than that, making it look too small and 'poorly responsive' to wider widths. 'Wide' and 'Full' width options will cause the image to scale well in all cases, but the standard width doesn't seem to be set up to scale like this making it problematic in fluid width themes.

I feel like this PR does take a step forward, but it does leave more to be addressed (especially in the case of fluid-width themes). Perhaps from here we could go about updating the sidebar resizing controls to be based on the size of the parent block/ main column. As well as updating the image behavior in the 'standard' width to save as a % of the parent block/ main columns size allowing it to properly 'scale up' for fluid width themes when viewed on viewports that are wider than the one used to edit in the first place? 🤔

[jasmussen]

This is getting better fast, impressive work!

I'm still seeing an issue though, if you scale the image (any size, just needs explicit width and height), then size the viewport down and select the image again, it's skewed:

I don't think this is necessarily true, there is CSS to keep it there on the front-end. Even if you expand the image larger than the borders and save it, the front end will still render the image's size limited by its parent block. This is a clear disconnect between what a user sees in the editor vs what is shown on the site.

This is an excellent point.

My immediate thought was that this would be a CSS specificity issue; that if a theme developer provided a ´max-width: 100%;` value for images in the theme, and also in the editor style, it would "just work". But of course it is never that easy. As you suggest, it's probably the behavior of the resizer component we use and how it behaves that makes images not respect editor styles like they should.

My initial concern with this PR was that we were adding code complexity that was likely to break down in the rather complex matrix of image block edgecases (figure display properties, caption widths, floats, scaled or nonscaled, retina or non retina). I wonder, is there an alternative to ResizableBox that we could look at? I seem to recall working with @johngodley a while back and him finding an alternative.

[johngodley]

I wonder, is there an alternative to ResizableBox that we could look at? I seem to recall working with @johngodley a while back and him finding an alternative.

I did look at alternatives (various resizing packages found on npm/github), but settled on ResizableBox, or went in another direction that didn't require ResizableBox.

[Addison-Stavlo]

Thanks again for the quick response @jasmussen . I have made a couple more updates to help address some of this.

1 - I set a height: auto for the image itself in the editor.scss file. This seems to prevent the scaling from getting off-ratio when resizing down as you noted.

2 - I moved the code to update the maxWidth from componentDidUpdate to componentDidMount to wait for the ref to become available to then apply the maxWidth initially and then set the updates to an event listener on resize. This allows the new maxWidth to be set immediately when loaded or on viewport resize (as opposed to how it was staying the same size as you resized the viewport and finally updated once clicked or used).

One thing that is nice is that changing the maxWidth on the resizer this way doesn't change the width - so it will scale-down in proper ratio and be confined to the width of its parent block, but will scale back up the width it was originally set at. This does create a little bit of odd behavior (shown in the last gif below) in trying to resize the image smaller while in a smaller viewport width than it had been set in, but imo this doesn't feel too horrible and is a small price to pay for everything else to be working the way it is now.

Scaling down:

Sidebar resizer tool working well:

The Odd-Behavior I noted:

Essentially the image is set to 1024px width, and the column is set to about 700px. The resizer will reduce the width based on the amount we drag to resize. Since the first attempt at resizing doesn't drag greater than the difference of 324px, when it is released it pops back to looking like nothing changed. In effect the first drag resized the image from 1024 to 850ish (bouncing back to show now change since max-width is still 700ish), the second drag from 850ish to 700ish, then finally the 3rd drag shows a change as we finally drag to resize the image to a width smaller than the max-width. I have staged this in a way that it seems as exaggerated as possible. While I don't think its a huge deal, it would be nice if we could figure out a way to fix this that wont revert any of the other behavioral wins we have gotten along the way so far. Il continue brainstorming on this last part.

[Addison-Stavlo]

I just pushed a change to fix the odd-behavior I noted in my last comment as well.

This is the If/Else condition added to the onResizeStop prop of the resizer component. Now if the image is saved in a size larger than the parent block, the function that saves the new image size at the end of a resizing event sets the change in size based off of the size shown as opposed to the saved size (which may be hundreds of pixels larger).

[jasmussen]

Thank you for valiantly trying to fix this issue.

I happen to agree it would be nice if there was a solid fix for this. I do still worry about the complexity here — adding the auto height and max-width to editor.scss is reminiscent of what we used to do, where the classic responsive rules were output by default for all images. However they were removed in order to give that control to the theme developers. While it's arguably making for a better user experience in the editor, it does not account for themes that provide special CSS to scale images that is out of the ordinary.

But I think perhaps it's time to expand the feedback loop beyond just me and my personal thoughts — I know you're both busy, but I would love some thoughts also from @MichaelArestad @shaunandrews and @kjellr on whether this behavior is a net positive or branching too far away from theme CSS control. Thank you!

[Addison-Stavlo]

adding the auto height and max-width to editor.scss is reminiscent of what we used to do, where the classic responsive rules were output by default for all images. However they were removed in order to give that control to the theme developers.

I would note that we are only adding the height: auto to the editor.scss, and no max-width property there. Given that the resize tool is set up to force maintaining the image's ratio, I don't see how this further restricts anyone.

While it's arguably making for a better user experience in the editor, it does not account for themes that provide special CSS to scale images that is out of the ordinary.

Even before this PR there is no special CSS a theme developer can provide to change the lockAspectRatio prop on the Resizer component, and even after this PR they should still be able to override any CSS made through the cascading properties of the language as usual.

[Addison-Stavlo]

Hi all,

I hope you don't mind the bump, but was hoping we might be able to get some more feedback on this.

I know you're both busy, but I would love some thoughts also from @MichaelArestad @shaunandrews and @kjellr on whether this behavior is a net positive or branching too far away from theme CSS control.

@MichaelArestad , @shaunandrews , @kjellr

Thanks in advance! 😄

[kjellr]

I would note that we are only adding the height: auto to the editor.scss, and no max-width property there. Given that the resize tool is set up to force maintaining the image's ratio, I don't see how this further restricts anyone.

I agree, this doesn't seem like it would be a problem for themes.

I gave this a test in Twenty Nineteen, Twenty Twenty, and the Gutenberg Starter Theme. It seemed to work well in all cases!

[Addison-Stavlo]

I agree, this doesn't seem like it would be a problem for themes.

I gave this a test in Twenty Nineteen, Twenty Twenty, and the Gutenberg Starter Theme. It seemed to work well in all cases!

That is great to hear. Thanks for taking the time to test this out!

I have rebased this, is there anything else we can do to move this along?
The state of the PR still has 'changes requested'.

[jasmussen]

If someone else approves feel free to "dismiss" my change request. AFK for the day. 👌

[Addison-Stavlo]

@youknowriad - any chance you would like to revisit this now that we have some more answers and testing done now?

[youknowriad]

I'm personally still not sure about whether we should add this. It seems very superficial to me to limit the resizing to the current wrapper's size because that same wrapper can have a different size depending on the current size of the screen.

I just created an overflow for example by doing this:

• Resize an image on a big screen
• Resize the screen: make it smaller so that the editor's width also is a bit smaller than the regular centered width
• See the overflow

[Addison-Stavlo]

I just created an overflow for example by doing this:

I am not able to re-create this. This should have been fixed multiple commits ago.

It seems very superficial to me to limit the resizing to the current wrapper's size

We can still set it wider than the current wrapper's size. The difference is now instead of stretching it out as wide as possible in a manner that feels 'broken', now we just enable the '100%' size button in the right hand toolbar.

This issue to limit its size in the editor has been open since 2018 - #12168. The issue has since been re-opened (#19424) and discussed by the design team in slack. Is this not something that we want to do?

[mapk]

I just tested this again and didn't experience any overflow problems. I adjusted the screen multiple times and dragged the handle around on the image... smooth sailing!

The user experience on this is far better than our current implementation. I'd love to get this in, especially since it appears to work well in multiple themes based on Kjell's feedback.

[Addison-Stavlo]

I am going to close this as it has been stagnant for months and I am no longer actively working on or pursuing this. If anyone wants to pick this up feel free...

[paaljoachim]

Are there any gold drops from this PR that would be nice to bring onward and forward?
Or has this been taken care of in other ways along the way?

[Addison-Stavlo]

Or has this been taken care of in other ways along the way?

I think the underlying problem still exists. If you add an image block, you can drag the size to be larger than what is allowable on the front end. Saving and reloading the editor the image still shows up in the editor as oversized.

Editor:

Front-end:

Although the fixes here weren't accepted as a combination of complexity and uncertainty on whether or not limiting the size this way was desired or too superficial.

[paaljoachim]

One concern is that this can cause some problems in a new Gallery block PR that uses nested Image blocks.
#25940

[paaljoachim]

I came across an older issue and tested based on what the author mentioned. Here were my findings. #12168 (comment)

Isabel @tellthemachines has also responded.