fix(classifier): smooth image dragging for pan/zoom #6065
Conversation
This file contains 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
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
from
941e8c0
to
da4a793
Compare
|
NB. I'm testing this (and recording the screen video) on a very-much overpowered M3 MacBook Pro. It'd be useful to get tests from machines that are more in line with what typical volunteers use. I might be seeing smooth dragging movement simply because I'm using a fast machine. |
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
3 times, most recently
from
1aef573
to
7614043
Compare
eatyourgreens
changed the title
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
from
7614043
to
e77a536
Compare
eatyourgreens
changed the title
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
2 times, most recently
from
0fdf304
to
7c42250
Compare
eatyourgreens
commented
Apr 30, 2024
...onents/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewer.js
Outdated
Show resolved
Hide resolved
12 tasks
eatyourgreens
commented
Apr 30, 2024
Comment on lines
68
to
82
| <svg | ||
| ref={canvasLayer} | ||
| viewBox={viewBox} | ||
| > | ||
| {children} | ||
| {enableInteractionLayer && ( | ||
| <InteractionLayer | ||
| frame={frame} | ||
| height={height} | ||
| scale={scale} | ||
| subject={subject} | ||
| width={width} | ||
| /> | ||
| )} | ||
| </svg> |
6 tasks
|
Noting that the image is cropped if you try to drag it outside a rotated view box, even though there’s visibly available space in the browser viewport. I’m not sure if that would be a blocker. To reproduce that, rotate an image by 90° or 270°, then drag it left or right. The width of the rotated view box (the actual height of the image) is narrower than the width of the subject viewer. EDIT: fixed by styling the nested |
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
2 times, most recently
from
d014044
to
1aed5f9
Compare
|
Here's a screen recording from Chrome 124, also running on an M3 MacBook. Screen.Recording.2024-05-02.at.17.24.35.mov |
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
5 times, most recently
from
3d70251
to
3febd39
Compare
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
from
3febd39
to
025b97b
Compare
eatyourgreens
changed the title
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
from
025b97b
to
03a0883
Compare
- Refactor `SVGPanZoom` to use a ref for `zoom`, rather than component state. Hopefully, this will lead to fewer rerenders when dragging a zoomed image, and smoother performance. - Use a ref to store pointer coordinates in the `draggable` decorator, to avoid unnecessary rerenders. - Move the cropped SVG viewbox into a nested SVG, for better performance when dragging a zoomed image. - Remove the `1.5` easing factor on dragged images, so that they 'stick' to the pointer when dragged. - Use `focus-visible` so that focus rings are only shown for keyboard interactions.
- remove unused variables. - use a ref to remember the current pointerId across renders. - clean up pointer capture.
eatyourgreens
force-pushed
the
refactor-pan-zoom
branch
from
03a0883
to
665d5b0
Compare
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.
SVGPanZoomto use a ref forzoom, rather than component state. Hopefully, this will lead to fewer rerenders when dragging a zoomed image, and smoother performance.draggabledecorator, to avoid unnecessary rerenders.1.5easing factor on dragged images, so that they 'stick' to the pointer when dragged.focus-visibleso that focus rings are only shown for keyboard interactions.useKeyZoomhook to support rotated images.Screen recordings from Firefox 125 running on an M3 MacBook (Sonoma 14.4.1.)
Pan and zoom with a trackpad:
Screen.Recording.2024-04-30.at.09.35.20.mov
Pan and zoom with a trackpad at a very high zoom level:
Screen.Recording.2024-04-30.at.10.18.27.mov
Keyboard focus with Tab and shift-Tab:
Screen.Recording.2024-04-30.at.09.43.05.mov
Package
Linked Issue and/or Talk Post
How to Review
Zoom and pan an image in the dev classifier, using both the keyboard and a trackpad/mouse to pan the zoomed image.
You can test the focus styling by tabbing through the page, to check that drawn marks have focus styling when focused (WCAG Success Criterion 2.4.7: focus visible.) When a mark has keyboard focus, Enter will open any associated subtask popup, which should also be fully keyboard accessible. Backspace will delete the focused mark (WCAG Success Criterion 2.1.1: Keyboard.)
Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrapordocker-compose up --buildand FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Bug Fix