lib-classifier: refactor single image viewer with VisXZoom #6390

mcbouslog · 2024-10-16T15:05:16Z

Package

lib-classifier

Linked Issue and/or Talk Post

closes Add mouse wheel zoom to image subject viewers #5109

Describe your changes

Notes

see existing VisXZoom notes - https://github.com/zooniverse/front-end-monorepo/tree/master/packages/lib-classifier/src/components/Classifier/components/SubjectViewer#pan-and-zoom
ZoomingScatterPlot uses VisXZoom: https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/ScatterPlotViewer/components/ZoomingScatterPlot/ZoomingScatterPlot.js#L206-L224

How to Review

Helpful explanations that will make your reviewer happy:

What Zooniverse project should my reviewer use to review UX?
- test project with single and multi image subjects, drawing task:
  - staging/lib-classifier: https://local.zooniverse.org:8080/?project=1693&workflow=3838
  - staging/app-project: https://local.zooniverse.org:3000/projects/markb-panoptes/test-project-1-mb-fem-lab
What user actions should my reviewer step through to review this PR?
1. load subject with image(s)
2. pan, zoom, rotate, invert, and reset image
3. draw marks on the image, repeat step 2 with drawn marks
Which storybook stories should be reviewed?
- mainly http://localhost:6006/?path=/story/subject-viewers-singleimageviewer--default, but also DataImageViewer, FlipbookViewer, ImageAndTextViewer, MultiFrameViewer, and SeparateFrames stories
- SeparateFrames stories onKeyDown (i.e. + to zoom in) doesn't work, but it does in the linked test project, not sure why
Are there plans for follow up PR’s to further fix this bug or develop this feature?

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

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

New Feature

The PR creator has listed user actions to use when testing the new feature
Unit tests are included for the new feature
A storybook story has been created or updated
- Example of SlideTutorial component with docgen comments, and its story

Refactoring

The PR creator has described the reason for refactoring
The refactored component(s) continue to work as expected

coveralls · 2024-10-16T15:13:03Z

coverage: 75.742% (+0.1%) from 75.621%
when pulling 0193a44 on lib-classifier_mouse-scroll-zoom
into e68e2c6 on master.

…onents

… to SingleImageViewerContainer

mcbouslog · 2024-11-14T20:45:02Z

...onents/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageCanvas.js

