search

Kitware / vtk-js

Visualization Toolkit for the Web
https://kitware.github.io/vtk-js/
BSD 3-Clause "New" or "Revised" License
1.24k stars 380 forks source link

RWI keypress event listener #1856

Open finetjul opened 3 years ago

finetjul commented 3 years ago

RenderWindowInteractor (RWI) observes key events (keydown, keypress, keyup) on body.

For plain HTML app, it makes sense: If a key down event is not handled by any DOM element, then the key press event gets to the VTK view. That's handy.

However, React handles events AFTER native event listeners (addEventListener).

Therefore, onKeyDown callback gets called after the RWI. Calling stopPropagate() within the callback is therefore useless and will not prevent RWI from processing the event because it is already processed.

What do you suggest ? 1) do native event listener within React (e.g. addEventListener() in componentDidMount()) 2) Make RWI stops listening <body> but <container> instead ? 3) Delegate event listening to the framework (React, Vue...)

My understanding is that it is the same issue with Vue. I have not looked nor tried, but few weeks ago, I observed something similar in PVGlance. If you enable "the First Person mode" and use keydown/keyup to change the camera zoom factor, it also moves the camera forward in the view.

floryst commented 3 years ago

If you need to run event handlers before RWI handles them, then register your handlers with capture: true. (I forget how to do this in React; possibly onKeyDownCapture?) This will run your handlers during the capture phase, which runs before the bubble phase. The RWI listens on the bubble/propagate phase.

finetjul commented 3 years ago

I did a quick check and capture events (onKeyDownCapture) are also processed AFTER native events.

jourdain commented 3 years ago

Why don't you use some KeyManipulator and control them from React rather than having react deal with some mouse handling?

finetjul commented 3 years ago

Sorry, I was not being clear.

The issue I have is not related to events on the "3D viewport" but when events are on a totally different React element (e.g. a custom ul/li list component). I want a custom behavior on arrow key down events for this React element.

image

VTK.JS processes them (i.e. scroll my current volume slice) before I can do something with it within the React world (i.e. select next line).

I'm not sure what a KeyManipulator is.

jourdain commented 3 years ago

Did you bind any key stuff in vtk.js? Or does the default preventing you getting the key handling in react?

finetjul commented 3 years ago

Do not get me wrong, vtk.js does not prevent the keydown event from being processed by react.

It is just that it is processed by both vtk.js AND react. What I want is the event to be processed by React and if no React component catch it, then the event can be processed by vtk.js.

(and yes, I have an vtk.js interactorstyle/widget that scrolls the slice on key press (by using the RWI event handling).

jourdain commented 3 years ago

We can make it so the container on which we bind the key is configurable but I usually get some issue capturing key press when not done on body/document...

So I'm not sure how we can properly handle it? Unless if you have a mouse enter/leave for the 3D view to activate/deactivate the handling of events for vtk.js?

Also, maybe you need to register vtk.js listener with a -1 as priority (second optional arg) to make them happen after the React ones? But I'm not sure it will actually work.

finetjul commented 3 years ago

addEventListener() does not support "priority".

Nonetheless, if RWI listens to document instead of <body>, then it handles events at the same level than the React Event Handler. And because RWI is typically instantiated within a React component, it addEventListener() AFTER react. It therefore gets the keydown event AFTER React.

So the following works in my React component onKeyDown function:

onKeyDown(event) {
 ...
 event.stopPropagation(); // -> so it does not bubble to React ancestor components 
 event.nativeEvent.stopImmediatePropagation(); // -> so it does not get forwared by RWI
}

This is not great (having to call event.nativeEvent.stopImmediatePropagation()), but it gives definitely more flexibility. Would you see an issue for the RWI to listen to keypress, keydown, keyup events on document rather than body ?

jourdain commented 3 years ago

Moving from body to document make sense to me...

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 17.10.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

finetjul commented 3 years ago

Actually, there is still an issue with events that have a "default behavior" (associated action is performed by the browser if preventDefault is not called) Because those events are typically not eaten by any user specific code, they would be handled by RWI.

This is the case for <input >, or <button> elements. You can see the line widget example, if you type 'c' in the text area. (technically this is not the fault of the RWI, but it is the same idea).

I guess RWI could check the target of the key event, if it falls in the list of "input", "button"... then it can ignore the event. However, if the event is a arrow up/down on a scrollable div, I'm not sure if we can detect it.

jourdain commented 3 years ago

The 'c' thing is because of the FullScreenRenderWindow

finetjul commented 3 years ago

The 'c' thing is because of the FullScreenRenderWindow

I know, I used it as an example of the issues we can get by having the RWI process any remaining event (no stopPropagation nor preventDefault have been called) that require user agent action (e.g. key strokes for <input>).

To me, RWI should observe only the container (not body, nor document), and it is the application's role to: 1) give focus to the container so that it can receive key events 2) optionally forward document events if needed.

And if an <input> is child (i.e. on top) of the container observed by RWI, then it is developper's role to stopPropagation() on the key events before they reach the container.

WDYT ?

jourdain commented 3 years ago

At a high level it make sense. My worry is that when we started to listen on the container it was "hard" to control the focus and make sure we actually capture the events we were supposed to.

We probably still want to either register key events on documents or container with the constraint that if we register them on container, we let the user deal with the focus. I think that would provide the most compromise and reduce complexity when focus handling is not needed.

@floryst what do you think? How does mousetrap handle those kind of things?

jourdain commented 3 years ago

It seems that they simply skip input/select/textarea/editable