/jquery

New issue

MouseEvent.pageX/pageY fill isn't needed in jQuery 3.0 #3092

Closed
anseki opened this Issue Apr 29, 2016 · 6 comments

Assignees

@dmethvin dmethvin

Labels

Behavior Change Event

Projects

None yet

Milestone

  3.0.0

3 participants

@anseki @dmethvin @gibson042
@anseki
anseki commented Apr 29, 2016

Related to: #3080
Original comment: #3080 (comment)

MouseEvent.pageX/pageY also might be influenced.
https://github.com/jquery/jquery/blob/9f268caaf43629addb9a1a2001ab341839300b14/src/event.js#L435
If native event don't have pageX/Y, these are calculated using clientLeft/clientTop.

If html element has border-width:10px like the example above, in a browser that doesn't support event.pageX/Y and correct clientTop/clientLeft, jQuery returns 10,10 as pageX/Y, when mouse is positioned at left-top-corner of body. In a browser that returns correct clientTop/clientLeft, jQuery returns 0,0 as pageX/Y.

I feel that 10,10 is correct result.

But I don't know recent browser that doesn't support event.pageX/Y.

Since Firefox returns 0 as clientTop/clientLeft of html element always, MouseEvent.pageX/pageY might be incorrect.
When native event object doesn't have pageX/Y, jQuery.event.mouseHooks.filter calculates these using document.documentElement.clientLeft/clientTop (i.e. border-width of html element).
(But I don't know recent browser that doesn't support event.pageX/Y.)

Represent:
https://jsfiddle.net/rent9q5g/
This simulates a browser that doesn't support event.pageX/Y.

Chrome:
m-ch

IE:
m-ie

Firefox:
m-ff

incorrect result in Firefox by its bug, but I think pageX: 10 pageY: 10 is correct result. That is, jQuery.event.mouseHooks.filter should not subtract clientTop/clientLeft.
@dmethvin
Member
dmethvin commented Apr 29, 2016

It's possible we don't need that code at all in 3.x (maybe even 2.x) depending on where pageX/Y are provided. If https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/pageX is correct it seems we should be covered even for 2.x. Removing it would be better than fixing it!
@gibson042
Member
gibson042 commented Apr 29, 2016

Removing it would be better than fixing it!

Agreed.
@dmethvin
Member
dmethvin commented Apr 30, 2016

I think we should remove the pageX/Y fill in 3.0, it seems to have full browser support. Let's discuss on Monday, I won't submit a PR now because it will conflict with @jbedard 's PR and we might as well create one on top of that.
@dmethvin dmethvin added a commit to dmethvin/jquery that referenced this issue May 5, 2016
@dmethvin dmethvin Event: Remove pageX/pageY fill for event object 
Fixes gh-3092

IE8 was the last major browser missing these.
 8024be7
@dmethvin dmethvin referenced this issue May 5, 2016
Closed

Event: Remove pageX/pageY fill for event object #3106

2 of 4 tasks complete
@dmethvin
Member
dmethvin commented May 6, 2016

Hey @anseki I just wanted to confirm that this problem does not exist in any of the event.pageX values that the browser calculates, correct? Once we remove our calculation as proposed in gh-3106 there should be no problem. (Unless the browser doesn't support event.pageX!)
@dmethvin dmethvin added this to the 3.0.0 milestone May 6, 2016
@dmethvin dmethvin self-assigned this May 6, 2016
@anseki
anseki commented May 6, 2016

I think that pageX/pageY of native event object are correct.
I updated the code above in JSFiddle.
https://jsfiddle.net/rent9q5g/2/
At least, Native pageX/pageY seem correct in current Firefox, Chrome and IE.
@dmethvin
Member
dmethvin commented May 6, 2016

Awesome, thanks for the confirmation!
@dmethvin dmethvin added a commit that closed this issue May 6, 2016
@dmethvin dmethvin Event: Remove pageX/pageY fill for event object 
Fixes gh-3092
CLoses gh-3106

IE8 was the last major browser missing these.
 931f45f
@dmethvin dmethvin closed this in 931f45f May 6, 2016
@dmethvin dmethvin changed the title from Incorrect `MouseEvent.pageX/pageY` to MouseEvent.pageX/pageY fill isn't needed in jQuery 3.0 May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment