Mouse event positions incorrect when page scrolled about luma.gl HOT 13 CLOSED

@tehwalris Your analysis sounds reasonable.

The background here is that the event code in luma.gl is really old, inherited in the original fork from PhiloGL and has only been changed cosmetically since then. The code is not well documented and does not have a good test cases or examples. It should probably be refactored (or perhaps even be replaced with something better), but so far this has not been a priority.

That said, I'm positive to a PR but a little worried about our ability to quickly review it and test it for correctness. If you add clear comments on what the code is doing (at a minimum add a JSDoc header with an edit of your comments above) and you leave the code in better shape than it is now that will make it easier for us to accept the PR. A test case or an example that helps us test events would be great, but not required, as I don't want to raise the bar too high.

from luma.gl.

@ibgreen I had the same concerns about the stability of changes to those parts of the code. I'll try to get something written next week with some basic tests.

from luma.gl.

I've thought some more about rewriting this event handling code. Ideally, we should make sure that these events are handled correctly in all target browsers - since the events they give might not be exactly the same. For this testing to make sense we need real click and touch events.

I thought webdriver might be a good candidate for this. I attempted to get it working, but I couldn't find a good way to integrate it with the existing test code.

Do we want to use something like webdriver for testing this event related code? How will that integrate with the current testing solution?

from luma.gl.

Do we want to use something like webdriver for testing this event related code? How will that integrate with the current testing solution?

The idea is good - the problem is that this is a WebGL repository. Unless webdriver has webgl support it will be a heavy machinery to include and maintain just to test a few files.

We have tried electron integration in the past, and also headless-gl, for the WebGL piece, but they are hardly useful here.

Do you have good experience with webdriver. Is it the right choice?

@chrisirhc Maybe you have some experience or thoughts about webdriver?

from luma.gl.

@ibgreen I've decided to leave webdriver and similar alone for now. It would be really hard to test what we actually need to test with automated testing - that click events happen where the users pointer/touch physically occurs.

I have made some test pages which can be used to verify that events are working correctly on different devices by hand. They are not pretty though, since they use raw DOM manipulation to not add any dependencies. Whats your opinion on that approach?

I've also started work on a new event handling system, which has raised a lot of questions:

• How far do we want to abstract from raw events, in general?
  • I think we should try keep our abstraction relatively minimal - just provide element relative position, normalized pressed mouse buttons and modifier keys.
• Should we provide down/up/move events which work for both touch and mouse?
  • I think this would be a useful addition. Sort of like polymer does.
• Do we want to support multi-touch, if we support touch events?
  • This seems very complicated to make a truly useful implementation of and out of scope.
• Do we want to support keyboard events?
  • Since keyboard event handlers are usually just attached to the document and have nothing to do with the luma.gl scene and canvas, I think this is best left to the user. Also keyboard events are very hard to normalize in current browsers.
• Do we want to leave the legacy event handling, wrap it for compatibility or immediately replace it with the new interface?

I'd love to have some opinions on those questions.

from luma.gl.

@tehwalris This looks impressive, I love the attention to tests, and your code is already pretty clean. I'd love to give some feedback so if you could put it up as an exposition only PR I'll give you a formal review.

In addition to manual tests, it would be great with a CI sanity test script that just checked that imports are working and that addEvents is called without crashing.

from luma.gl.

For your questions:

How far do we want to abstract from raw events, in general?

Agreed. There are probably good modules out there is someone needs more.

Should we provide down/up/move events which work for both touch and mouse?

Yes! makes perfect sense!

Do we want to support multi-touch, if we support touch events? This seems very complicated to make a truly useful implementation of and out of scope.

I think that pinch to zoom and e.g. a two finger pan / click would be very useful. At least the first case could probably be handled similar to mouse wheel?

Do we want to support keyboard events?

Not a strong opinion. Maybe we just allow the keyboard events to be registered, and provide a minimum of constants.

Do we want to leave the legacy event handling, wrap it for compatibility or immediately replace it with the new interface?

We don't want to break apps without a major bump to the API (semver.org), and we don't want to bump too often.

In the mean time, things we plan to remove in the next version are moved to the src/deprecated folder. But note that even if you move the files, for now it must still be possible to import the same way (through addons), otherwise apps break.

from luma.gl.

I see #113 is merged. Does this mean this bug is fixed on the uber:events branch?

from luma.gl.

I see #113 is merged. Does this mean this bug is fixed on the uber:events branch?

@dcposch As I recall, the PR was merged into the experimental folder on the events branch as a candidate to be a replacement event system.

Are you being affected directly, or through deck.gl? We could potentially try to get the new events handling merged into dev and publish a 3.0.0-alpha version that includes it.

It would still be in the experimental folder though, so there would be an additional step if this relates to deck.gl.

from luma.gl.

@ibgreen I'm using deck.gl

from luma.gl.

@dcposch Reflecting on this, I think the biggest issue with this scrolling bug is that all examples in deck.gl are full screen, so they don't trigger it. We should add a scrolling example to deck.gl for the upcoming release, that should make this issue easy to reproduce and close. Will make a note in deck.gl issue.

from luma.gl.

Sure. More immediately, I think this can be fixed by simplifying luma.gl.

Right now it looks like luma.gl is trying to calculate the offset of the canvas to the page, then subtracting that from the mouse event position to get X and Y relative to the canvas. Apparently, it's doing it wrong.

You could just use offsetX and offsetY from the mouse event to get a position relative to the canvas directly, no math required.

https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/offsetX

(It's marked "experimental", but apparently supported in all browsers all the way back to IE6.)

@ibgreen

from luma.gl.

Looks like this was addressed here: visgl/deck.gl#445

from luma.gl.