1300421 - clientX/screenX of touch events reported as device pixels not css pixels

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Matt Cosentino]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160901030202

Steps to reproduce:

Listen to touchmove event and check the clientX or screenX property of the Touch object.


Actual results:

The values are reported as device pixels.


Expected results:

The values should be reported as css pixels.

Comment 1

[Matt Cosentino]

Firefox: css pixels
Firefox Beta: css pixels
Firefox Nightly: device pixels

Comment 2

[ovidiu boca[:Ovidiu]]

Hi Matt,

Can you please provide a test case, some steps to reproduce this issue? 
This sound like a regression.

Comment 3

[Matt Cosentino]

1. Open the console
2. window.addEventListener('touchstart', function(event){ console.log(event.touches[0].screenX); });
3. Zoom the page
4. Touch the screen
5. Check the value in the console

Comment 4

[ovidiu boca[:Ovidiu]]

Hi Matt,

(In reply to Matt Cosentino from comment #3)
> 1. Open the console
Web console or browser console? I assume that you are talking about web console

> 2. window.addEventListener('touchstart', function(event){
> console.log(event.touches[0].screenX); });
What should I do with this function? I need to paste it in web console? I did that and I receive that is  undefined. 

Tested on Windows 10 x64 with FF Nightly 51.0a1

Comment 5

[Matt Cosentino]

Yes, the web console. That function adds the event listener, it does not return a value. It will log to the web console the value of screenX when you touch the screen. You will see that the value is the same no matter what zoom level is set.

Comment 6

[ovidiu boca[:Ovidiu]]

Attachment: Recording #1 bug 1300421.mp4

Hi Matt, 
I tested again this issue on Windows 10 x64 with the latest FF Nightly 51.0a1(2016-09-12) and I can't reproduce the issue. 

I will add the test page that I created: http://jsbin.com/yaqukovuje/edit?html,js,console,output   and the video from you with your output. 
Please update your FF Nightly version and retest this issue.

Comment 7

[Matt Cosentino]

Tested with Nightly 51.0a1 (2016-09-13) and I still get the issue. Tested in safe mode and with e10s both on and off. It appears that clientX is now being correctly reported in CSS pixels though, so the issue is limited to just screenX.

Comment 8

[ovidiu boca[:Ovidiu]]

There will me one more think that you can try to see if doesn't help you, open Firefox Nighlty with new profile. Here you have a link that can help you to do that: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager

Due to the fact that from your description it shows that this is a regression I want to ask you if you are willing to try a tool that can help us narrow down a regression. It's call mozregression, basically just automates the process of downloading different builds and running them with a clean profile so you can say "Yes, it works" or "No, it doesn't" to each. Here is the link http://mozilla.github.io/mozregression/install.html. 

But first please make a new profile a see if you can reproduce this.

Comment 9

[Matt Cosentino]

Ok, I'm getting different results now, what I previously said might not be completely correct:

Nightly on Win 10: screenX is in device pixels, clientX is in css pixels
Nightly on Android: screenX is in css pixels, clientX is in device pixels

Firefox on Win 10: touch events not supported
Firefox on Android: screenX is in device pixels, clientX is in device pixels

But, if <meta name="viewport" content="width=device-width"> is set:

Nightly on Android: screenX is in device pixels, clientX is in css pixels
Beta on Android: screenX is in css pixels, clientX is in css pixels
Firefox on Android: screenX is in css pixels, clientX is in css pixels

I was unable to find a regression point on Win 10, it seems that I was wrong and screenX was never reported in css pixels. Apparently mozregression supports Fennec, but it's not clear on how to use it.

Comment 10

[ovidiu boca[:Ovidiu]]

From your last comment I see that you reproduce this on an Android device, right? And on Win 10 screenX was never reported in css pixels. 
Based on the fact that the problem is on device, please give more details about what device you use to try to test it.

Comment 11

[Matt Cosentino]

Yes, it is a regression on Android, and it has never been correct on Win 10. My guess is that when support for touch events was added to Win 10 it was broken on Android.

Nexus 5
Android 6.0.1

Aurora on Android: screenX is in css pixels, clientX is in css pixels

I'm trying to enable touch events on Windows using dom.w3c_touch_events.enabled, but it doesn't seem to be working. Is there another preference that is blocking it?

Comment 12

[ovidiu boca[:Ovidiu]]

Kats can you please take a look at this bug and maybe you can help us here?
There is another pref that is blocking the touch events to be enable?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Touch events on windows are currently only supported on nightly with e10s enabled. So if you are on non-nightly you should not be getting any touch events at all unless you have flipped prefs. And on nightly you need to make sure e10s is force-nabled before you'll get the correct touch events (note that devices with touch support will not have e10s enabled by default. So basically this cannot be a regression on windows because we have no releases with touch events out.

Note also that pinch zoom (on Android) and browser zoom (on desktop) are different and will behave differently. It sounds like you are possibly conflating the two. Matt, if you can provide a standalone test page and specific str for what you believe the bug to be I can investigate and/or run mozregression.

Comment 14

[u421692]

Tested on Firefox for Android:

I've tested with the following test page: http://jsbin.com/yaqukovuje/edit?html,js,console,output on Nexus 9 (Android 6.0.1) on latest Nightly and Aurora, and can see different behaviors:

Trying to tap on the margin of the screen on Aurora: the max value I've got was 761
Trying to tap on the margin of the screen on Nightly: the max value I've got was 1520

If the ratio between css pixel and device pixel is 2, I think this might be an issue
I'm not really familiar with this type of issues, Kats, what dou you think? 
Should I try to find a regression window for this?

Comment 15

[Matt Cosentino]

Thanks, force-enabling e10s did the trick. I can now see that the release version of Firefox also reports screenX in device pixels instead of css pixels.

Here is a test case:

https://www.arclearn.com/screenX.jsp

Tap the screen
The first value is screenX, second is clientX
They should both be reported in css pixels

https://developer.mozilla.org/en-US/docs/Web/API/Touch/screenX

"The Touch.screenX property is the horizontal (x) coordinate of a touch point relative to the screen in CSS pixels."

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mihai Pop from comment #14)
> Tested on Firefox for Android:
> 
> I've tested with the following test page:
> http://jsbin.com/yaqukovuje/edit?html,js,console,output on Nexus 9 (Android
> 6.0.1) on latest Nightly and Aurora, and can see different behaviors:
> 
> Trying to tap on the margin of the screen on Aurora: the max value I've got
> was 761
> Trying to tap on the margin of the screen on Nightly: the max value I've got
> was 1520
> 
> If the ratio between css pixel and device pixel is 2, I think this might be
> an issue
> I'm not really familiar with this type of issues, Kats, what dou you think? 
> Should I try to find a regression window for this?

