Adjust Image Cropper handle (focal point)

[bjarnef]

Description

This PR adjust focal point (handle) in Image Cropper to be more consistent with handle in Color Area. https://github.com/umbraco/Umbraco-CMS/issues/16672

It moves the focal point based on x and y coordinates in the container on mouse click or touch. Furthermore it moves in the handle / focal point on keyboard arrows, while holding down Shift key allows to fine-tune position.

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [x] Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

To re-use some existing logic and make the UI/UX consistent.

How to test?

Screenshots (if appropriate)

https://github.com/umbraco/Umbraco.CMS.Backoffice/assets/2919859/a9d80fce-ed13-4a5d-8fc4-e458c69054ce

Checklist

• [ ] If my change requires a change to the documentation, I have updated the documentation in this pull request.
• [x] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.

[bjarnef]

There a minor issue when resetting focal point, where it also need to reset the coords like it does on image loaded here: https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/files#diff-edf1088805f907d36474f517086a2f1a7b8c0f07f1aeaa66d6d1861da99a6d12R72-R74

It am not sure the disabled property is something we need, but I think it would be a nice feature. In the current backoffice if a property editor is readonly, I think the focal point is just hidden.

If think it may be useful to have a feature to show the focal point, but maybe some editors should allow the move it or in a special context like split view or when not active language variant.

[bjarnef]

@iOvergaard @JesmoDev please have a look 😎

[nielslyngsoe]

Hi @bjarnef looks good.

I also see that you are trying to fix a few other things, like keyboard navigation etc. Thats cool, would it be possible to finish it in near future?

Otherwise, I would recommend splitting your PRs up. Small PRs that are easy to review can sometimes be really good as they are fast to review and we can then get them merged in. In this way, we can keep the list of open PRs to a minimum. (as I was up for merging this, but its not done in its current state, just the change of the focus point look) :-)

[bjarnef]

@nielslyngsoe it should be ready to review, but any feedback are welcome 😉 I am still learning by doing 🤭😅😎

[bjarnef]

Do we have any way to know when reset focal point happens? It seems firstUpdated() happens on init and updated() each time focal point is moved, but we only want to update coords to center when focal point reset is triggered.

[bjarnef]

I found a workaround to update coords if focal point is updated/reset to center using the reset button. https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/commits/11666ac4e506924e5c308cac709f0fb8bf728364

[nielslyngsoe]

@bjarnef yes, that is one solution. But the one we mainly use is to append setter and getter methods to the property itself. You should be able to fairly simple find a lot of solutions doing so. A typical one is the value and config property on any Property Editor UI.

[bjarnef]

@nielslyngsoe I tried the following instead. https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/commits/03bcee207030af7fb2ba3dd4c30dd423329590f0

A bit similar to the UmbInputImageCropperElement here: https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/11efe83172c254e4027702a920fc6bd3cfe41a8a/src/packages/core/components/input-image-cropper/input-image-cropper.element.ts#L19

However to make #resetCoords() working it need to happen after image has loaded and CSS applied here to get the rendered size. https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/files#diff-edf1088805f907d36474f517086a2f1a7b8c0f07f1aeaa66d6d1861da99a6d12R92

Or call #setFocalPointStyle() but it requires focalPointElement to be found via query which seems to happen after property value setter.

[bjarnef]

@JesmoDev this probably conflicts a bit with changes in https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1052

This PR makes the Image Cropper handle works using keyboard navigation and use same logic as color picker handle.

I could use some help with implementation of the property as @nielslyngsoe suggested - see last commit.

[bjarnef]

@iOvergaard do you know how we can solve the last part? I think it would be great to use same handle as in Color Picker and make it accessible as well.

[bjarnef]

@iOvergaard @nielslyngsoe could you have another review at this? Try moving focal point around and also navigate it with keyboard using arrow keys and eventually hold shift key to fine adjust.

Also while focal point not in center navigate reset button, hit enter and after focal point is moved reset to center, focus focal point again using keyboard and moving it should move it from center.

The handle/focal point is now re-using same logic as in handle of color picker.

[bjarnef]

@iOvergaard I fixed the tests as well.

There's a lot of @typescript-eslint/no-explicit-any warnings though.

[bjarnef]

@nielslyngsoe could you have another look on this? 😁😎

[bjarnef]

@iOvergaard @nielslyngsoe anything I can help with to merge/close this one? :)

[bjarnef]

@iOvergaard sorry for pinging you again, but anything preventing us from re-using the existing drag logic used in color picker in Image Cropper focal point as well? 🤗

[iOvergaard]

Hi @bjarnef

Just wanted to get back to you. We would like to look into this functionality, but we are getting pretty close to the deadline for 14.0, so it will probably have to wait until after release. Meanwhile, I see a lot of out-commented code in your PR - do you need us to look further into it or are you going to update it?

[bjarnef]

Hi @iOvergaard

Yes, the commented code can be removed. When testing it seemed to work well to me and it use using the same logic as color picker instead of handle the handle different in Image Cropper :)

The only issue I had previous was resetting the handle, when clicking the reset button, but this should also have been fixed.

Only thing left may be trigger the UmbChangeEvent, which I noticed recently was refactored, when handle position change, but it isn't crucial.

Finally just combine it with the other PRs from @JesmoDev as it may get some merge conflicts depending, which PRs are merged first :)

[bjarnef]

@iOvergaard I have fixed the merge conflicts.

A few things I noticed:

• drag function seems not to avaiable from @umbraco-cms/backoffice/external/utils, but clamp is available from @umbraco-cms/backoffice/utils, but it seems to be redundant from UI library.

Backoffice: https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/dd1d4fe47e842946ee9431eb71d41a5864411832/src/packages/core/utils/math/math.ts#L31-L33

