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

Invalid pageX and pageY properties returned in non-mouse events on Firefox #3692

Closed
stevenbenner opened this Issue Jun 11, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@stevenbenner
Contributor

stevenbenner commented Jun 11, 2017

Description

This is probably a Firefox issue more than a jQuery issue, but it seems to be a regesseion in that it's something that jQuery used to handle but no longer does.

With the latest version of jQuery I am finding that invalid pageX and pageY properties are being returned on Firefox for events that should not contain that data.

The issue only manifests in Firefox for me. Chromium and IE don't seem to have the problem. Firefox does include the invalid pageX and pageY data in the raw event.

This was causing a bug in one of my jQuery plugins (stevenbenner/jquery-powertip#158).

Link to test case

https://jsfiddle.net/stevenbenner/f94zetz0/
(Click on the input and note the output)

This checks what the type and value of the pageX property is when the focus event is fired with multiple versions of jQuery, as well as the raw DOM event.

Firefox 52.0 32-bit Windows (no add-ons) and Firefox 53.0.3 64-bit Linux (lots of add-ons):

Raw Event: number - 0
jQuery 3.2.1: number - 0
jQuery 2.2.4: undefined - undefined
jQuery 1.12.4: undefined - undefined

Chromium 58, Internet Explorer 11

Raw Event: undefined - undefined
jQuery 3.2.1: undefined - undefined
jQuery 2.2.4: undefined - undefined
jQuery 1.12.4: undefined - undefined
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 11, 2017

Member

@stevenbenner I've pinged the twitter webcompat account about this via https://twitter.com/davemethvin/status/874008700205363200 to see if this should properly be undefined for a focus event. The reason you're seeing it only on jQuery 3 is because we now transparently/lazily return more properties from the original event and as your test case shows the value is 0 in Firefox.

Member

dmethvin commented Jun 11, 2017

@stevenbenner I've pinged the twitter webcompat account about this via https://twitter.com/davemethvin/status/874008700205363200 to see if this should properly be undefined for a focus event. The reason you're seeing it only on jQuery 3 is because we now transparently/lazily return more properties from the original event and as your test case shows the value is 0 in Firefox.

@stevenbenner

This comment has been minimized.

Show comment
Hide comment
@stevenbenner

stevenbenner Jun 12, 2017

Contributor

Thanks. I'm interested to hear peoples thoughts on this.

Based on the MDN docs, I suspect that these properties are included in Firefox because the FocusEvent inherits from UIEvent, and in their implementation that includes UIEvent.pageX (note that the doc page has a warning that it is non-standard). These properties are probably only trustworthy when coming from a MouseEvent (e.g. MouseEvent.pageX).

So it certainly looks like Firefox is not to spec on this one.

Is this the type of thing that jQuery should normalize? A fix should be fairly trivial to implement, but hacky since the value of these properties cannot be trusted. My workaround was to compare the event type to a list of events that would be expected to include cursor position data, and ignore the coordinate properties included with any events that are not matched.

Contributor

stevenbenner commented Jun 12, 2017

Thanks. I'm interested to hear peoples thoughts on this.

Based on the MDN docs, I suspect that these properties are included in Firefox because the FocusEvent inherits from UIEvent, and in their implementation that includes UIEvent.pageX (note that the doc page has a warning that it is non-standard). These properties are probably only trustworthy when coming from a MouseEvent (e.g. MouseEvent.pageX).

So it certainly looks like Firefox is not to spec on this one.

Is this the type of thing that jQuery should normalize? A fix should be fairly trivial to implement, but hacky since the value of these properties cannot be trusted. My workaround was to compare the event type to a list of events that would be expected to include cursor position data, and ignore the coordinate properties included with any events that are not matched.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 12, 2017

Member

Just about all the browsers are on a fast update cycle nowadays. If this is a compat issue it should be fixed by Firefox relatively quickly. It seems like an obscure enough issue that code sensitive to this should be able to check for it. Since events can occur at a high frequency, jQuery tries to keep extra code out of that path.

Member

dmethvin commented Jun 12, 2017

Just about all the browsers are on a fast update cycle nowadays. If this is a compat issue it should be fixed by Firefox relatively quickly. It seems like an obscure enough issue that code sensitive to this should be able to check for it. Since events can occur at a high frequency, jQuery tries to keep extra code out of that path.

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Jun 12, 2017

@denschub @wisniewskit might have an idea about it.

@stevenbenner did it start with a specific version of Firefox?

karlcow commented Jun 12, 2017

@denschub @wisniewskit might have an idea about it.

@stevenbenner did it start with a specific version of Firefox?

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Jun 12, 2017

In https://github.com/jquery/jquery/search?q=pagex&type=Commits&utf8=%E2%9C%93
The most recent change is:

931f45f
Event: Remove pageX/pageY fill for event object

Ah also Safari Version 10.1.1 (12603.2.4) and Safari Tech Preview Release 32 (Safari 11.0, WebKit 12604.1.23.0.4) return the same results than Firefox.

karlcow commented Jun 12, 2017

In https://github.com/jquery/jquery/search?q=pagex&type=Commits&utf8=%E2%9C%93
The most recent change is:

931f45f
Event: Remove pageX/pageY fill for event object

Ah also Safari Version 10.1.1 (12603.2.4) and Safari Tech Preview Release 32 (Safari 11.0, WebKit 12604.1.23.0.4) return the same results than Firefox.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jun 12, 2017

Member

I don't want to have to keep a list of all appropriate event types (or even a blacklist), because that's not future-proof. It seems clear to me that we're doing the right thing here. We're even returning the property value lazily, meaning we no longer copy over this property ahead of time. That is a step in the right direction and possibly the best course we can take. Since the value isn't reliable or useful to any user, this is not a high priority issue. It's unlikely we'll change anything here.

Member

timmywil commented Jun 12, 2017

I don't want to have to keep a list of all appropriate event types (or even a blacklist), because that's not future-proof. It seems clear to me that we're doing the right thing here. We're even returning the property value lazily, meaning we no longer copy over this property ahead of time. That is a step in the right direction and possibly the best course we can take. Since the value isn't reliable or useful to any user, this is not a high priority issue. It's unlikely we'll change anything here.

@timmywil timmywil closed this Jun 12, 2017

@timmywil timmywil removed the Needs review label Jun 12, 2017

@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.