Yes please. If the behaviour changed on Android between Aurora and Nightly that's unexpected, I would like to get the regression window for that. Thanks!

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I bisected this using the STR/URL in comment 15 on Fennec. This was the regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1ea96bf70664dbac2584881fcf39408733278e46&tochange=4eb5775fd334887121a7a779a27e6727acdcb25f

Jonathan, can you take a look? Let me know if you don't have cycles to take this in the near future. Thanks!

Comment 18

[Kartikaya Gupta (email:[e-mail])]

To be clear, what I did was:

- Load the URL
- Tap the screen
- Check if the two numbers were the same. Same == good, different == bad.

Comment 19

[Jonathan Kew [:jfkthame]]

Yeah, it's entirely plausible that bug 1288760 could have affected behavior here. Getting the coordinate handling to be reliable across different platforms (where the underlying platform behavior is different) and with mixed resolutions is a huge pain. It sounds like we were never really correct/consistent here across both Windows and Android. :(

I probably won't get to look into this for at least a couple weeks, as I'm tied up with some other issues at the moment, so if you (or anyone else) have a chance to poke at it, that'd be great.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Hmm, sounds like we need to then backout bug 1288760.

Comment 21

[Masatoshi Kimura [:emk]]

Excluding the first part.
(I suggest to open a new bug for the rest part to make issue tracking simpler.)

Comment 22

[Kartikaya Gupta (email:[e-mail])]

I kicked off a try push with the last 4 csets from bug 1288760 backed out to verify it fixes the problem and still passes tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f288aa046425

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Update a test to catch the error

I updated one of the existing APZ tests to check for this bug. The test just simulates a user tapping on a button, which generates touch events as well as a click event. I verified the test fails in current m-c but passes with the backout applied.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backout the last 4 csets from bug 1288760

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8792943 [details] [diff] [review]
Part 1 - Update a test to catch the error