UI library: https://github.com/umbraco/Umbraco.UI/blob/e159c43696e787f12165f5a19da024fffc06b64e/packages/uui-base/lib/utils/math.ts#L4

not sure why it also has a clamp function here: https://github.com/umbraco/Umbraco.UI/blob/e159c43696e787f12165f5a19da024fffc06b64e/packages/uui-loader-bar/lib/uui-loader-bar.element.ts#L5

and in backoffice a private clamp function here: https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/dd1d4fe47e842946ee9431eb71d41a5864411832/src/packages/core/components/split-panel/split-panel.element.ts#L92-L94

any reason to re-invent the wheel? :)

• wrapperElement and focalPointElement isn't HTMLImageElement but HTMLElement

[bjarnef]

I think it resolved the issues. However on initial change of focal post (click or drag) the value is undefined and thus return an console error. It seems to log twice where the second time it has the initial value of focal point.

[bjarnef]

@iOvergaard any update on this? Can I help with anything to close this?

[iOvergaard]

@iOvergaard any update on this? Can I help with anything to close this?

@bjarnef Going to fix the clamp issue in UUI for the upcoming 1.9 at least. Then we'll need to have another look at this PR. I don't know off the top of my head if we have to fix the media picker as well, which now also has a focal point.

[bjarnef]

@iOvergaard @leekelleher I have updated this with latest changes. If media picker has a focal point I guess it would work, but haven't tested.

I guess I would need to run CMS project on same time to test we media? Is it possible the media library to have mock image similar to image cropper?

[iOvergaard]

I guess I would need to run CMS project on same time to test we media? Is it possible the media library to have mock image similar to image cropper?

Unfortunately not because the backend is currently using relative URLs for images. You can change the branch in the submodule on the umbraco-cms repository and build using npm run build:for:cms and test it out directly in the CMS, though.

[bjarnef]

@iOvergaard currently I have backoffice in a separate location.

It is possible to change path in .gitmodules:

[submodule "src/Umbraco.Web.UI.Client"]
	path = src/Umbraco.Web.UI.Client
	url = https://github.com/umbraco/Umbraco.CMS.Backoffice.git

or clone backoffice repo directly to Umbraco.Web.UI.Client folder?

[iOvergaard]

@bjarnef The Umbraco.Web.UI.Client folder is a clone of the backoffice. You can go into the folder and run git commands, e.g. git checkout main.

If you want to use your fork there, you can add it as a remote, e.g. git remote add bjarnef https://github.com/bjarnef/Umbraco.CMS.Backoffice.git and then git checkout bjarnef/feature/image-cropper-handle

[bjarnef]

@iOvergaard thanks. I also needed to fetch from upstream/remote before checking out the branch.

It works great with focal point in media picker as well.

With focal point

Without focal point

A few adjustments:

• Added top margin about actions buttons below image cropper, so if focal point is a bottom it doesn't overlap buttons.
• Only crosshair cursor on wrapper, when focal point is visible.
• Cancel drag/click in wrapper (area) when focal point is hidden.
• Move cursor to image in crop mode as in the old backoffice.

The slider could probably round the decimals in tooltip to three decimals and while dragging it seem the handle a moved when delayment compared to slider in old backoffice.

The crops should probably also show Label instead of Alias on the right as in old backoffice. It seems the array of crops in UmbImageCropperPropertyEditorValue type didn't have a label property as in configuration.

Besides that I think the image cropper as property on e.g. a media item is a bit small compared to the old backoffice, which makes it more difficult to place the focal point on small details/objects in image.

[bjarnef]

I included label property to the types, but I seems the label property is missing from endpoint at local crops in media picker property value?

I noticed it was shown at Image Cropper in media section.

but then I noticed it actually has e.g. "Square" as alias and the label was undefined.

Anyway shouldn't the endpoints map the label as well? Looking at the configuration in .NET it doesn't seem we have this property (but it is stored?) I also noticed alias is nullable in media picker crops, but non-nullable in image cropper configuration. Should probably not be nullable as it is required, but I guess it would be a breaking change to fix.

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/PropertyEditors/MediaPicker3Configuration.cs#L38

https://github.com/umbraco/Umbraco-CMS/blob/9a49ab712b0f244c3e6522ecf654c072eaacba7b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperConfiguration.cs#L18

[iOvergaard]

@bjarnef

The crops should probably also show Label instead of Alias on the right as in old backoffice. It seems the array of crops in UmbImageCropperPropertyEditorValue type didn't have a label property as in configuration.

Yes, you are right. We need to look that up in the property editor, which we already did, but didn't use for stored crops. I have fixed that in #2145. Thanks for your discovery here!

[bjarnef]

@iOvergaard feel free to make additional commits to this PR :)

I noticed some other behavioural oddities.

• Change media item allow me to select a folder (it wasn't possible in old backoffice)
• Without focal point hidden and dragging image it doesn't seem preview thumbnail updates.
• Changing current selected crop reset zoom position which wasn't the case in old backoffice https://github.com/umbraco/Umbraco-CMS/issues/16821
• I can't navigate crops using keyboard https://github.com/umbraco/Umbraco-CMS/issues/16820

Another minor issue I noticed is when opening developer console the image isn't re-calculated. Not a big issue, but I think it would similar a virtual keypad open on tablet. Anyway it worked nicer in old backoffice :)

When not in crop mode (at Media "crop" menu item), we probably also need checkered background as before to see a white logo/image?

[iOvergaard]

@bjarnef All good proposals. Would you add them as separate issues, please?

Change media item allow me to select a folder (it wasn't possible in old backoffice)

This one we are aware of, we currently have no way of detecting if a media item is actually a folder - all media items can contain subitems, but obviously does not make sense to select one if it itself doesn't have an umbracoFile. We need more advanced checks for that.