+
+import SVGImage from '../SVGComponents/SVGImage'
+
+function calculateAdjustedViewBox({ naturalWidth, naturalHeight, transformMatrix }) {


I think this calculation needs additional work, specifically to handle zoomed out panning. When zooming out, then panning, the image jumps around erratically.

mcbouslog · 2024-11-15T15:32:12Z

...onents/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewer.js

+          <VisXZoom
+            height={naturalHeight}
+            panning={panning}
+            setOnPan={setOnPan}
+            setOnZoom={setOnZoom}
+            width={naturalWidth}
+            zoomConfiguration={DEFAULT_ZOOM_CONFIG}
+            zoomingComponent={SingleImageCanvas}
+            zooming={zooming}
+            {...singleImageCanvasProps}
+          />


See https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/ScatterPlotViewer/components/ZoomingScatterPlot/ZoomingScatterPlot.js#L206-L224 for existing implementation of VisXZoom. constrain could potentially be added to the SingleImageViewer.

goplayoutside3

I started reading through each viewer and testing their UX. Because this will be an in depth review and lots of moving parts, I'm pausing with a couple of questions and will resume once they're resolved.

packages/lib-classifier/src/components/Classifier/components/SubjectViewer/README.md

...c/components/Classifier/components/SubjectViewer/components/FlipbookViewer/FlipbookViewer.js

… load

eatyourgreens · 2024-11-22T09:11:18Z

When Sarah converted the single image viewer to VisXZoom (back in 2020, I think), it broke all the drawing tools. This PR breaks all the drawing tools too, in (I think) the same way. Should I add videos/issue descriptions, or is this still a work in progress?

To see this, try drawing while zoomed in on this branch, particularly with any tool that uses dragging motions/gestures.

Here's a screen recording with the rectangle tool, dragging the resize handles while zoomed in. You can also reproduce this with the cat face ellipse tool in I Fancy Cats.

Screen.Recording.2024-11-22.at.14.33.49.mov

I think one big barrier to a major refactor was a lack of tests, in turn due to lack of SVG support in jsdom. It's really easy to break the drawing tools without being alerted by failing CI tests.

eatyourgreens · 2024-11-22T10:13:25Z

There's an undocumented feature, used by Seabird Watch and Penguin Watch, which you should add to your manual drawing tool tests.

When you're making a large number of marks, you don't want to be constantly switching between zoom mode and drawing mode. For this reason, you can control pan-and-zoom with the keyboard, from a focused drawing mark.

To see this in action, try placing marks, while zooming and panning with the keyboard, at either of the following links:

You should be able to mark a penguin, then zoom and pan from the keyboard while continuing to mark penguins on the zoomed picture (on crowded pictures, you really need to zoom in, so this feature isn't optional.)

Screen.Recording.2024-11-22.at.10.15.02.mov

This PR breaks that keyboard accessibility for drawn marks.

eatyourgreens · 2024-11-22T11:09:35Z

One last comment: on this branch, drawn marks scale up with the image when you zoom in. Drawn marks should ignore the scale transformation and stay the same size at all zoom levels (see #6066 for one attempt to fix this for stroke widths. You can also style SVG shapes so that they are independent of scale.)

mcbouslog · 2024-11-22T18:19:24Z

Thank you @eatyourgreens , the comments and videos are very helpful! To summarize I think you've identified 3 issues:

marks with drag and move handles aren't scaled and working as expected
keyboard pan and zoom should be enabled in annotation mode
marks aren't properly scaled zoomed in or out

I'll report back on progress.

eatyourgreens · 2024-11-22T18:25:14Z

Thank you @eatyourgreens , the comments and videos are very helpful! To summarize I think you've identified 3 issues:

marks with drag and move handles aren't scaled and working as expected

You're welcome. For that first one, I can also reproduce it with the point tool, without drag handles. Drop a point on the image somewhere, then drag and move it while zoomed in. The pointer movement, within the subject viewer, isn't properly mapped to the image. It looks like the pointer coordinates are still scaled to the subject viewer dimensions, not the new, larger image dimensions. Dragging only works when the subject viewer and image are the same size. Could be a missing transformation somewhere on the <rect> element that acts as the drawing canvas. Maybe it isn't resizing to match the image size?

mcbouslog · 2024-11-22T23:02:54Z

I can recreate the noted 2 and 3 issues, but I'm having trouble recreating 1 (marks with drag and move handles aren't scaled and working as expected)? I'm using Chrome v131, but I don't think it'd be browser specific.

ZoomInAndDrawRectangle.mp4

eatyourgreens · 2024-11-23T03:50:03Z

Sorry, I forgot to say that my video was Firefox 132. SVG implementations do vary between browsers, unfortunately.

EDIT: I think the drawing tools are only using bits of the SVG 2 spec that are well-supported across the major browser engines.

eatyourgreens · 2024-11-23T13:15:39Z

I just tried a few different browsers. Scaling and dragging seems to be ok in Chrome or Safari, but broken in Firefox.

eatyourgreens · 2024-11-23T17:12:37Z

This might be relevant. Dragging interactions get the current transformation matrix from a svg element stored in SVGContext here:

front-end-monorepo/packages/lib-classifier/src/plugins/drawingTools/components/draggable/draggable.js

Line 51 in 1862ade

const { canvas } = useContext(SVGContext)

Creating a mark here gets the CTM from the DrawingCanvas component in the InteractionLayer component, which is an SVG rect that handles all the pointer events that create marks.

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js

Line 88 in 1862ade

? svgPoint.matrixTransform(canvas.current?.getScreenCTM().inverse())

These refs don’t point to the same element, so that could account for differences in behaviour between creating a mark and dragging a mark. They probably should be using the same element and transformation matrix. I don't think I've noticed this before. At least, I can't find an issue for it, and I'm sure I would have opened an issue for something like this.

eatyourgreens · 2024-11-23T17:15:42Z

Update: dragging a zoomed-in mark is broken in Firefox on the production release. I'll open an issue. It's not a bug that's been introduced on this branch. The reason is that the draggable decorator is getting the screen CTM from the wrong DOM node, so not transforming pointer coordinates correctly.

Drawing is broken for zoomed-in images in Firefox #6490

Update to the update:

very quick fix for that broken ref, and some code cleanup, in fix(lib-classifier): canvas context for SVG drawing tools #6491.

...onents/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageCanvas.js

...lassifier/components/SubjectViewer/components/SingleImageViewer/components/PlaceholderSVG.js

…th useKeyZoom

mcbouslog · 2025-01-29T21:18:41Z

A brief update on a few issues previously noted:

classification.metadata.subject_dimensions broken, reversion of previous bug - I think fixed per c58b452 and 7eb30ef and 0a2d54f
marks with drag and move handles aren't scaled and working as expected - fixed with fix(lib-classifier): canvas context for SVG drawing tools #6491
keyboard pan and zoom should be enabled in annotation mode - fixed with 0193a44
marks aren't properly scaled zoomed in or out - working on
...others to come from additional testing... - working on

mcbouslog added 4 commits October 15, 2024 12:11

Add initial files for single image viewer with visx zoom

8a2889c

Add rotation to single image viewer

b274f8b

Initial refactor single image viewer with VisXZoom

58d39be

Refactor single image viewer with VisXZoom mouse drag and zoom

878edd9

mcbouslog added 25 commits October 24, 2024 14:49

Rename SingleImageViewer

09d2d92

Add PlaceholderSVG component

0d203b0

Fix drawing mark dragEnd

d598a4c

Refactor viewers that use SingleImageViewer

c93f219

Add resetView to MultiFrameViewer

39ad1a6

Edit SeparateFrame SingleImageCanvas subjectId

7b9da7a

Remove SVGPanZoom

3929816

Add PlaceholderSVG maxHeight

b0ef4c6

Refactor SeparateFrame with transform matrix

f6b8415

Refactor SeparateFrame with SingleImageContainer

f0d4086

Refactor ImageAndTextViewer properties

75a568a

Refactor MultiFrameViewer with SingleImageViewer

260d87b

Refactor FlipbookViewer and SeparateFrameViewer with SingleImage comp…

467973b

…onents

Revert FlipbookViewer handleFrameChange changes

066619a

Add default props to SingleImageCanvas

49d6ca3

Refactor FlipbookViewer with SingleImageContainer

ec4651c

Revert height and width edit in VisXZoom

0c27c95

Remove previous SingleImageViewer tests

c1a434e

Add PlaceholderSVG stories and tests

f2f2a56

Delete SingleImageConnector

8809d9f

Refactor ImageAndTextViewer and MultiFrameViewer to pass loadingState…

f4bf4b0

… to SingleImageViewerContainer

Refactor FlipbookViewer with SingleImageViewer and useSubjectImage

88c669e

Refactor SeparateFrame with SingleImageViewer and useSubjectImage

9c0a872

Remove title default prop

a52f9cd

Fix tests, cleanup

39f75e4

Update READMEs, remove SVGPanZoom references

e8a4c45

mcbouslog commented Nov 14, 2024

View reviewed changes

mcbouslog requested review from a team and goplayoutside3 November 14, 2024 20:51

mcbouslog commented Nov 15, 2024

View reviewed changes

goplayoutside3 reviewed Nov 18, 2024

View reviewed changes

packages/lib-classifier/src/components/Classifier/components/SubjectViewer/README.md Outdated Show resolved Hide resolved

...c/components/Classifier/components/SubjectViewer/components/FlipbookViewer/FlipbookViewer.js Outdated Show resolved Hide resolved

Refactor FlipbookViewer onReady exclusively per initial default frame…

c58b452

… load

eatyourgreens reviewed Nov 23, 2024

View reviewed changes

...onents/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageCanvas.js Show resolved Hide resolved

eatyourgreens reviewed Nov 23, 2024

View reviewed changes

...lassifier/components/SubjectViewer/components/SingleImageViewer/components/PlaceholderSVG.js Outdated Show resolved Hide resolved

eatyourgreens mentioned this pull request Dec 6, 2024

fix(lib-classifier): canvas context for SVG drawing tools #6491

Merged

12 tasks

mcbouslog and others added 9 commits January 13, 2025 14:59

Merge branch 'master' into lib-classifier_mouse-scroll-zoom

e19ef2a

Merge branch 'master' into lib-classifier_mouse-scroll-zoom

716581f

Merge branch 'master' into lib-classifier_mouse-scroll-zoom

cecd104

Remove PlaceholderSVG xmlns, refactor maxHeight and maxWidth properties

e6d5b39

Refactor PlaceholderSVG with subjectImage ref

7eb30ef

Fix text subject dimension metadata duplicates

0a2d54f

Refactor ZoomEventLayer styling properties

462b04b

Remove TODO from Subject Viewers README

7dc2905

Refactor useKeyZoom with customKeyMappings, refactor image viewers wi…

0193a44

…th useKeyZoom

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib-classifier: refactor single image viewer with VisXZoom #6390

lib-classifier: refactor single image viewer with VisXZoom #6390

mcbouslog commented Oct 16, 2024 •

edited

Loading

coveralls commented Oct 16, 2024 •

edited

Loading

mcbouslog Nov 14, 2024 •

edited

Loading

mcbouslog Nov 15, 2024 •

edited

Loading

goplayoutside3 left a comment

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

mcbouslog commented Nov 22, 2024

eatyourgreens commented Nov 22, 2024 •

edited

Loading

mcbouslog commented Nov 22, 2024

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

mcbouslog commented Jan 29, 2025 •

edited

Loading


		import SVGImage from '../SVGComponents/SVGImage'

		function calculateAdjustedViewBox({ naturalWidth, naturalHeight, transformMatrix }) {

lib-classifier: refactor single image viewer with VisXZoom #6390

Are you sure you want to change the base?

lib-classifier: refactor single image viewer with VisXZoom #6390

Conversation

mcbouslog commented Oct 16, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

Describe your changes

Notes

How to Review

Checklist

General

General UX

New Feature

Refactoring

coveralls commented Oct 16, 2024 • edited Loading

mcbouslog Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

mcbouslog Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

goplayoutside3 left a comment

Choose a reason for hiding this comment

eatyourgreens commented Nov 22, 2024 • edited Loading

eatyourgreens commented Nov 22, 2024 • edited Loading

eatyourgreens commented Nov 22, 2024 • edited Loading

mcbouslog commented Nov 22, 2024

eatyourgreens commented Nov 22, 2024 • edited Loading

mcbouslog commented Nov 22, 2024

eatyourgreens commented Nov 23, 2024 • edited Loading

eatyourgreens commented Nov 23, 2024 • edited Loading

eatyourgreens commented Nov 23, 2024 • edited Loading

eatyourgreens commented Nov 23, 2024 • edited Loading

mcbouslog commented Jan 29, 2025 • edited Loading

mcbouslog commented Oct 16, 2024 •

edited

Loading

coveralls commented Oct 16, 2024 •

edited

Loading

mcbouslog Nov 14, 2024 •

edited

Loading

mcbouslog Nov 15, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 22, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

mcbouslog commented Jan 29, 2025 •

edited

Loading