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

[Experiment] Add freeform image cropper component #63335

Draft
wants to merge 20 commits into
base: trunk
Choose a base branch
from

Conversation

kevin940726
Copy link
Member

@kevin940726 kevin940726 commented Jul 10, 2024

What?

The first draft of implementing a freeform image cropper component aiming to solve #22582.

Note

This PR is a WIP.
Please see the features and TODOs section below. We want to open a draft PR first to gather some very early feedback to ensure we are on the right track.

Why?

See #22582. The current image cropping tool lacks some advanced features we want to support. The library we used doesn't provide the features we need, and we can't find other third-party libraries that do. This component is an attempt to implement our own solution and potentially use it in the Media Library revamp.

How?

Lots of math 🧠.

Add a new <ImageCropper> component in the @wordpress/components package. Currently, it's not exported at all and only available in the storybook environment for testing. We'll export it as a private component next once we reach a point where it can be integrated into the block editor.

The basic usage is as follow:

<ImageCropper.Provider width={250} height={200} src={IMG}>
  <ImageCropper />
</ImageCroppper.Provider>

See the storybook for advanced usage.

Features and TODOs

Here's an unexhaustive list of supported features and TODOs with demo recordings.

  • Resizing

    • ✅ Resize the cropping window

      Kapture.2024-08-01.at.15.19.18.mp4

    • ✅ Scale after resizing

      Kapture.2024-08-01.at.15.19.18.mp4

    • ✅ Resize after rotation

      Kapture.2024-08-01.at.15.20.12.mp4

    • Resize max bounds

      It should not be resized to be bigger than the image. Additionally, zooming out while resizing near the max bounds is nice-to-have. Here's a demo from the Google Photos iOS app:

      RPReplay_Final1722504920.mov

  • Dragging

    • ✅ Drag/pan the image

      Kapture.2024-08-01.at.15.23.01.mp4

    • ✅ Drag max bounds

      Kapture.2024-08-01.at.15.23.34.mp4

    • ✅ Drag after rotation

      Kapture.2024-08-01.at.15.25.06.mp4

  • Rotation

    • ✅ Rotate the image around the center of the cropping window

      Kapture.2024-08-02.at.01.40.35.mp4

    • ✅ Rotate 90 degrees (clockwise and counterclockwise)

      Kapture.2024-08-01.at.15.25.06.mp4

    • ✅ Scale after rotation

      Kapture.2024-08-01.at.15.28.32.mp4

    • Rotation animation

      The cropping window should also rotate with the image.

      Kapture.2024-08-01.at.15.29.07.mp4

  • Flipping

    • ✅ Flip the image horizontally

      Flipping the image vertically can be achieved by rotating and flipping.

      Kapture.2024-08-01.at.15.30.12.mp4

    • ✅ Flip animation

      Kapture.2024-08-01.at.15.30.12.mp4

  • Zooming

    • ✅ Zoom in/out the image around the cursor's position by pinching

      Kapture.2024-08-01.at.15.30.50.mp4

    • ✅ Zoom after rotation

      Kapture.2024-08-01.at.15.31.44.mp4

    • ✅ Zoom max and min bounds

      Currently, the maximum and minimum scale bounds are set to 1 and 10 respectively.

    • Zoom with a mouse or keyboard.

    • Resize after zooming out

  • Aspect ratio

    • Lock/Unlock the aspect ratio
  • Responsive

    • Resize the container after resizing the window

      It works but has some bugs 😅

  • Accessibility

    • Keyboard shortcuts
    • Screen reader support
    • Reduced motion support
  • Styling

    • Designers' approval
  • Integration

  • Testing

    • Unit tests
    • End-to-end tests

Testing Instructions

  1. Run the storybook dev server (npm run storybook:dev)
  2. Visit http://localhost:50240/iframe.html?args=&id=components-imagecropper--default to view and play with the component.

Testing Instructions for Keyboard

This component is currently not accessible to the keyboard, but it's on the plan.

