New input event handling #113

tehwalris · 2016-12-10T21:21:31Z

This is the new event handling system that I'm creating, with the goal to replace the legacy code in luma.gl which is currently handling events, following issue #112. This is not a full PR yet, just a request for review. The following points need to be addressed before this is ready:

Wrapper to maintain legacy interface, to not make breaking changes
Combined mouse/touch event interface (extra to separate ones)
Not adding event listeners to DOM for events the user hasn't requested
Picking support
Mouse wheel support
Documentation
Automated smoke test that create simple event listeners (current tests are all interactive)

There is one known issue in this code, which I have not found a way to fix. In Chrome on Android, the address bar scrolls off screen, together with the page scroll. This means that the viewport size changes during movement. It seems that clientX/Y on touch events are still reported relative to the original viewport as the user is dragging to hide this address bar. This means that the user sees the pointer in a slightly wrong position if they are performing a touch which is also scrolling the page and this address bar out of view. After releasing their touch, so that the address bar "confirms" it's change to show/hide, everything works correctly again. You can use the basic "touch" test page to see this issue.

I'd love to get some feedback on my work so far, as well as any ideas to fix that issue.

…events.

ibgreen

@tehwalris

Thanks for opening a PR, it looks like most of the functionality we talked about is in there!

This is not a proper review. I have mostly tried to read the code and attempt to understand the design, and made some comments as I went along. They are mostly stylistic at this point.

To do a proper review, what I most need to understand is what the public API will look like (is it just the addEvents function that is public, and are all the other classes internal?).

Also if there was a single line of comment, or even better proper JSDoc, at least on the constructors and public methods of all classes, that would really help clarify the intended design.

ibgreen · 2016-12-11T17:12:40Z

src/core/event/EventManager.js

@@ -0,0 +1,60 @@
+/* global console */


Filenames. Convention in luma.gl is to use underscore_case for filenames, matching names of classes that are in PascalCase.

So this would be event-manager.js

ibgreen · 2016-12-11T17:15:04Z

src/core/event/ElementRelativePositionTransformer.js

