Skip to content
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

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

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

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

anseki opened this issue Apr 29, 2016 · 6 comments
Assignees
Milestone

Comments

@anseki
Copy link

@anseki anseki commented Apr 29, 2016

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

MouseEvent.pageX/pageY also might be influenced.

( doc && doc.clientLeft || body && body.clientLeft || 0 );

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
Copy link
Member

@dmethvin 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
Copy link
Member

@gibson042 gibson042 commented Apr 29, 2016

Removing it would be better than fixing it!

Agreed.

@dmethvin
Copy link
Member

@dmethvin 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 added a commit to dmethvin/jquery that referenced this issue May 5, 2016
Fixes jquerygh-3092

IE8 was the last major browser missing these.
@dmethvin
Copy link
Member

@dmethvin 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
Copy link
Author

@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
Copy link
Member

@dmethvin dmethvin commented May 6, 2016

Awesome, thanks for the confirmation!

@dmethvin dmethvin closed this in 931f45f May 6, 2016
@dmethvin dmethvin changed the title Incorrect MouseEvent.pageX/pageY MouseEvent.pageX/pageY fill isn't needed in jQuery 3.0 May 6, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants