Skip to content

Image Cropper: Ensure to check for focal point #21275

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.

Sign up for GitHub

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

Open
bjarnef wants to merge 4 commits into
base: main
Choose a base branch
from
+5 −3
Open

Image Cropper: Ensure to check for focal point #21275

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a98397c
Ensure to check for focal point before accessing left and top propert…
bjarnef Jan 2, 2026
3604b40
Merge branch 'main' into feature/image-cropper-focal-point-nullable
bjarnef Jan 7, 2026
891bed7
Merge branch 'main' into feature/image-cropper-focal-point-nullable
bjarnef Jan 28, 2026
1a462b3
Merge branch 'main' into feature/image-cropper-focal-point-nullable
bjarnef Feb 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ...packages/media/media/components/input-image-cropper/image-cropper-focus-setter.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ export class UmbImageCropperFocusSetterElement extends UmbLitElement {
@property({ attribute: false })
set focalPoint(value) {
this.#focalPoint = value;
this.#setFocalPointStyle(this.#focalPoint.left, this.#focalPoint.top);
if (this.#focalPoint) {
this.#setFocalPointStyle(this.#focalPoint.left, this.#focalPoint.top);
}
this.#onFocalPointUpdated();
Comment on lines 41 to 45
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The null check is added after assigning the value to this.#focalPoint. If value is null or undefined, the private field #focalPoint will be set to null, which contradicts its initialization on line 50 where it has a default value of { left: 0.5, top: 0.5 }. This could cause issues elsewhere in the code that expects #focalPoint to always be a valid object.

Consider either:

  1. Not assigning null values: Check if value is truthy before assignment
  2. Always calling #onFocalPointUpdated() regardless of the value to handle the null state consistently

The current implementation sets #focalPoint to null but still calls #onFocalPointUpdated() which now has a null check added at line 107.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@bjarnef bjarnef Jan 4, 2026

Choose a reason for hiding this comment

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

It always call #onFocalPointUpdated().

It just ensure #focalPoint is not null or undefined before accessing left and top properties.

}
get focalPoint() {
Expand Down Expand Up @@ -77,7 +79,7 @@ export class UmbImageCropperFocusSetterElement extends UmbLitElement {
async #initializeImage() {
await this.updateComplete; // Wait for the @query to be resolved

if (!this.hideFocalPoint) {
if (!this.hideFocalPoint && this.focalPoint) {
this.#setFocalPointStyle(this.focalPoint.left, this.focalPoint.top);
}

Expand All @@ -102,7 +104,7 @@ export class UmbImageCropperFocusSetterElement extends UmbLitElement {
}

#onFocalPointUpdated() {
if (this.#isCentered(this.#focalPoint)) {
if (this.#focalPoint && this.#isCentered(this.#focalPoint)) {
this.#resetCoords();
}
}
Expand Down
Loading