movementX and movementY have the wrong value for mousedown and mouseup

NEW
Unassigned

Status

()

defect
P5
normal
7 years ago
6 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:?, blocking-basecamp:-, firefox13 unaffected, firefox14- affected, firefox15- affected, firefox16- affected, firefox-esr10 unaffected)

Details

(Whiteboard: [games:p?])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
See this test case: http://people.mozilla.org/~eakhgari/pointerlock.html

Open the web console, and move the mouse around and click.  This happens both when pointer lock is in effect and when it's not.
(Reporter)

Updated

7 years ago
(Reporter)

Updated

7 years ago
Summary: movementX and movementY have the wront value for mousedown and mouseup → movementX and movementY have the wrong value for mousedown and mouseup
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
It looks like onclick the mozMovementX,Y values are something very close to the MouseEvent.screenX/Y coords. It's not exactly the same, maybe due to the menubar height?
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 2

7 years ago
(In reply to Chris Pearce (:cpearce) from comment #1)
> It looks like onclick the mozMovementX,Y values are something very close to
> the MouseEvent.screenX/Y coords. It's not exactly the same, maybe due to the
> menubar height?

Yeah, and they need to be calculated the same way that they are calculated for mousemove.  I'll try to take a look at this tomorrow.
I think mozMovement{X,Y} should be 0 for mouse down/up/click events, that's what Chrome appears to do.

I am running a patch through Try that makes this change now:
https://tbpl.mozilla.org/?tree=Try&rev=9c95957c365c
I think the pointer lock spec agrees, it's not entirely clear that movement{X,Y} should be non-zero only for mousemove events, but I think that's the implication.

"The members movementX and movementY must provide the change in position of the pointer, as if the values of screenX/Y were stored between two subsequent mousemove events eNow and ePrevious and the difference taken movementX = eNow.screenX-ePrevious.screenX."

http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html#extensions-to-the-mouseevent-interface
(Reporter)

Comment 5

7 years ago
(In reply to Chris Pearce (:cpearce) from comment #3)
> I think mozMovement{X,Y} should be 0 for mouse down/up/click events, that's
> what Chrome appears to do.

No, I believe that's the wrong thing to do.  movement{X,Y} need to have the real values for mousedown/up same way was mousemove.  In my tests with Chrome, if you move the mouse really fast and click immediately in a way that a mousemove event is skipped, a mousedown event will be raised with non-zero movementX/Y.
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> No, I believe that's the wrong thing to do.  movement{X,Y} need to have the
> real values for mousedown/up same way was mousemove.

But that means if script wants to observe all mouse movements it must listen for mouse movement in events other than mousemove? That seems to me to break the contract/abstraction of the mousemove event.

It seems to me that in the case where the mouse have moved before the click comes in, we should dispatch a mousemove event before dispatching the mousedown/up/click events.
CC'ing Vincent for thoughts on reading of the spec.
(Reporter)

Comment 8

7 years ago
(In reply to Chris Pearce (:cpearce) from comment #6)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > No, I believe that's the wrong thing to do.  movement{X,Y} need to have the
> > real values for mousedown/up same way was mousemove.
> 
> But that means if script wants to observe all mouse movements it must listen
> for mouse movement in events other than mousemove? That seems to me to break
> the contract/abstraction of the mousemove event.
> 
> It seems to me that in the case where the mouse have moved before the click
> comes in, we should dispatch a mousemove event before dispatching the
> mousedown/up/click events.

That's a good point...  I mean, maybe compatibility with Chrome isn't too much of a concern.  The spec is really vague here.
(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > > No, I believe that's the wrong thing to do.  movement{X,Y} need to have the
> > > real values for mousedown/up same way was mousemove.
> > 
> > But that means if script wants to observe all mouse movements it must listen
> > for mouse movement in events other than mousemove? That seems to me to break
> > the contract/abstraction of the mousemove event.
> > 
> > It seems to me that in the case where the mouse have moved before the click
> > comes in, we should dispatch a mousemove event before dispatching the
> > mousedown/up/click events.
> 
> That's a good point...  I mean, maybe compatibility with Chrome isn't too
> much of a concern.  The spec is really vague here.

My initial understanding was that all system cursor movement would be presented via mousemove events. On my primary development platform (linux) that appears to be the case for Chrome. That assumption is wrong and or a bug has been introduced recently, I just verified on Chrome canary windows the movement can be lost (though on Chrome 19 I have not discovered that).

IMHO if browsers are able to provide all movement in mousemove events it would be best. I'm not aware of why that shouldn't be possible.

The specification made the assumption that all movement would be delivered via mousemove events. If we believe implementations can always provide mousemove events, I agree the specification should explicitly state that movementX/Y should only be non zero for mousemove events - it would significantly simplify application code.

I'm willing to do what's necessary to fix Chrome to always provide movement via mousemove events.

Here's one of my test pages, btw, that makes loss in pointer motion easy to see manually:
http://scheib.github.com/HTMLMisc/PointerLockAndFullscreen.html
Click on the pink box to align the system cursor and the HTMLCursor. Move frantically without having the mouse locked. The cursors should remain aligned. 

BTW Firefox nightly linux is currently drifting when fullscreen.
Patch, with test.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #633382 - Flags: review?(bugs)
Comment on attachment 633382 [details] [diff] [review]
Patch: Only report MouseEvent.movement{X,Y} values for mousemove events.

Why should movement point be available only on mousemove events?
In general we can't guarantee that the page gets all the mousemove events
(they may go to an iframe for example).
Attachment #633382 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 633382 [details] [diff] [review]
> Patch: Only report MouseEvent.movement{X,Y} values for mousemove events.
> 
> Why should movement point be available only on mousemove events?
> In general we can't guarantee that the page gets all the mousemove events
> (they may go to an iframe for example).

Olli: What I said in comment #6; we'll break the abstraction of the mousemove event if we report mouse moves in events other than mousemove. Authors will have to listen for all up/down/click/move events in order to capture all movements, and that doesn't seem obvious.

If there's unreported movement when a mousedown/up/click is processed, we'd be better to dispatch a mousemove event to report it before dispatching the click (admittedly my patch doesn't do that).

Either way we should clarify the spec... The pointerlock discussion was held in public-webapps right?
public-webapps is the right place for Pointer Lock spec discussion, though I'm the editor and actively listening here.
Would we ever consider backing out bug 633602 if this was left unfixed in FF14? If not, we'll untrack for release.
(Reporter)

Comment 15

7 years ago
Is there any update on this, Chris?
Don't think this blocks basecamp. If I understand correctly you should only be looking at these properties after getting a mouselock, and since you can't do that on B2G (right?) no pages should be looking at these properties.
blocking-basecamp: ? → -
(In reply to Alex Keybl [:akeybl] from comment #14)
> Would we ever consider backing out bug 633602 if this was left unfixed in
> FF14? If not, we'll untrack for release.

We can ship without this being fixed.

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Is there any update on this, Chris?

I've been working on other things.

I was waiting for Olli to reply to my comment 12, and am considering writing a patch to flush mouse movements with a mousemove event before dispatching mouse up/down/click events.
(In reply to Chris Pearce (:cpearce) from comment #12)
> If there's unreported movement when a mousedown/up/click is processed, we'd
> be better to dispatch a mousemove event to report it before dispatching the
> click (admittedly my patch doesn't do that).

As far as I know, we don't report all the mousemoves. There are some tricky cases like moving the window, but
not mouse etc.
(In reply to Jonas Sicking (:sicking) from comment #16)
> Don't think this blocks basecamp. If I understand correctly you should only
> be looking at these properties after getting a mouselock, and since you
> can't do that on B2G (right?) no pages should be looking at these properties.

MouseEvent.mouseMovement{X,Y} are specified to report valid values, regardless of whether the mouse is locked or not.

We also report mouseMovement{X,Y} on B2G for the mouse events generated by a user touch & drag actions, so an app could still rely on mousemove when running on B2G.
Whiteboard: [games:p3]
We're shipping this in FF14, I see no reason to track for upcoming releases in that case.
(Reporter)

Comment 21

7 years ago
Chris, are you working on a fix for this?  I'm planning to work around this bug in Emscripten since clicking can be completely broken in full-screen mode, but that would not be ideal since we may lose some real mouse movement information with that workaround.
I was planning on updating my patch to flush unreported mouse moves before sending mousedown/up/click, but I'm not sure when that will be, maybe a week to two. If you want to take the bug and work on it in the meantime, that's fine with me.
(Reporter)

Comment 24

7 years ago
(In reply to Vincent Scheib from comment #23)
> I've updated the spec to be clear that movement should be delivered via
> mousemove events.
> 
> http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html#extensions-to-
> the-mouseevent-interface
> diff: http://dvcs.w3.org/hg/pointerlock/log/default/index.html

Great, thanks a lot!

Chris, this matches what your patch here does in Gecko right?
(Reporter)

Comment 25

7 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> (In reply to Vincent Scheib from comment #23)
> > I've updated the spec to be clear that movement should be delivered via
> > mousemove events.
> > 
> > http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html#extensions-to-
> > the-mouseevent-interface
> > diff: http://dvcs.w3.org/hg/pointerlock/log/default/index.html
> 
> Great, thanks a lot!
> 
> Chris, this matches what your patch here does in Gecko right?

Ping?
(In reply to Ehsan Akhgari [:ehsan] from comment #25)
> Ping?

Oops, sorry!

> (In reply to Ehsan Akhgari [:ehsan] from comment #24)
> > (In reply to Vincent Scheib from comment #23)
> > > I've updated the spec to be clear that movement should be delivered via
> > > mousemove events.
> > > 
> > > http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html#extensions-to-
> > > the-mouseevent-interface
> > > diff: http://dvcs.w3.org/hg/pointerlock/log/default/index.html
> > 
> > Great, thanks a lot!

This is good. Thanks Vincent.

> > Chris, this matches what your patch here does in Gecko right?

I think my patch here has the effect of *discarding* mouse movement between the last mouse move and the mouse down. This may or may not be a negligible amount of movement. I'm probably not going to get to fixing this for a week or two, since I've got lots of blocking-basecamp+ stuff to work on first.
Bumping this up in priority; Chris, any chance of updating your patch?  Or can you describe what needs to be done?
Whiteboard: [games:p3] → [games:p2]
Vlad: My priority queue is overflowing to the point that it's leaking. I recommend you find someone else to tackle this.
Assignee: cpearce → nobody
Whiteboard: [games:p2] → [games:p?]
This is still happening: http://jsbin.com/yezipiz/8

Updated

10 months ago
Priority: -- → P5
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.