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

eatyourgreens · 2024-11-23T18:22:20Z

Make sure that SVGContext.canvas is a reference to the SVG rect element that's used as a drawing canvas, so that creating new drawing marks and editing existing marks both use the same screen CTM when calculating the pointer position in SVG space.

This fixes a bug in Firefox where the draggable decorator doesn't correctly track the active pointer, in the context of a zoomed SVG image.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

lib-classifier

Linked Issue and/or Talk Post

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

How to Review

On I Fancy Cats, you should be able to draw while zoomed in, without seeing any of the mismatched positioning described in #6490. You can try it out by drawing an ellipse while zoomed in, or by placing a point then dragging the point to a new position.

Try it out in Firefox and Chrome, while zoomed in and also after rotating the image. Pointer interactions should be correctly mapped to the SVG image now, regardless of any external transformation.

While drawing, in any browser, you should be able to pan and zoom with the keyboard too.

The stroke width of drawn marks should also stay the same as you zoom in. Lines shouldn’t be thicker at increased magnification.

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

Bug Fix

The PR creator has listed user actions to use when testing if bug is fixed
The bug is fixed
Unit tests are added or updated

...mponents/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js

eatyourgreens · 2024-11-23T18:27:24Z

...mponents/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js

+  const svgContext = useContext(SVGContext)
+  const canvasRef = useRef()
+  svgContext.canvas = canvasRef.current


I'm not 100% sure if this is the best way to update a reference within a context object, but it works in Chrome, Safari, and Firefox.

SVGContext.canvas is a reference to the element that the draggable decorator (and drawing tools in general) will use to calculate the screen CTM for pointer interactions (ie. to get SVG coordinates from pointer coordinates.)

...mponents/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js

coveralls · 2024-11-23T18:29:57Z

coverage: 77.364% (+0.01%) from 77.354%
when pulling 5219203 on eatyourgreens:fix-svg-canvas-context
into 3e8f2ee on zooniverse:master.

eatyourgreens · 2024-11-23T18:30:00Z

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

@@ -68,7 +66,6 @@ function SingleImageViewer({
            transform={transform}
          >
            <SVGImageCanvas
-              ref={canvasLayer}


This ref was originally used for dragging interactions, which caused the Firefox bug.

mcbouslog · 2024-12-05T17:38:43Z

I think this has broken dragging/pan with the mouse (across browsers). In pan/zoom mode, when clicking then dragging the subject image, I get the following console error:

Uncaught TypeError: Cannot read properties of undefined (reading 'getScreenCTM')
    at getEventOffset (draggable.js:38:83)

However, if I remove the changes to SingleImageViewer I think mouse dragging/panning works and Firefox zoomed in drawn marks dragging works as expected. I need to wrap my head around why, but posting an update.

eatyourgreens · 2024-12-05T17:59:24Z

Oh, I think that might be because the draggable image is dragged in a different context to draggable drawing marks. Marks should get the CTM from the rect that defines the drawing canvas, but the image is rendered separately.

The fix here was for the drawn marks, so it probably would break the draggable image.

eatyourgreens · 2024-12-06T12:36:55Z

However, if I remove the changes to SingleImageViewer I think mouse dragging/panning works and Firefox zoomed in drawn marks dragging works as expected.

I tried this quickly, but it breaks the drawing tools. Dragging the image also doesn't work as expected with the CTM that's used by the drawing tools. I'm not sure why the image and drawing tools are using different CTMs.

I find that clicking and dragging a zoomed-in image, after drawing some marks, causes the image to jump around unexpectedly. I think you mentioned this bug, or a similar bug, on #6390. The transformation matrix for clicks on the image is generating the wrong x,y coordinates in SVG space.

eatyourgreens · 2024-12-06T12:41:47Z

I'll push my changes up, since they're easy to revert and this branch is already broken anyway.

eatyourgreens · 2024-12-06T20:57:50Z

Since pushing my latest commit, I'm not seeing those problems any more in Firefox, so maybe I've fixed the problem? It's still bothering me a bit that the image and drawn marks are using different transformation matrixes. It seems like both should have the same transformations applied to them.

eatyourgreens · 2024-12-11T23:26:03Z

Thinking about this some more, currentTarget might be a more robust way to get the DOM node that is currently handling pointer events. It would simplify the interfaces for draggable behaviour too.

EDIT: I tried this. It breaks the coordinate transformations for drag handles, which are rendered relative to the parent mark.

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

Line 28 in 3e8f2ee

const transform = `translate(${x}, ${y}) scale(${1 / scale})`

Make sure that `SVGContext.canvas` is a reference to the SVG `rect` element that's used as a drawing canvas, so that creating new drawing marks and editing existing marks both use the same screen CTM when calculating the pointer position. This fixes a bug in Firefox where the `draggable` decorator doesn't correctly track the active pointer, in the context of a zoomed SVG image.

eatyourgreens commented Nov 23, 2024

View reviewed changes

...mponents/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js Outdated Show resolved Hide resolved

eatyourgreens commented Nov 23, 2024

View reviewed changes

...mponents/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js Outdated Show resolved Hide resolved

eatyourgreens commented Nov 23, 2024

View reviewed changes

eatyourgreens force-pushed the fix-svg-canvas-context branch from 6565c8d to e958109 Compare November 23, 2024 18:31

eatyourgreens mentioned this pull request Nov 23, 2024

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

Open

27 tasks

mcbouslog self-requested a review November 25, 2024 16:07

mcbouslog self-assigned this Nov 26, 2024

eatyourgreens force-pushed the fix-svg-canvas-context branch 2 times, most recently from 22f3fae to 79fea74 Compare December 3, 2024 00:31

eatyourgreens force-pushed the fix-svg-canvas-context branch 3 times, most recently from 0e7a945 to 960252c Compare December 6, 2024 20:54

eatyourgreens added 3 commits December 12, 2024 07:44

DRY up SVG event-handling functions

6eacb2a

revert changes to SingleImageViewer

5219203

eatyourgreens force-pushed the fix-svg-canvas-context branch from 960252c to 5219203 Compare December 12, 2024 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens Nov 23, 2024 •

edited

Loading

coveralls commented Nov 23, 2024 •

edited

Loading

eatyourgreens Nov 23, 2024

mcbouslog commented Dec 5, 2024 •

edited

Loading

eatyourgreens commented Dec 5, 2024 •

edited

Loading

eatyourgreens commented Dec 6, 2024 •

edited

Loading

eatyourgreens commented Dec 6, 2024

eatyourgreens commented Dec 6, 2024

eatyourgreens commented Dec 11, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

eatyourgreens commented Nov 23, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

How to Review

Checklist

General

General UX

Bug Fix

eatyourgreens Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 23, 2024 • edited Loading

eatyourgreens Nov 23, 2024

Choose a reason for hiding this comment

mcbouslog commented Dec 5, 2024 • edited Loading

eatyourgreens commented Dec 5, 2024 • edited Loading

eatyourgreens commented Dec 6, 2024 • edited Loading

eatyourgreens commented Dec 6, 2024

eatyourgreens commented Dec 6, 2024

eatyourgreens commented Dec 11, 2024 • edited Loading

eatyourgreens commented Nov 23, 2024 •

edited

Loading

eatyourgreens Nov 23, 2024 •

edited

Loading

coveralls commented Nov 23, 2024 •

edited

Loading

mcbouslog commented Dec 5, 2024 •

edited

Loading

eatyourgreens commented Dec 5, 2024 •

edited

Loading

eatyourgreens commented Dec 6, 2024 •

edited

Loading

eatyourgreens commented Dec 11, 2024 •

edited

Loading