fix(classifier): Polygon tool drag handles #6550

eatyourgreens · 2024-12-12T16:06:12Z

Refactor the polygon tool drag handles so that pointerdown does not close and finish the shape, overriding the pointer dragging behaviour. Instead, the shape is closed and finished on pointerup.

Refactor the draggable decorator so that it decorates any existing onPointerDown, onPointerMove, and onPointerUp handlers with draggable behaviour. The draggable behaviour is run before the existing handler, for each event.

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

Package

lib-classifier

Linked Issue and/or Talk Post

closes Drag handles are broken for polygon drawing tool #6549.
original bug introduced by Classifier: refactor polygon tool #3389, I think.

How to Review

Screen.Recording.2024-12-12.at.16.06.48.mov

Clicking a drag handle should still close and finish a polygon, but pointerdown should now start dragging where previously it was cancelled.

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

coveralls · 2024-12-12T16:13:31Z

coverage: 77.363% (-0.02%) from 77.385%
when pulling 61e6b16 on eatyourgreens:fix-polygon-drag-handles
into 2150c33 on zooniverse:master.

eatyourgreens · 2024-12-12T16:14:01Z

packages/lib-classifier/src/plugins/drawingTools/components/Polygon/Polygon.js

@@ -72,7 +72,6 @@ function Polygon({ active, mark, scale, onFinish }) {
          strokeWidth={strokeWidth}
          strokeDasharray={strokeDasharray}
        />
-      )}


The polygon tool code is actually broken here, and has been for a while. 😳

goplayoutside3

While I can move the drag point by running this PR with https://localhost.zooniverse.org:3000/projects/eatyourgreens/-project-testing-ground/classify/workflow/3370?env=staging, when I release the drag point then move my mouse around the subject viewer, hovering sometimes picks up the drag point again. Do you see that behavior too? I'm using Brave Browser on MacOS.

eatyourgreens · 2024-12-16T07:41:29Z

I haven’t seen that, but it sounds like a bug where pointer capture isn’t automatically released on pointer up.

@goplayoutside3 do you see that same bug on other shapes that use drag handles; lines, rectangles, or ellipses? Are you using a mouse or a trackpad? I'm using a trackpad, which might be the difference.

EDIT: I've just been able to reproduce it in the storybook in Firefox. I'll add a comment to the code diff.

eatyourgreens · 2024-12-16T10:59:28Z

packages/lib-classifier/src/plugins/drawingTools/components/Polygon/Polygon.js

@@ -84,7 +83,7 @@ function Polygon({ active, mark, scale, onFinish }) {
              y={point.y}
              fill='currentColor'
              dragMove={point.moveTo}
-              onPointerDown={handleClosePolygon}
+              onPointerUp={handleClosePolygon}


Previously this overrode onPointerDown for draggable elements, which stopped dragging from happening at all. Now it overrides onPointerUp which means dragging is never canceled once it has started. 😳

Fixed in 4059705, but that partly reverts #3536.

b3b8304 might be a better fix. It decorates existing event handlers with draggable behaviour, rather than replacing or overriding them.

Tested in Chrome and Firefox with a few different drawing tools. Seems to work OK, and dragging is now cancelled on pointer up, while also running the mark's pointerup handler.
https://localhost:8080/?project=eatyourgreens%2F-project-testing-ground&workflow=3370

Drawing while zoomed-in still triggers #6490 in Firefox, but that's a separate issue.

The ellipse tool still seems to work as expected in I Fancy Cats.

I've updated the PR description too, as the scope has changed from the polygon tool to the draggable decorator.

Refactor the polygon tool drag handles so that `pointerdown` does not close and finish the shape, overriding the pointer dragging behaviour. Instead, the shape is closed and finished on `pointerup`.

eatyourgreens commented Dec 12, 2024

View reviewed changes

goplayoutside3 self-assigned this Dec 16, 2024

goplayoutside3 added the bug Something isn't working label Dec 16, 2024

goplayoutside3 reviewed Dec 16, 2024

View reviewed changes

eatyourgreens force-pushed the fix-polygon-drag-handles branch from f7435ad to c791810 Compare December 16, 2024 10:49

eatyourgreens commented Dec 16, 2024

View reviewed changes

eatyourgreens force-pushed the fix-polygon-drag-handles branch 3 times, most recently from 09e19d6 to 9f60321 Compare December 16, 2024 21:15

eatyourgreens mentioned this pull request Dec 16, 2024

Drag handles are broken for polygon drawing tool #6549

Open

eatyourgreens added 3 commits December 19, 2024 09:47

fix(classifier): Polygon tool drag handles

12ff562

Refactor the polygon tool drag handles so that `pointerdown` does not close and finish the shape, overriding the pointer dragging behaviour. Instead, the shape is closed and finished on `pointerup`.

allow onFinish events to bubble up the DOM

07acf97

decorate draggable event handlers

61e6b16

eatyourgreens force-pushed the fix-polygon-drag-handles branch from 9f60321 to 61e6b16 Compare December 19, 2024 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(classifier): Polygon tool drag handles #6550

fix(classifier): Polygon tool drag handles #6550

eatyourgreens commented Dec 12, 2024 •

edited

Loading

coveralls commented Dec 12, 2024 •

edited

Loading

eatyourgreens Dec 12, 2024

goplayoutside3 left a comment

eatyourgreens commented Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024

eatyourgreens Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024

eatyourgreens Dec 16, 2024

fix(classifier): Polygon tool drag handles #6550

Are you sure you want to change the base?

fix(classifier): Polygon tool drag handles #6550

Conversation

eatyourgreens commented Dec 12, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

How to Review

Checklist

General

General UX

Bug Fix

coveralls commented Dec 12, 2024 • edited Loading

eatyourgreens Dec 12, 2024

Choose a reason for hiding this comment

goplayoutside3 left a comment

Choose a reason for hiding this comment

eatyourgreens commented Dec 16, 2024 • edited Loading

eatyourgreens Dec 16, 2024

Choose a reason for hiding this comment

eatyourgreens Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Dec 16, 2024

Choose a reason for hiding this comment

eatyourgreens Dec 16, 2024

Choose a reason for hiding this comment

eatyourgreens commented Dec 12, 2024 •

edited

Loading

coveralls commented Dec 12, 2024 •

edited

Loading

eatyourgreens commented Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024 •

edited

Loading

eatyourgreens Dec 16, 2024 •

edited

Loading