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

Image block: Implement aspect ratio control #51138

Closed
8 tasks
richtabor opened this issue May 31, 2023 · 23 comments · Fixed by #51545
Closed
8 tasks

Image block: Implement aspect ratio control #51138

richtabor opened this issue May 31, 2023 · 23 comments · Fixed by #51545
Assignees
Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.

Comments

@richtabor
Copy link
Member

richtabor commented May 31, 2023

Based on the design explorations in #38990, let's implement the aspect ratio control for the core/image block.

Visual

CleanShot 2023-05-31 at 15 58 05

Figma Prototype

Objective

If I have an aspect ratio applied to an Image block with a set height and width, I should be able to drop in another image and the aspect ratio, height, and width values are all maintained.

Tasks

  • Add "Aspect Ratio" SelectControl below "Resolution" to core/image block.
  • Set default value of the aspect ratio control to "Original".
  • Update edit/save.
  • If block has aspect ratio and intrinsic height/width, set both inputs to "Auto"
  • If block has aspect ratio value, render Contain/Cover ToggleGroup.
  • If one input is changed, calculate the other (maintaining "Auto" visually).
  • If both inputs are changed, set aspect ratio select to "Custom" and use the non-intrinsic height/width values as the aspect-ratio css.

Follow-up

  • Add support for height, width, and aspect ratio controls for Image blocks without media (placeholder).