Review of attachment 8792943 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/test/mochitest/helper_tap.html
@@ +18,5 @@
>  }
>  
>  function clicked(e) {
>    is(e.target, document.getElementById('b'), "Clicked on button, yay! (at " + e.clientX + "," + e.clientY + ")");
> +  is(e.clientX, e.screenX, "ClientX and ScreenX on the event are the same");

This seems odd. One is a coordinate within the window's client area, the other is a global screen coordinate. So why should they be the same? That would presumably be expected only if the window is in a full-screen state ... and even then, only if the screen has its origin at (0,0), which is not necessarily true in the general case (though it would be on our single-display-only test systems).

Comment 26

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> This seems odd. One is a coordinate within the window's client area, the
> other is a global screen coordinate. So why should they be the same? That
> would presumably be expected only if the window is in a full-screen state
> ... and even then, only if the screen has its origin at (0,0), which is not
> necessarily true in the general case (though it would be on our
> single-display-only test systems).

Yeah that's a good point. I was thinking of Fennec where this is always true, at least in automation. Well, never mind the test then - can we land the backout?

Comment 27

[Jonathan Kew [:jfkthame]]

Before we go ahead with the backout (though that may well be the best thing for now), I'd like to understand a bit better what's going on here, and in particular, what has currently broken as a result of this. I just installed both FF49 and Nightly52 on a Nexus tablet, and while I can see that the behavior has changed, it's not clear to me whether it was ever correct. For one thing, it doesn't seem to account for panning at all. As such, is this actually being used in any significant way at this point?

Comment 28

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> For
> one thing, it doesn't seem to account for panning at all.

It's not clear to me what you mean. Do you expect clientX/screenX to change after panning? I wouldn't expect them to, since they are relative to the window's client area or the screen area and are unaffected by panning.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> (In reply to Jonathan Kew (:jfkthame) from comment #27)
> > For
> > one thing, it doesn't seem to account for panning at all.
> 
> It's not clear to me what you mean. Do you expect clientX/screenX to change
> after panning? I wouldn't expect them to, since they are relative to the
> window's client area or the screen area and are unaffected by panning.

It's entirely possible that I don't understand how things ought to work here.

Suppose I have a page, zoomed in such that only the top-left 1/4 of it is visible.

I touch the middle of my screen, and get a touch event with clientX = 500 (say).

Now I pan across so that I'm viewing the top-right 1/4 of the page.

Again, I touch the middle of my screen. What is clientX for this touch event?

The underlying page hasn't changed; we haven't rescaled or reflowed it at all, just panned it sideways. I thought the client area was the area within which we laid out the page (even though not all of it is visible at one time), and wouldn't change when we pan; and therefore the event (at a constant physical screen position) would now correspond to a different clientX coordinate.

But what I'm actually seeing is that I get the same clientX for the same physical touch, even after the view of the page has been panned beneath me to that I'm touching a different element on the page.

So am I assuming the wrong thing there, and the client area really means only the currently-visible area? The clientX/Y coordinates of everything on the page will be constantly shifting as we pan and zoom?

(Excuse my ignorance... I haven't worked with these APIs.)

Comment 30

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #29)
> It's entirely possible that I don't understand how things ought to work here.
> 
> Suppose I have a page, zoomed in such that only the top-left 1/4 of it is
> visible.
> 
> I touch the middle of my screen, and get a touch event with clientX = 500
> (say).
> 
> Now I pan across so that I'm viewing the top-right 1/4 of the page.
> 
> Again, I touch the middle of my screen. What is clientX for this touch event?

It should be the same as before, clientX = 500

> The underlying page hasn't changed; we haven't rescaled or reflowed it at
> all, just panned it sideways. I thought the client area was the area within
> which we laid out the page (even though not all of it is visible at one
> time), and wouldn't change when we pan; and therefore the event (at a
> constant physical screen position) would now correspond to a different
> clientX coordinate.

Not quite. You can think of the client area as the area of the window which *displays* the part of the page. Therefore the client area itself should always be fully visible (unless the browser window is partly offscreen), and panning the page around moves it inside the client area. The clientX coordinate on the event is the coordinate of the event relative to the top-left of the client area. In a sense the client area is very similar to the screen area, it just has a different origin. The client area (relative to the screen) doesn't move when the content is panned, but it might move if, for example, an extra chrome toolbar is added and the content is shifted down.

> But what I'm actually seeing is that I get the same clientX for the same
> physical touch, even after the view of the page has been panned beneath me
> to that I'm touching a different element on the page.

Yup, that's what I would expect. The pageX and pageY properties on the event are the ones that are relative to the top-left of the document, and would change as the document is panned around, given the event is in the same place on the screen.

> So am I assuming the wrong thing there, and the client area really means
> only the currently-visible area? The clientX/Y coordinates of everything on
> the page will be constantly shifting as we pan and zoom?

Yes. You can see this on desktop as well - if you use the web console on desktop, see how document.body.getBoundingClientRect() shifts as you scroll.

Comment 31

[Jonathan Kew [:jfkthame]]

OK, thanks - it helps to know what to expect here, I was thinking incorrectly about the relationship to pan&zoom.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

[Tracking Requested - why for this release]: Regression in 51, will likely break things

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8792945 [details] [diff] [review]
Backout the last 4 csets from bug 1288760

OK, let's do this for now until we have a better fix.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75abc730f820
Back out 4 csets from bug 1288760 for regressing event coordinate reporting. r=jfkthame

Comment 35

[Kartikaya Gupta (email:[e-mail])]

I filed bug 1306309 to get these patches relanded. I'll also request uplift of this backout to 51 once it's merged to m-c.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75abc730f820

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8792945 [details] [diff] [review]
Backout the last 4 csets from bug 1288760

Approval Request Comment
[Feature/regressing bug #]: bug 1288760
[User impact if declined]: websites that rely on screenX/screenY of touch events may not work properly
[Describe test coverage new/current, TreeHerder]: not much automated test coverage unfortunately
[Risks and why]: pretty low risk, this is a backout of the regressing patches.
[String/UUID change made/needed]: none

Comment 38

[Gerry Chang [:gchang]]

Comment on attachment 8792945 [details] [diff] [review]
Backout the last 4 csets from bug 1288760

Fix a regression related to touch events. Take it in 51 aurora.

Comment 39

[Gerry Chang [:gchang]]

Track 51+ as regression.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba4c1706898b