@@ -0,0 +1,15 @@
+import autobind from 'autobind-decorator';
+
+export default class ElementRelativePositionTransformer {


Unless there is more coming, maybe a creating a separate class and file is overkill here? A utility function transformRelativeElementPosition(element, eventPosition) would seem more lightweight and appropriate, and such a function could be placed in the same file as a couple of other utilities.

ibgreen · 2016-12-11T17:30:41Z

src/core/event/createGetModifierState.js

@@ -0,0 +1,19 @@
+const modifierEventKeyMap = {


Seems like this could go in a utils.js file together with ElementRelativePositionTransformer.js ?

ibgreen · 2016-12-11T17:32:39Z

src/core/event/createGetModifierState.js

+  Shift: 'shiftKey'
+};
+
+export default function createGetModifierState(rawEvent) {


Is there a way to do this without a closure? Not a huge thing, but it seems like an ES5 idiom. Even the createGetModifierState name is a good indication, combining create and get in the same function makes it sound more complex than it is.

ibgreen · 2016-12-11T17:34:12Z

src/core/event/addEvents.js

+import TouchEventManager from './TouchEventManager';
+
+export default function addEvents(element, userHandlers, options) {
+  const tranformPosition = (new ElementRelativePositionTransformer(element)).fromViewportRelative;


The parenthesis around the new expression should not be needed here.

ibgreen · 2016-12-11T17:44:44Z

src/core/event/EventManager.js

+/* global console */
+
+export default class EventManager {
+  constructor(nextHandlers) {


The name here throws me off, i.e. the next in nextHandlers? Is the idea that the app can have multiple handlers for the same event? Why not just call the parameter handlers?

Some JSDoc would be invaluable.

ibgreen · 2016-12-11T17:46:27Z

src/core/event/EventManager.js

+    this._nextHandlers = nextHandlers;
+  }
+
+  attach(element) {


So the idea here is that the same handler can be added to multiple elements?
Something to consider: Is that worth the additional complexity in the API? As opposed to just creating new handlers.

ibgreen · 2016-12-11T17:46:56Z

src/core/event/EventManager.js

+
+  attach(element) {
+    for (const eventType of this.constructor.handledRawEventTypes) {
+      element.addEventListener(eventType, this[eventType]);


Another thought: If we add handlers, should we also have a way of removing them? Maybe a TODO for the design?

ibgreen · 2016-12-11T17:50:45Z

src/core/event/EventManager.js

+    };
+  }
+
+  getMiddlewareHandlers() {


The naming here:

What do we mean with 'middleware' in this context?

No underscore - is this a public function? If so when should it be called?

luma classes usually have public methods first, followed by private (underscore) methods.

Again JSDoc would come to the rescue here.

ibgreen · 2016-12-11T18:10:56Z

Documentation
Automated smoke test that create simple event listeners (current tests are all interactive)

@tehwalris

Among the things you list, I'd prioritize those two points, as they will accelerate getting this merged. Others look like nice incremental improvements.

The more docs there are, the more straightforward it will be to review this, e.g:

An overview, listing what features this implements (e.g. that the old system didn't) (README.md)
Describing the public API (README.md)
And code comments explaining the design (inline in source)
Also since this is getting pretty big, I think we might want to elevate it into its own "submodule" (rather than put it in core) which means that we would create a src/events directory and move the code there:
It would have its own README.md
and its own test subdirectory inside that directory.

… to util file.

ibgreen · 2016-12-14T18:55:25Z

@tehwalris I have created a new branch, 'events', off the latest master - rebase your PR towards that branch and I'll merge. It would allow us to test what you are doing, propose some changes and help you out with some of the remaining work.

One thing we would like to build are some simple event controllers on top of the new event handling, so it would be good to get it in place soon.

tehwalris · 2016-12-14T19:46:11Z

@ibgreen I've rebased my PR, however merging this directly is a breaking change. The new event framework is currently in the place where the legacy one was and there is no back-compatibility layer implemented yet.

What do you mean by "event controllers" that you want to implement?

ibgreen · 2016-12-14T20:05:07Z

What do you mean by "event controllers" that you want to implement?

@tehwalris- I'm thinking about packaging up some common interaction patterns (like panning/tilting a plane, or rotating around a center point) into simple classes that registers events and keeps updating a view matrix when events are fired.

This could be used to package up the current deck.gl event handling or create something like the event THREE.js orbit controls

This would allow a luma app to add a basic interactivity with a single line of code.

ibgreen · 2016-12-14T20:07:57Z

I've rebased my PR, however merging this directly is a breaking change.

We could move your new module to src/experimental and require the user to import it using
import 'luma.gl/experimental'. The new EffectsManager in deck.gl is an example of this approach.

We'd thus keep the old event handling as default for now while making it possible to play with your new system e.g. in deck.gl.

What do you think?

tehwalris · 2016-12-14T20:12:58Z

I think experimental approach to merging is exactly what we need here.

For the event controllers you are making, they would be implemented extending EventManager like the touch gesture one, right?

For further updates to the event code, I should open another PR, correct?

ibgreen · 2016-12-14T20:28:51Z

Sounds good. You can open a new PR now to move the code over to experimental, that will make it easier to review your changes.

About the controllers - I expect they might need touch events from several sources (e.g. both mouse and keyboard) so subclassing from one of these controllers might be too restrictive?

As you can probably see, I am still not fully up to speed on your vision here - e.g. if applications are expected to use those controllers directly or mostly use addEvents.

tehwalris · 2016-12-14T21:10:20Z

@ibgreen I expected applications to only use addEvents. Here's a small explanation of my current design. I will add this along with other documentation I'm working on to a PR when I have time. Currently this is just personal work and my time for working on this is quite limited. Hopefully it's still helpful in the meantime. Possibly your controllers will fit in here directly.

Different events are handled by EventManagers. These are objects which listen on certain "input events" and fire "output events". The MouseEventManager and TouchEventManager get input events directly from the browser - they are attached to the DOM element. Handler functions which the user passes to addEvents get called for the corresponding "output events" of an EventManager. These managers do things like position conversion and normalizing the interface for modifier keys.

So for example a mouse down event goes through the following:

DOM element ---(as mousedown)--> MouseEventManager ---(as onMouseDown)--> users event handler

Then there are EventManagers like the TouchGestureEventManager which provides an abstraction (drag and pinch) over basic touch events. It is a sort of middleware, since its input events come from the TouchEventManager not the DOM element. It also has state to track what kind of gesture is currently in progress. The input events of this sort of EventManager can also come from multiple event managers (eg. touch and keyboard) - this was done to allow an creating an EventManager to combine mouse and touch input.

For a touch down event, a possible flow is:

DOM element ---(as touchstart)-->TouchEventManager
                                          |---(as onTouchStart)--> users event handler
                                          |---(as onTouchStart)--> TouchGestureEventManager ---(as onTouchGestureStart)--> users event handler

ibgreen · 2016-12-14T22:43:41Z

@tehwalris
Oh I see, the EventManagers can be chained/composed. Then it makes more sense. I think that was the piece I was missing.

I will add this along with other documentation I'm working on to a PR when I have time.

Sounds good. I wasn't sure about the priority for you, since I think this all originated from a bug you reported in deck.gl. I totally appreciate that this is something you are doing for fun. The pace is up to you. Contributions are always welcome and I am mainly trying to make sure you are unblocked and give you clear guidance on how to get this merged into master.

tehwalris added 7 commits December 3, 2016 17:33

Mouse events and keyup keyboard event.

91c7fdb

Removed keyboard events. Pointer position in separate field on mouse …

c48e3b8

…events.

Test page selector.

317252b

Removed old interaction tests. Added more interaction test pages.

36c5e91

Removed unused dependecy (lodash.foreach).

aa27d07

Basic touch support in new event handling.

80761f5

Improved touch support. Touch gestures.

24b21fb

ibgreen reviewed Dec 11, 2016

View reviewed changes

tehwalris added 2 commits December 12, 2016 08:34

Clearer naming in event handling framework (incoming/outgoing events).

f59a451

Change filename style to match project. Move small separate functions…

9e92930

… to util file.

tehwalris changed the base branch from master to events December 14, 2016 19:42

ibgreen merged commit 98a2e03 into visgl:events Dec 14, 2016

dcposch mentioned this pull request Jan 19, 2017

Mouse event positions incorrect when page scrolled #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New input event handling #113

New input event handling #113

tehwalris commented Dec 10, 2016

ibgreen left a comment

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen Dec 11, 2016

ibgreen commented Dec 11, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016

		@@ -0,0 +1,15 @@
		import autobind from 'autobind-decorator';

		export default class ElementRelativePositionTransformer {

New input event handling #113

New input event handling #113

Conversation

tehwalris commented Dec 10, 2016

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented Dec 11, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016

tehwalris commented Dec 14, 2016

ibgreen commented Dec 14, 2016