@richtabor richtabor added Needs Dev Ready for, and needs developer efforts [Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement. labels May 31, 2023
@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

In order to use the same dropdown we had in the Featured Image block, I think 'Custom' will have to be an option from the beginning.

I can probably auto-fill the width/height controls when it is selected if that's okay. For example, if square ratio is selected and width is 100, then selecting custom will set height to be 100. If both are auto, then it can use the values from the intrinsic width and height.

@richtabor does that seem reasonable to you?

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

I think 'Custom' will have to be an option from the beginning.

One problem I see with doing this is that I'd expect to be able to set the custom aspect ratio and let the image fill the wide width or something, for example. Unfortunately, that won't be possible because the width and height have units.

Likewise, if I wanted the image to be 50% width and 16:10 (custom) aspect ratio, that isn't possible.

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

I'd say 'classic' makes me think 3:2 instead of 4:3. Not sure why we chose 4:3 for the cropping tool, but since we're already diverging from that, maybe we could use 3:2 instead here?

@richtabor
Copy link
Member Author

In order to use the same dropdown we had in the Featured Image block, I think 'Custom' will have to be an option from the beginning.

In trunk, the Post Featured Image block uses "Original" if there is no aspect ratio set:

CleanShot 2023-05-31 at 12 43 35

I can probably auto-fill the width/height controls when it is selected if that's okay. For example, if square ratio is selected and width is 100, then selecting custom will set height to be 100. If both are auto, then it can use the values from the intrinsic width and height.

I'm not following. A user wouldn't select "Custom"—"Custom" would be applied for you, if both the height and width input fields are modified.

@richtabor
Copy link
Member Author

Likewise, if I wanted the image to be 50% width and 16:10 (custom) aspect ratio, that isn't possible.

I think that's fine. The alternative is to set the image in a row/column and use that as the bounding element (which is what most folks do with layouts/patterns that include images anyhow).

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

I think that's fine. The alternative is to set the image in a row/column and use that as the bounding element (which is what most folks do with layouts/patterns that include images anyhow).

Does that mean the meaning of width/height changes if you have an aspect ratio selected?

i.e. When aspect ratio is 'Original' the width and height set, width and height is set in CSS. And when aspect ratio is something else, aspect-ratio is set to the width / height and width is not set.

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

I'm not following. A user wouldn't select "Custom"—"Custom" would be applied for you, if both the height and width input fields are modified.

Even if it gets automatically selected when you set both width and height, 'custom' has to be part of the list.

aspect ratio options

And to be clear, 'original' would still need to exist as an option too–it has a different meaning from 'custom'.

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

If width and height are both set, does the aspect ratio select become disabled?

If so, do you think it will be clear that deleting one of the widths or heights is the way to make it not disabled?

Or if not, what should happen when one of the other aspect ratios is selected?

@richtabor
Copy link
Member Author

And to be clear, 'original' would still need to exist as an option too–it has a different meaning from 'custom'.

Yes, "custom" uses the height/width attributes to define the aspect ratio.

'custom' has to be part of the list.

Good point. I'd almost want it to be labeled "Freeform".

If width and height are both set, does the aspect ratio select become disabled?

No, if height and width are set, then aspect ratio should be "custom" and the height/width are used as the aspect ratio CSS. That way I could drop in a new image and maintain the aspect ratio, re #38990 (comment).

Or if not, what should happen when one of the other aspect ratios is selected?

If I change from a custom/freeform aspect ratio to a defined ratio, like square, then we should keep the width and reset height to auto - to employ the aspect ratio. What do you think of that?

@ajlende
Copy link
Contributor

ajlende commented May 31, 2023

Thanks for the answers!

'custom' has to be part of the list.

Good point. I'd almost want it to be labeled "Freeform".

if height and width are set, then aspect ratio should be "custom" and the height/width are used as the aspect ratio CSS.

Brainstorming ideas here to make it clear that the width and height controls change their meaning. Maybe it could be something like "Use width/height for aspect ratio"? That seems a bit long, but I worry, as it is, that there could be some confusion around what width/height do.

If I change from a custom/freeform aspect ratio to a defined ratio, like square, then we should keep the width and reset height to auto - to employ the aspect ratio. What do you think of that?

👍

@richtabor
Copy link
Member Author

I think it's clear enough that "Custom" will use the height/width values. It's also more likely "custom" is applied when the height and width have been changed (instead of selecting it from the Select).

We can follow-up with additional descriptive text perhaps, if custom is selected; not a blocker imo though.

@richtabor
Copy link
Member Author

I'd say 'classic' makes me think 3:2 instead of 4:3. Not sure why we chose 4:3 for the cropping tool, but since we're already diverging from that, maybe we could use 3:2 instead here?

I've updated the proposal above with this:

CleanShot 2023-05-31 at 15 59 25

I think I like it fine. Can surely iterate further, but not a blocker.

@ajlende ajlende self-assigned this Jun 9, 2023
@ajlende
Copy link
Contributor

ajlende commented Jun 9, 2023

@richtabor I'm struggling again with the details of custom ratios as I've been trying to implement this.

At the moment, the image block dimensions represent pixel values, but in many other places, like the post featured image block, the fields include selectable units. I see units in your design, and I'd rather not add a second implementation for aspect ratio specific to the image block, so I'd like to add units to the image block as part of my PR.

There are three problems that I'm running into while I'm trying to do this.

  1. An aspect ratio is unitless. If we're replacing the width/height controls to represent an aspect ratio, we can only really do that if the units are the same. I can't convert width: 100%; height: 300px; to a ratio without knowing the layout of the page and converting the % to px or vice versa. I was going to just clear the values if the units are different, but that doesn't really feel good—to have the values you typed in disappear when selecting a ratio and having the units disappear as well.

  2. Switching from px units to the default height: auto; max-width: 100%; is going to change the layout of the page. This issue exists even if we create a separate implementation for the image block that doesn't use the unit controls because there are still implicit px units. Someone who selects an aspect ratio is going to be surprised when their 200px width image suddenly becomes full width when they select an aspect ratio. If they want to keep the 200px width they have to know to add a wrapper block where a width can be set. It's removing functionality that people are used to in the image block when an aspect ratio is selected, and that doesn't feel very good.

  3. The Post Featured Image block currently supports setting a custom width with the aspect ratio selected. This functionality would be removed using these designs. It isn't a problem allowing existing blocks to still render the same as it did before, but from a UX perspective, that functionality may be missed.

Problems 1 & 3 could be solved by creating a second implementation for the image block, but eventually we're going to want to use the same implementation in all places—it's just postponing the problem and adding additional complexity with deprecations and backwards compatibility.

@richtabor
Copy link
Member Author

I see units in your design, and I'd rather not add a second implementation for aspect ratio specific to the image block,

so I'd like to add units to the image block as part of my PR.

Switching from px units to the default height: auto; max-width: 100%; is going to change the layout of the page.

I don't think we should change too much with the initial implementation. I'd be fine omitting units; just using px (as-is) and auto.

The Post Featured Image block currently supports setting a custom width with the aspect ratio selected. This functionality would be removed using these designs.

I'm not following. We set them to auto if an aspect ratio is applied (say 1:1), but if you type in an integer value that value should be applied — like in this view below, where I'm adding 100 (px) for the width. The result would be a 100x100px image.

CleanShot 2023-06-10 at 11 24 22
@richtabor
Copy link
Member Author

Someone who selects an aspect ratio is going to be surprised when their 200px width image suddenly becomes full width when they select an aspect ratio.

We shouldn't need to make the image full width/wider — just use the intrinsic width value. If I upload an image that is 800px wide, and select 1:1 aspect ratio, it would be 800x800px.

@ajlende
Copy link
Contributor

ajlende commented Jun 10, 2023

We shouldn't need to make the image full width/wider — just use the intrinsic width value. If I upload an image that is 800px wide, and select 1:1 aspect ratio, it would be 800x800px.

Let's say I upload an image with a resolution of 3840x2160px. In the editor, I changed the width to be 800px so it isn't so wide on the page. Then I select 1:1 aspect ratio. Now the size of the image is 3840x3840px on the page, not the 800px that I want it to be.

@ajlende
Copy link
Contributor

ajlende commented Jun 10, 2023

The css changes from

height: auto;
max-width: 100%;
width: 800px;

to

height: auto;
max-width: 100%;
aspect-ratio: 1;

if the meaning of the height and width controls is supposed to represent an aspect ratio.

@ajlende
Copy link
Contributor

ajlende commented Jun 10, 2023

I don't think we should change too much with the initial implementation. I'd be fine omitting units; just using px (as-is) and auto.

I want to use the same component for Post Featured Image, so that would be removing the existing unit select functionality there.

image
@porg
Copy link

porg commented Jun 12, 2023

When you do this, please also closely observe these aspects:

  1. Resizing via drag-handles: Resizes proportionally.
    • Modifier key to temporarily override proportional resizing: Not offered. Would be fine.
    • Proposed modifier keys should respect design app conventions:
      • SHIFT to toggle proportional resizing
      • ALT to resize from the center (where appropriate).
      • CMD to toggle snapping to nearest preset AR (should this get implemented)
    • As soon as you trigger a manual override, the dropdown menu (as in the mockups) would need to update to "Custom".
      • Further resizing would maintain that "Custom" AR.
      • Using the modifier key again later would update the "Custom" AR again to a new value.
  2. Resize image by number fields
    • Mostly resizes without maintaining aspect ratio.
    • But a few times I experienced that numerical resizing (by either of the three methods: number input, arrow-up/down keys, arrow up/down buttons within numfield) maintained aspect ratio!
    • I cannot reproduce the precondition reliably, but the instance I remember, prior to num-input I had resized via handles.
    • Observed in: WordPress 6.2, plugin Gutenberg 15.9.1, Safari 16.5, macOS 11 Big Sur
@porg
Copy link

porg commented Jun 12, 2023

WordPress.Image.Block.1.drag.handles.resize.proportionally.mp4
WordPress.Image.Block.2.numerical.input.resizes.not.proportionally.mp4
WordPress.Image.Block.3.numerical.input.sometimes.resizes.proportionally.mp4
@ajlende
Copy link
Contributor

ajlende commented Jun 14, 2023

@porg Thanks for taking the time to make some videos. I'll keep them in mind when working on this, but dragging and modifier keys are related to the ResizableBox component which would add too much to the scope of this issue. Would you mind opening up a new separate issue with those things?

@ajlende
Copy link
Contributor

ajlende commented Jun 14, 2023

@richtabor I think I was just confused earlier by the units disappearing in the design. After coming back to this after a break I think what you intended was having the width/height controls always control the width or height CSS properties like always.

The "Custom" aspect ratio shouldn't actually create a custom aspect ratio, and it probably doesn't even need to set the aspect-ratio property if both width and height are specified in CSS.

@richtabor
Copy link
Member Author

The "Custom" aspect ratio shouldn't actually create a custom aspect ratio, and it probably doesn't even need to set the aspect-ratio property if both width and height are specified in CSS.

As long as the aspect ratio is unset if both height and width are changed.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jun 15, 2023
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] Enhancement A suggestion for improvement.
4 participants