Screenshots or screencast

@kevin940726 kevin940726 added [Feature] Media Anything that impacts the experience of managing media [Type] Experimental Experimental feature or API. [Feature] Component System WordPress component system labels Jul 10, 2024
@kevin940726 kevin940726 self-assigned this Jul 10, 2024
@jasmussen
Copy link
Contributor

Nice one, took it for a quick spin:

cropping

This is looking solid and promising. Small bug with zooming too much and it flips the image.

This is very nascent, so hard to fully review until it's in a branch, implemented for the Image block, but a few general observations/thoughts:

  • The dark frame around it we probably want to avoid.
  • We should not have 8 resize handles. In the editor for resizing the image, we have 2, right and bottom. There's an open question around how the resize handles left and right will work when this gets transplanted into the canvas, that's likely also related to the dark frame for "scaling up", so those are some questions we will need to consider later. But we can at least remove the resize handles in the corner.
  • We'll need to think about where to place the slider for rotating the image, and whether that slider is the right interface for it. E.g. it can't live inside the block toolbar directly. Something like the gradient direction tool may be useful here.

Nice work.

@talldan
Copy link
Contributor

talldan commented Jul 10, 2024

We'll need to think about where to place the slider for rotating the image, and whether that slider is the right interface for it. E.g. it can't live inside the block toolbar directly. Something like the gradient direction tool may be useful here.

If the number of resize handles were reduced, there could be rotation handles on the corners, or some applications have a handle that extends out like this:
Screenshot 2024-07-10 at 3 18 33 pm

@andrewserong
Copy link
Contributor

This is feeling really nice to use so far! Another couple of questions, and possibly for @jasmussen (and once we integrate with the image block):

  • How might we expose aspect ratio controls at the same time? The dropdown in the block toolbar can be a bit hidden for users, I've seen some people miss them altogether.
  • The scrollwheel to zoom feature works pretty well for me, but I didn't notice it at first. Would we also want to have zoom in / zoom out buttons, alongside the rotation control?
@kevin940726
Copy link
Member Author

We'll need to think about where to place the slider for rotating the image, and whether that slider is the right interface for it.

Most cropping tools that inspire this PR have their rotation slider below the image. For instance, here's Google Photos web:

Kapture.2024-07-11.at.13.47.33.mp4

The dimmed background is also needed in this case. I think we can also consider placing the slider at the bottom of the editing canvas? It can be toggled with a click in the block toolbar since it's an advanced usage.

there could be rotation handles on the corners, or some applications have a handle that extends out like this

I also considered this, but IMO this sort of UI only makes sense when you're rotating the cropping window itself, not the background image. Because this PR currently assumes that every cropped image should still be an unrotated rectangle, rotating the cropping window doesn't make sense here.

The scrollwheel to zoom feature works pretty well for me, but I didn't notice it at first. Would we also want to have zoom in / zoom out buttons, alongside the rotation control?

I think we should, at least for feature completion of accessibility. We can potentially add more keyboard controls too (Ctrl++/-?) in addition to the UI control.

@jasmussen
Copy link
Contributor

How might we expose aspect ratio controls at the same time? The dropdown in the block toolbar can be a bit hidden for users, I've seen some people miss them altogether.

We need a fresh design for this. The following much older design could be a starting point, but it needs a fresh take that is more like the existing and with a smaller visual diff.
Crop and rotate in isolation
Crop and rotate in-canvas i2

For example, the modal may look fun at a quick glance, but let's not do this, it's not compatible with cropping tools that are likely to also exist in a future media library, and it also is a clunky amount of additional steps compared to a more lean and light-weight in-canvas cropping tool. I'm sharing it only because there may be some elements as far as aspect ratio that may be worth revisiting.

Note specifically how the block toolbar itself becomes modal. Instead of having a single image block toolbar that grows wider with crop tools, this one proposes a drilldown effect for the block toolbar, affording better and clearer tools. Not sure how feasible, but it's worth resurfacing.

