fix: Subject location only worked on Safari, incorrect subject location upscaling#1566
Merged
fix: Subject location only worked on Safari, incorrect subject location upscaling#1566
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts focal-point widget drag math and positioning to correctly store subject coordinates, and recompiles the affected JS bundles. Sequence diagram for updated focal point dragging and coordinate storagesequenceDiagram
actor AdminUser
participant Browser
participant FocalPointInstance
participant ImageContainer
participant LocationInput
AdminUser->>Browser: Drag focal point circle
Browser->>FocalPointInstance: _onMouseDown(event)
FocalPointInstance->>ImageContainer: getBoundingClientRect()
FocalPointInstance->>FocalPointInstance: store dragStartX, dragStartY, circleStartX, circleStartY
loop While dragging
Browser->>FocalPointInstance: _onMouseMove(event)
FocalPointInstance->>ImageContainer: getBoundingClientRect()
FocalPointInstance->>FocalPointInstance: compute deltaX, deltaY
FocalPointInstance->>FocalPointInstance: centerX = circleStartX + deltaX
FocalPointInstance->>FocalPointInstance: centerY = circleStartY + deltaY
FocalPointInstance->>FocalPointInstance: centerX = clamp(0, containerRect.width - 1, centerX)
FocalPointInstance->>FocalPointInstance: centerY = clamp(0, containerRect.height - 1, centerY)
FocalPointInstance->>ImageContainer: circle.style.left = centerX px
FocalPointInstance->>ImageContainer: circle.style.top = centerY px
FocalPointInstance->>LocationInput: _updateLocationValue(centerX, centerY)
end
Browser->>FocalPointInstance: _onMouseUp()
FocalPointInstance->>FocalPointInstance: remove drag listeners
Updated class diagram for focal point manager and instanceclassDiagram
class FocalPointManager {
+options
+focalPointInstances
+FocalPointManager(options)
+initialize()
+destroy()
+_init(container)
}
class FocalPointInstance {
+options
+container
+image
+circle
+location
+ratio
+isDragging
+dragStartX
+dragStartY
+circleStartX
+circleStartY
+FocalPointInstance(container, options)
+_updateLocationValue(x, y)
+_getLocation()
+_makeDraggable()
+_onMouseDown(event)
+_onMouseMove(event)
+_onMouseUp()
+_onImageLoaded()
+destroy()
}
FocalPointManager "1" o--> "*" FocalPointInstance
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_onMouseDown,circleStartX/circleStartYstill addr.width/r.heightbut_onMouseMovenow uses these as the top-left values directly, which will cause the focal point to jump by one circle size on drag; consider basingcircleStartX/Yonr.left - n.leftandr.top - n.topinstead. - After the change to position the circle using its top-left (
style.left = centerX,style.top = centerY), the variable names and semantics (centerX,centerY, and_updateLocationValue(centerX, centerY)) no longer reflect centers; clarify whether the stored coordinates are intended to be circle centers or top-lefts and adjust the math/naming accordingly to avoid future confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_onMouseDown`, `circleStartX`/`circleStartY` still add `r.width`/`r.height` but `_onMouseMove` now uses these as the top-left values directly, which will cause the focal point to jump by one circle size on drag; consider basing `circleStartX/Y` on `r.left - n.left` and `r.top - n.top` instead.
- After the change to position the circle using its top-left (`style.left = centerX`, `style.top = centerY`), the variable names and semantics (`centerX`, `centerY`, and `_updateLocationValue(centerX, centerY)`) no longer reflect centers; clarify whether the stored coordinates are intended to be circle centers or top-lefts and adjust the math/naming accordingly to avoid future confusion.
## Individual Comments
### Comment 1
<location> `filer/static/filer/js/addons/focal-point.js:169-170` </location>
<code_context>
- const halfCircle = circleRect.width / 2;
-
// Constrain center to container bounds
- centerX = Math.max(halfCircle, Math.min(containerRect.width + halfCircle, centerX));
- centerY = Math.max(halfCircle, Math.min(containerRect.height + halfCircle, centerY));
+ centerX = Math.max(0, Math.min(containerRect.width - 1, centerX));
+ centerY = Math.max(0, Math.min(containerRect.height - 1, centerY));
// Position circle by its top-left corner (center - radius)
- this.circle.style.left = `${centerX - halfCircle}px`;
- this.circle.style.top = `${centerY - halfCircle}px`;
+ this.circle.style.left = `${centerX}px`;
+ this.circle.style.top = `${centerY}px`;
// Update location value with center coordinates
</code_context>
<issue_to_address>
**issue (bug_risk):** Circle bounds and stored coordinates no longer account for the circle size, which can push the circle partially outside the container and changes semantics of the stored focal point.
Previously `centerX`/`centerY` were true center coordinates: they were clamped with `halfCircle`, rendered via `center - halfCircle`, and stored as centers in `_updateLocationValue`. Now they’re clamped to `[0, width - 1]` / `[0, height - 1]` and used directly for `style.left/top`, making them effectively top-left coordinates while `_updateLocationValue(centerX, centerY)` still suggests center semantics.
To keep center-based focal points, clamp using the circle radius (`halfCircle` or `circleRect.width/height / 2`), render via `center - radius`, and continue storing center coordinates. If you intend to switch to top-left semantics instead, rename the variables, clamp using `containerRect.width - circleRect.width` / `height - circleRect.height`, and update `_updateLocationValue` and `_onImageLoaded` to use top-left consistently.
</issue_to_address>
### Comment 2
<location> `filer/static/filer/js/addons/focal-point.js:165-174` </location>
<code_context>
let centerY = this.circleStartY + deltaY;
- // Get circle dimensions
- const circleRect = this.circle.getBoundingClientRect();
- const halfCircle = circleRect.width / 2;
-
</code_context>
<issue_to_address>
**issue (bug_risk):** Initial placement on image load is no longer centered on the stored focal point, which may cause visual mismatch vs previously saved values.
`_onImageLoaded` previously treated `x`/`y` as the focal point center, offsetting by `circleRect.width/2` and `circleRect.height/2` before setting `style.left/top`. The new code uses `x`/`y` as the top-left, but `_updateLocationValue` still persists the raw drag coordinates assuming center semantics. This will shift existing saved focal points by half the circle size. If the top-left convention is intentional, either normalize/migrate existing values at load time, or keep `_onImageLoaded` using center-based coordinates until the stored data format is updated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fsbraun
changed the title
fsbraun
commented
Jan 13, 2026
…-filer into fix/correct-fp-scaling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1566 +/- ##
=======================================
Coverage 76.95% 76.95%
=======================================
Files 77 77
Lines 3666 3666
Branches 498 498
=======================================
Hits 2821 2821
Misses 675 675
Partials 170 170 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrief
approved these changes
Jan 13, 2026
Collaborator
|
Thanks for the quick response! |
PeterW-LWL
added a commit
to PeterW-LWL/django-filer
that referenced
this pull request
Jan 15, 2026
This was unintentional added in django-cms#1566
4 tasks
fsbraun
added a commit
that referenced
this pull request
Jan 15, 2026
* chore: remove `console.log` statements from codebase This was unintentional added in #1566 * Change django-polymorphic dependency version constraint --------- Co-authored-by: Fabian Braun <fsbraun@gmx.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes an JS issue which miscalculated the subject location from the focal point selector widget:
Fix focal point widget positioning to correctly use top-left coordinates and respect container bounds when dragging or initializing the subject marker.
Bug Fixes:
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.