fix/zoom - #254
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes zoom focal-point anchoring for offset containers and adds configurable fit-relative minimum zoom.
Changes:
- Converts wheel and drag focal points to annbox-relative coordinates.
- Adds
min_zoom_fit_ratiowith tests and documentation. - Adds release metadata and an offset-container demo.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.js |
Implements focal conversion and zoom floor. |
src/toolbox.ts |
Routes button zoom through the setter. |
src/configuration.ts |
Adds the zoom-floor configuration. |
index.d.ts |
Exposes the zoom setter. |
tests/e2e/wheel-zoom-focal-point.spec.js |
Tests focal-point anchoring. |
tests/e2e/min-zoom-fit-ratio.spec.js |
Tests zoom-floor behavior. |
demo/offset-container.html |
Adds an offset-container test demo. |
demo/set-annotations.html |
Enables the zoom floor. |
demo.js |
Lists the new demo URL. |
api_spec.md |
Documents the configuration option. |
ulabel-wheel-zoom-focal-point.md |
Documents the focal-point defect. |
CHANGELOG.md |
Records version 0.26.3 changes. |
src/version.js |
Bumps the runtime version. |
package.json |
Bumps the package version. |
package-lock.json |
Synchronizes the lockfile version. |
Suppressed comments (1)
src/index.js:6796
- The drag path has the same transformed-coordinate mismatch as wheel zoom: subtracting
rect.left/topremoves an offset but does not convert viewport distances back into annbox layout pixels under CSS scaling. Use the same inverse coordinate conversion here so shift-drag remains anchored in transformed hosts.
const annbox_rect = document.getElementById(this.config["annbox_id"]).getBoundingClientRect();
this.rezoom(
this.drag_state["zoom"]["mouse_start"][0] - annbox_rect.left,
this.drag_state["zoom"]["mouse_start"][1] - annbox_rect.top,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/index.js:6662
viewport_to_annbox_localderives scale fromrect.width / annbox.clientWidth, butclientWidth/clientHeightexclude scrollbars. Since the annbox usesoverflow: scroll, this will inflate the computed scale when scrollbars are present and can reintroduce focal-point drift even with no CSS transform. UseoffsetWidth/offsetHeight(layout border-box size, including scrollbars) for the scale denominator so the ratio reflects only CSS transforms.
const scale_x = annbox.clientWidth > 0 ? rect.width / annbox.clientWidth : 1;
const scale_y = annbox.clientHeight > 0 ? rect.height / annbox.clientHeight : 1;
tests/e2e/wheel-zoom-focal-point.spec.js:95
pre_zoom_until_overflowscan fall out of its retry loop without ever overflowing, silently letting the test proceed with a state where the focal-point invariant cannot hold (scroll positions clamp at 0). Make this helper fail loudly if it cannot achieve overflow so failures are diagnosable rather than flaky downstream assertion failures.
if (overflows) return;
await page.mouse.wheel(0, -300);
await page.waitForTimeout(30);
}
}
tests/e2e/min-zoom-fit-ratio.spec.js:19
- The test computes
fit_zoomfromannbox.clientWidth/clientHeight, but the implementation floors zoom usingget_viewport_height_ratio/get_viewport_width_ratio(jQuery.height()/.width()). Withoverflow: scroll, scrollbars can makeclientWidth/clientHeightsmaller than what the code uses, leading to platform-dependent flakiness. Computefit_zoomthe same way the library does so the assertions match behavior.
const fit_zoom = Math.min(
annbox.clientHeight / ul.config.image_height,
annbox.clientWidth / ul.config.image_width,
);
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
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.
fix/zoom
Description
handle_wheelanddrag_rezoomused to pass rawclientX/clientY(viewport coords) straight intorezoom, which treats its focal-point arguments as annbox-local. When the container sat at viewport(0, 0)the two frames coincided and the bug was invisible; anywhere else (e.g. hosted inside a centered dialog, a padded panel, or below a header) zoom would snap by roughly the annbox's screen offset. Both call sites now convert to annbox-local viagetBoundingClientRect()before callingrezoom, so zoom stays anchored to the cursor / mousedown point regardless of how the host embeds ULabel.min_zoom_fit_ratio. Sets a zoom-out floor as a multiplier of the "whole image just fits the viewport" zoom.0(default) preserves the existing behavior (no floor).1.0prevents users from zooming out past the fit-to-viewport level.>1forces the image to always overflow. Zoom-in is unaffected. The floor recomputes from live annbox dimensions, so it adapts to browser resize.PR Checklist
package.jsonhas been bumped since last releasepackage.jsonandsrc/version.jsapi_spec.md)changelog.mdBreaking API Changes
No