Figma.

The scrollwheel to zoom feature works pretty well for me, but I didn't notice it at first. Would we also want to have zoom in / zoom out buttons, alongside the rotation control?

I like the slider in the toolbar, though I'm also aware that this is substantially more difficult to get right, than it looks at a glance. So also here it would seem useful to free up space in the block toolbar, however we can. Since it's already modal through the apply/cancel actions, perhaps that can be a way to take up more space in the block toolbar?

@talldan
Copy link
Contributor

talldan commented Jul 11, 2024

I also considered this, but IMO this sort of UI only makes sense when you're rotating the cropping window itself, not the background image. Because this PR currently assumes that every cropped image should still be an unrotated rectangle, rotating the cropping window doesn't make sense here.

That's a good point 😄

I think my main concern would be how this works in terms of inline editing when there are other blocks after the image, and also when there's a caption in that position. Something more minimal might be easier to wrangle. Having secondary UI controls in the inspector or toolbar is still always an option (and might be needed for accessibility).

It can be something to iterate on though, it'll be interesting to see the designs.

@kevin940726
Copy link
Member Author

Kapture.2024-07-16.at.18.30.39.mp4

I added the rotation (90deg clockwise and counter-clockwise) feature. The reason angle and turns(can't think of a better name 😅) are separated is because the value for angle is between -45 to 45, and we need a way to distinguish between angle: 45, turns: 0 and angle: -45, turns: 1.

Currently, getImageBlob for rotated images doesn't work as expected. I probably need some help on some math there and simplify the logic in reducer 😅.

};
}

