-
Notifications
You must be signed in to change notification settings - Fork 209
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
Mouse event positions incorrect when page scrolled #112
Comments
@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. |
@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. |
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? |
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? |
@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:
I'd love to have some opinions on those questions. |
@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. |
For your questions:
Agreed. There are probably good modules out there is someone needs more.
Yes! makes perfect sense!
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?
Not a strong opinion. Maybe we just allow the keyboard events to be registered, and provide a minimum of constants.
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. |
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 Are you being affected directly, or through deck.gl? We could potentially try to get the new events handling merged into It would still be in the experimental folder though, so there would be an additional step if this relates to deck.gl. |
@ibgreen I'm using deck.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. |
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 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.) |
Looks like this was addressed here: visgl/deck.gl#445 |
This issue is the root cause of deck.gl issue 248. When clicking on a luma.gl view which is partially scrolled off screen by the pages scroll bar, relative mode click positions are incorrect. This issue does not occur if the luma.gl view is scrolled off screen by a containing elements scroll bars.
I think I've identified what's causing this issue. In the calculation of relative event positions,
pos
is subtracted fromx
/y
(which isepos
). Sinceepos
is page relative, butpos
is client relative, positions are incorrect if the client has scrolled the page.EventsProxy
options used arerelative
true
andcenterOrigin
false
.Is my reasoning correct? If so, I'll can submit a PR which addresses the issue.
The text was updated successfully, but these errors were encountered: