Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(drawing-tools): ignore pointers in 'pan and zoom' mode #6749

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Mar 9, 2025

When 'Pan' is selected in the image toolbar:

  • deactivate the active mark, so that it doesn't capture the pointer.
  • style all inactive drawn marks with pointer-events: none;.

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

Package

  • lib-classifier

Linked Issue and/or Talk Post

How to Review

https://localhost:8080/?env=staging&project=markb-panoptes/test-project-1-mb-fem-lab&workflow=3847

Draw a large ellipse, or rectangle, then switch to Pan mode. Pan and zoom the image with a trackpad or touchscreen. Switch back to drawing mode to continue editing the active mark.

Screen.Recording.2025-03-09.at.09.15.14.mov
Screen.Recording.2025-03-09.at.09.08.42.mov

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

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

When 'Pan' is selected in the image toolbar:
- deactivate the active mark, so that it doesn't capture the pointer.
- style all drawn marks with `pointer-events: none;`.
@coveralls
Copy link

Coverage Status

coverage: 75.435%. remained the same
when pulling 3020ce2 on eatyourgreens:drawing-pointer-events
into a43d853 on zooniverse:master.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Mar 9, 2025

I ignored transcribed lines in this PR, so previously transcribed lines will continue to capture the pointer, even when 'Pan' is selected in the toolbar. Pan and zoom for transcription subjects might be fiddly for subjects with very crowded lines.

@eatyourgreens eatyourgreens changed the title fix(drawing-tools): ignore the pointer in 'pan and zoom' mode fix(drawing-tools): ignore pointers in 'pan and zoom' mode Mar 9, 2025
@eatyourgreens
Copy link
Contributor Author

Updated the PR title because there might be multiple pointers firing pointer events eg. two finger scrolling to zoom in and out on a trackpad. All active pointers should be ignored by the changes here.

@goplayoutside3
Copy link
Contributor

@eatyourgreens thank you for looking into the mouse-wheel zoom behavior 'inside' marks like rectangles, etc. I think handling pointer events to allow mouse-wheel zoom is a good change.

However, I don't want to include the additional change to deactivate drawn marks in pan/move mode in this PR because the ability to zoom in on an image, pan it in pan mode, and then adjust the position of drawn mark without having to switch back to annotate mode has existed on production for a long time. Example: Elephant ID EY workflow. While deactivating drawn marks in pan mode might be the eventual UX decision, I want to consult with Sean in a discussion first. Okay to separate the two behaviors?

@eatyourgreens
Copy link
Contributor Author

Okay to separate the two behaviors?

I'm not sure how you would separate them. Did you have a particular idea in mind?

@goplayoutside3
Copy link
Contributor

Enable mouse-wheel zoom while hovering 'inside' a drawn mark and drawn marks should remain interactive in pan mode.

@eatyourgreens
Copy link
Contributor Author

I understand that, but how to do it? Here, marks are styled with either pointer-events: none; or pointer-events: painted;. There isn't a CSS style that will discriminate between pointer events for two-finger scrolling and single-finger dragging.

@goplayoutside3
Copy link
Contributor

Ah I see, thanks for the clarification! I think the inability to isolate mouse-wheel zoom on hover over a mark, yet keep the current UX of interactive marks in pan mode is what led to the conclusion mentioned in #6746:

the conclusion of that conversation was the component architecture of InteractionLayer (DrawingToolMarks, PreviousMarks, etc) makes detecting mouse-wheel actions difficult. We decided to go ahead with the launch of mouse-wheel zoom despite this quirk.

I'll tag Mark in for review of this PR with the recommendation we consult with Sean about changes to mark interactivity in pan mode vs annotate mode.

@eatyourgreens
Copy link
Contributor Author

Right, I see now. Sorry, I thought that this was a case of having the drawing canvas handle pointer events in drawing mode, and the subject image handle pointer events in pan-and-zoom mode.

PFE doesn't disable mark selection in pan mode either. If I select 'pan and zoom', then click on a rectangle, the rectangle is selected and the mode automatically switches back to drawing. I should have checked this first.

Screen.Recording.2025-03-10.at.15.14.05.mov

Scrolling to zoom works anywhere in the subject viewer in PFE, but that's because PFE dynamically adds a wheel listener to the root of the viewer, so it fires whenever pan-and-zoom is selected and you scroll the image up and down.

https://github.com/zooniverse/Panoptes-Front-End/blob/c8d9fba40e46d36ff90996675b0ea63ba1b1a0aa/app/components/pan-zoom.jsx#L33-L41

https://github.com/zooniverse/Panoptes-Front-End/blob/c8d9fba40e46d36ff90996675b0ea63ba1b1a0aa/app/components/pan-zoom.jsx#L52-L55

@eatyourgreens
Copy link
Contributor Author

Enable mouse-wheel zoom while hovering 'inside' a drawn mark and drawn marks should remain interactive in pan mode.

@goplayoutside3 @mcbouslog after a little bit of experimentation, what you need to do is something like wrap your zoomed components in an element that listens to wheel events:

      {_zoom => {
        zoomRef.current = _zoom
        return (
          <g
            height={height}
            onWheel={move ? _zoom.handleWheel : DEFAULT_HANDLER}
            width={width}
          >
            <ZoomingComponent
              initialTransformMatrix={_zoom.initialTransformMatrix}
              move={move}
              transformMatrix={_zoom.transformMatrix}
              transform={_zoom.toString()}
              {...props}
            >
              <ZoomEventLayer
                focusable
                height={height}
                onDoubleClick={onDoubleClick}
                onKeyDown={onKeyZoom}
                onPointerEnter={onPointerEnter}
                onPointerDown={panning ? _zoom.dragStart : DEFAULT_HANDLER}
                onPointerMove={panning ? _zoom.dragMove : DEFAULT_HANDLER}
                onPointerUp={panning ? _zoom.dragEnd : DEFAULT_HANDLER}
                onPointerLeave={onPointerLeave}
                panning={panning}
                tabIndex={0}
                width={width}
              />
            </ZoomingComponent>
          </g>
        )
      }}

Note that I've used the built-in wheel handler that's bundled with @visx/zoom: _zoom.handleWheel.

@mcbouslog
Copy link
Contributor

Looks great @eatyourgreens , thank you! I was just toying around with this locally.

@eatyourgreens
Copy link
Contributor Author

@mcbouslog thanks! I'm not sure if this is what's actually needed, since it turns off pointer events in the drawing layer, so that wheel events fire on the underlying ZoomEventLayer instead.

I did notice that the custom wheel handler isn't really doing anything that @visx/zoom already does, so I've opened #6756 to replace it.

@mcbouslog mcbouslog self-assigned this Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drawing: drawn marks intercept trackpad pan and zoom gestures
4 participants