function imageCropperReducer( state: State, action: Action ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to add some inline comments to the code within this reducer. Geometry code like this is notoriously hard to read.

I spotted a bug, if I resize twice from the bottom-right, the top-left of the resizable box jumps position incorrectly:

Kapture.2024-07-17.at.12.28.41.mp4

I found it a little difficult to understand the intention of the code in the RESIZE_WINDOW action, so some comments would help greatly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it a little difficult to understand the intention of the code in the RESIZE_WINDOW action, so some comments would help greatly.

Yeah, that's one of the things I need help with 😅. Half the time I don't even know what I'm doing 🤦. I'd need to re-organize my thoughts and refactor this in a way that makes it easier to write comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here might help - #63335 (comment).

There might be one or two things I got wrong because I'm mostly doing this as a mental exercise, but I think a good rule is to not modify the underlying image coordinates until the user hits 'apply'. Instead store the manipulations in a 'transform' object that describes how the image has been manipulated. It might be similar to what you're doing already, but I found it difficult to tell.

I think the important thing is to get the reducer working correctly and then have the UI work from that state. It might be challenging with some of the opinionated UI pieces like resizableBox, but a solid foundation for the maths will make everything work much more reliably.

Comment on lines 7 to 50
export type State = {
// The container/image's width.
width: number;
// The container/image's height.
height: number;
// The rotation angle between -45deg to 45deg.
angle: number;
// The number of 90-degree turns.
turns: 0 | 1 | 2 | 3;
// The zoom scale.
scale: number;
// Whether the image is flipped horizontally.
flipped: boolean;
// The offset position of the cropper window.
offset: Position;
// The position of center of the image.
position: Position;
// The size of the cropper window.
size: Size;
// Whether the cropper window is resizing.
isResizing: boolean;
// Whether the image is dragging/moving.
isDragging: boolean;
};
Copy link
Contributor

@talldan talldan Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good aim could be to model the state like this:

State = {
    image: {
		// the image would always have a `top`/`left` of zero, because we apply `transform.translation` to move it. A rect is a web friendly storage format so might as well use it to keep things consistent.
        rect: { top, right, bottom, left, x, y, width, height }
    },
    imageTransforms: {
        rotate: number,
        scale: number, 
        translate: Vector2<number>,
        reflect: Vector2<bool>
    },
    // the resizable box
    clippingRect: { 
        rect: { top, right, bottom, left, x, y, width, height }
    },
    // UI states
    isResizing: bool,
    isDragging: bool,
}

I'm visualizing the image manipulation as two separate things.

Firstly, if you completely disregard the 'clipping rect', there are distinct changes the user can make to the image. They can rotate it (slider), zoom it (mouse wheel) and translate it (drag it). As the user makes those changes the deltas are added to the transform state. The image state doesn't change at all until the user clicks 'apply', at which point the transform state is reset to zero.

Keeping the transform as a state like this is pretty important as the scaling and rotation need to be applied before translation (moving) when transforming something geometrically around its center (assuming the origin is the center of the image, if not it can be translated to its center first before scaling, and then translated back again).

The clipping rect is a separate thing on top of that, but from what I can tell, it doesn't affect the underlying image until the user applies the crop, so I think a rect is the only state needed for it.

There might be more needed after that, for example I notice that the clipping rect and the image are often translated together (after resizing the clipping area, the image and clipping rect are recentered), but I think it'd be better to store that as a separate transform object (e.g. viewTransform) that's applied to the resizable box and the image (after the other transforms) so that they both move by the exact same amount.

This reflects pretty closely how video games work in having a series of 'transforms' (model, view, projection), though they store them in matrices:
https://gamedev.stackexchange.com/questions/178643/the-view-matrix-finally-explained

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also going to comment that turns might be more portable/easily understood as a rotate value, possibly in combination with the deg/radian?

Copy link
Member Author

@kevin940726 kevin940726 Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean a rotate and a degree value? I never like the naming of turns so I might switch to that 😅.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean a rotate and a degree value?

Oh, the controls are separate, right? Then, yes 😄

var( --wp-cropper-window-x ),
var( --wp-cropper-window-y )
);
box-shadow: 0 0 0 100vmax rgba( 0, 0, 0, 0.5 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic start. Awesome work @kevin940726

What do you think about adding some simple guide rulers? I'm using Google photos as inspiration 😄

Kapture.2024-07-22.at.15.39.57.mp4
Example
export const Resizable = styled( ResizableBox )`
	transform: translate(
		var( --wp-cropper-window-x ),
		var( --wp-cropper-window-y )
	);
	box-shadow: 0 0 0 100vmax rgba( 0, 0, 0, 0.6 );
	&:active {
		box-shadow: 0 0 0 100vmax rgba( 0, 0, 0, 0.4 );
		&::after,
		&::before,
		${ Draggable }::after, ${ Draggable }::before {
			position: absolute;
			padding: 0;
			width: 1px;
			height: 100%;
			overflow: hidden;
			left: 33.33%;
			content: ' ';
			background: rgba( 255, 255, 255, 0.33 );
			z-index: 1000;
			display: block;
		}

		&::before {
			right: 33.33%;
			left: auto;
		}

		${ Draggable }::before {
			left: auto;
			width: 100%;
			height: 1px;
			top: 33.33%;
		}

		${ Draggable }::after {
			left: auto;
			width: 100%;
			height: 1px;
			bottom: 33.33%;
			top: auto;
		}
	}
`;

Also, I know it's eye candy, but a subtle animation during the post-crop translate (not dragging) would look pleasing to my eye at least 🤗

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think we can also show it on dragging and rotating too!.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit that includes the rulers and the animations!

Kapture.2024-07-29.at.15.46.56.mp4
);
Default.args = {
src: 'https://upload.wikimedia.org/wikipedia/commons/thumb/3/34/Hydrochoeris_hydrochaeris_in_Brazil_in_Petr%C3%B3polis%2C_Rio_de_Janeiro%2C_Brazil_09.jpg/1200px-Hydrochoeris_hydrochaeris_in_Brazil_in_Petr%C3%B3polis%2C_Rio_de_Janeiro%2C_Brazil_09.jpg',
width: 250,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be helpful to provide an example of a crop container that resizes with the window/parent container?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely! It's on the TODO list but not supported yet. I think we can first implement the core features and then make it responsive. We also need to fix that it's not cropping the image in its original resolution 😅.

@kevin940726
Copy link
Member Author

I added a section of features and TODOs in the PR description. Please let me know if any of them isn't clear! I'll try to add more too.

@ajlende
Copy link
Contributor

ajlende commented Aug 1, 2024

Related to resizing the container, when we're using this in the block editor for the image block, the crop area should be the same size as the final image. One of the goals of core block editing is to make the editing mode look as close to the preview mode as possible.

Comment on lines 78 to 81
dispatch( {
type: 'ROTATE_CLOCKWISE',
isCounterClockwise: true,
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this API is a little confusing as one onehand it's ROTATE_CLOCKWISE and then on the other it's isCounterClockwise: true. Maybe the action type could be ROTATE_90 or something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I renamed it to ROTATE_90_DEG in 2fd0c69.

Comment on lines +97 to +98
angle: 0,
rotations: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, why does the angle needs to be stored separately to rotations? The two feel like the same thing to me.

Copy link
Contributor

@ajlende ajlende Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are the same thing, but this matches the UI of the Google photos cropper that many of these interactions seemed to be based off of. One could deconstruct any angle into these two parts, but I haven't convinced myself which way would be better. Mostly thinking in terms of undo/redo.

image
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! We need two separate values here so that we can distinguish between angle: 45, rotations: 0 and angle: -45, rotations: 1, which are both 45 degree but represent different states in the UI.

@ajlende
Copy link
Contributor

ajlende commented Aug 2, 2024

I was playing around with more image croppers today, and the closest ones to what I had in mind for image block cropping were Adobe Express and Canva.

Adobe Express

Adobe Express has two resizable boxes, essentially; one for the crop region and one for the image. It showcases how rotation can be done in-canvas without a slider like Google.

The part that doesn't work for us is that the positioning post crop will be different for us since we're working with an element that's part of the regular DOM flow and rules instead of a canvas where everything is absolutely positioned.

Screen.Recording.2024-08-02.at.4.13.54.PM.mov

Canva

Canva "solves" the problem by not allowing the crop area to be changed while editing, so the size of the crop area in the page always matches the final result.

Canva also adds a feature that keeps the crop are in bounds of the image that Adobe Express doesn't handle.

Screen.Recording.2024-08-02.at.3.56.36.PM.mov

Google Photos

However, Google photos handles the reflow quite nicely by resizing the image on mouse up.

Screen.Recording.2024-08-02.at.4.39.35.PM.mov

Combining them

I think we should be able to combine the Adobe Express double box model with the Canva crop area bounds constraints and the Google Photos reflow mechanic to make something that will work in both the context of inline editing for the image block and as a fixed editor in the admin redesign.

@kevin940726
Copy link
Member Author

I think we should be able to combine the Adobe Express double box model with the Canva crop area bounds constraints and the Google Photos reflow mechanic to make something that will work in both the context of inline editing for the image block and as a fixed editor in the admin redesign.

What do you have in mind for using the double box model for inline editing? If we reflow(scale) the image after resizing then the image box will grow bigger to the point that it might be difficult to fit the box into the screen 🤔.

If the idea is to adopt the rotation button, then I think we can still do that using the Google Photos UX? Possibly something like a button rather than a slider below the cropper window?

I think we can use some design feedback here too 😆.

@talldan
Copy link
Contributor

talldan commented Aug 6, 2024

If we reflow(scale) the image after resizing then the image box will grow bigger to the point that it might be difficult to fit the box into the screen 🤔.

That's the kind of thing that concerns me too. Along with the possibility of having elements of the cropping tools overlapping other blocks. Engaging spotlight mode during cropping might be an option to keep focus on the image block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Feature] Media Anything that impacts the experience of managing media [Type] Experimental Experimental feature or API.
6 participants