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

Event propagation to canvas #1388

Closed
axelboc opened this issue Mar 16, 2023 · 3 comments · Fixed by #1473
Closed

Event propagation to canvas #1388

axelboc opened this issue Mar 16, 2023 · 3 comments · Fixed by #1473
Assignees

Comments

@axelboc
Copy link
Contributor

axelboc commented Mar 16, 2023

Is your feature request related to a problem?

Currently, useCanvasEvents registers its event handlers on the WebGL canvas. As a result, any element positioned on top of the canvas (via Html, SvgElement, FloatingControl, etc.) intercepts every event before it can reach the canvas element, thus preventing interaction (zoom, pan, etc.). To work around this and let events reach the canvas, we use pointer-events: none on every overlayaing element.

The problem arises with interactive overlaying elements - e.g. a resize handle on an SVG ROI; the ROI itself, to be able to move it around, etc. Currently, we use pointer-events: auto to re-enable intercepting events on specific interactive elements, but the downside is that it then re-intercepts every event! There's no way to fine-grain which events to propagate and which to stop -- it's all or nothing.

An example of why this is problematic: if we want a ROI to be draggable, then there's no way to let through wheel events so users can still zoom on the visualisation underneath.

Requested solution or feature

useCanvasEvents could register its event handlers on .r3fRoot instead of canvas in order to take advantage of event bubbling.

The upside of this solution is its simplicity — we don’t try to “catch” and re-fire events or anything like that, and we don’t need to set pointer-events: none on every overlaying element. The downside is that it's an opt-out-only solution: every event bubbles by default, so now we need to stop events that we don't want to let through to the canvas.

For instance, in this configuration, pressing down on the reset zoom button in the floating toolbar and then dragging the cursor away would pan the visualization. In this case, I'd see a few ways to "opt out" to prevent the pan:

  1. Put onPointerDown={evt => evt.stopPropagation()} on the reset zoom button.
    • Upsides: 1) fine-grained control; 2) very explicit, no hidden magic.
    • Downsides: 1) we have to do this for every event we want to stop (most interactions rely on pointerdown to start, but this may not be the case in the future or with custom interactions); 2) if users implement their own floating controls, they have to remember to stop events themselves; 3) users have to do this for every element they want to make interactive.
  2. Add a div wrapper inside FloatingControl and stop events there.
    • Upside: 1) every control stops the same events and users don’t have to do it themselves.
    • Downside: 1) less control over which events to stop; 2) hidden behaviour; 3) solution specific to floating controls.
  3. In useCanvasEvents's event handlers, detect if the target of the event is a button and stop processing the event if that’s the case.
    • Upside: 1) even though it’s a hidden behaviour, there’s a logic to it as buttons are interactive elements, so they make for a nice propagation barriers.
    • Downside: 1) what about wheel. right click, etc.? should those events be “let through”? 2) not all interactive elements will be buttons - e.g. dragging a ROI implemented as an SVG rect.
  4. In useCanvasEvents's event handlers, if the target of the event has a specific attribute, like data-stop-propagation="pointerdown wheel", don’t process any of the events listed. Conversely, if relevant, do process the events listed in data-allow-propagation, disregarding other rules like the one from the previous bullet point.
    • Upsides: 1) explicit behaviour; 2) full control.
    • Downsides: 1) user has to know/understand which events to stop/allow like in bullet point 1.

I think bullet point 4 is the most promising, as it's the most versatile solution. data-stop-propagation is generic-enough to cover all cases.

Bullet 3 could also be beneficial, but less so since most interactive elements other than floating controls are likely to be SVG elements — it also increases the complexity, requiring an additional data-allow-propagation for full control.

Alternatives you've considered

Catching events on overlaying elements and re-firing them on the canvas. Similar opt-in/opt-out challenges as above apply, though.

@loichuder
Copy link
Member

I agree that it is a tricky and painful problem but great sum up as always.

Solution 1. seems the most promising for me.

Solution 4. is more-or-less an implementation variant of it to avoid rewriting evt.stopPropagation() so it also gets my vote. I am just afraid that the actual implementation may be quite complicated.

@axelboc
Copy link
Contributor Author

axelboc commented Apr 21, 2023

Opting out of specific events (pointerdown, wheel, etc.) may not be the right approach after all. Cancelling pointerdown means cancelling any interaction that begins with this event, regardless of modifier keys and mouse button pressed (e.g. middle button).

I feel like users need to be able to cancel specific interactions instead. More thinking needed...

@axelboc
Copy link
Contributor Author

axelboc commented Aug 10, 2023

Following the refactor of VisCanvas in #1467, the event target for useCanvasEvents would now be canvasWrapper, which is a new wrapper around r3fRoot (since overflowing annotations end up in this container).

Also, the CSS Grid layout on visCanvas would make it easier to position elements on top of the canvas (with grid-area: canvas) and yet outside of canvasWrapper.

If we did this for the floating toolbar, then registering useCanvasEvents on canvasWrapper would have no impact -- the floating controls would still intercept all events out of the box like they do currently without having to resort to evt.stopPropagation(). This would work for any other element that needs to intercept every event.

For elements that need to let some events through, then evt.stopPropagation() remains the best low-level solution. We may have to roll with it and see the impact in practice before trying to come up with a higher-level solution.

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 a pull request may close this issue.

2 participants