947337 - Touch Events coordinates returns strange values

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I was hoping that bug 940706 fix that but it does not. In the contacts app there is a list of letters on the right side that are used as shortcuts. With APZ it does not work all the time. 

Looking at the touchmove events it seems event.touches[0].[pageY/clientY] returns strange coordinate sometimes like the copy-paste above that prevents the shortcut bar from working correctly.

I/GeckoDump( 7897): Shortcuts (touchmove): 226
I/GeckoDump( 7897): Shortcuts (touchmove): -12176
I/GeckoDump( 7897): Shortcuts (touchmove): 253
I/GeckoDump( 7897): Shortcuts (touchmove): -18478
I/GeckoDump( 7897): Shortcuts (touchmove): 271
I/GeckoDump( 7897): Shortcuts (touchmove): -24989
I/GeckoDump( 7897): Shortcuts (touchmove): -24989
I/GeckoDump( 7897): Shortcuts (touchmove): 288
I/GeckoDump( 7897): Shortcuts (touchmove): -26160
I/GeckoDump( 7897): Shortcuts (touchmove): 319
I/GeckoDump( 7897): Shortcuts (touchmove): -27722
I/GeckoDump( 7897): Shortcuts (touchmove): 331
I/GeckoDump( 7897): Shortcuts (touchmove): -32913
I/GeckoDump( 7897): Shortcuts (touchmove): 345
I/GeckoDump( 7897): Shortcuts (touchend): -36844

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Steps to reproduce:
 - populate the device with some data if needed. |cd $GAIA; make reference-workload-medium| with the device plugged in.
 - Open the contacts app on the device
 - Wait a bit for contacts to be fully populate.
 - Try to use the shortcut bar on the right side

Expected result:
 - The view navigate to the appropriate list of contacts

Actual result:
 - Sometimes it work, most of the time it does not.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

I wonder if this is the cause of the problem captured in the movie attachment to bug 942854

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #2)
> I wonder if this is the cause of the problem captured in the movie
> attachment to bug 942854

I dumped a few events there and there is sometimes strange values with negative pageY. Or even sometimes the pageY values are bigger than the size of the screen. Not sure if this is the root cause of the bug there, but in all cases this stuff probably does not help.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Wonder if we're messing up the coordinate  systems somehow?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Wonder if we're messing up the coordinate  systems somehow?

It seems like the coordinates are messed up by the call to http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#776 

Before calling that they are good, and after they are not anymore.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Apparently the transformation matrix passed to http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#433 contains messed up values for |y|.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #6)
> Apparently the transformation matrix passed to
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#433 contains messed up values for |y|.

Obviously without the transformation matrix a few things are broken but I also can't reproduce bug 942752 and bug 943882. Without it, calendar is also not jumping all over the place as seen in the video from bug 942854, but the rendering issue is still here.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Do you still see this problem?  Because the jumpy behavior in 942854 seems to be gone for me.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #10)
> Do you still see this problem?  Because the jumpy behavior in 942854 seems
> to be gone for me.

I rebuilt today and I still see the strange coordinates I mentioned.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Seems like http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#986 is the matrix that contains the weird values.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #12)
> Seems like
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#986 is the matrix that contains the weird values.

I wonder if the 'weird' values are not because of APZCTreeManager trying to get the scroll offset coordinate of the contacts list while panning the contacts shortcuts.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #13)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #12)
> > Seems like
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> > APZCTreeManager.cpp#986 is the matrix that contains the weird values.
> 
> I wonder if the 'weird' values are not because of APZCTreeManager trying to
> get the scroll offset coordinate of the contacts list while panning the
> contacts shortcuts.

Seems like it happens when the the current scroll offset is out of sync with the last paint scroll offset.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug947337

This is not clear to me why we need to care about paint offsets while dispatching events that are normally tied to the window.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug909877.patch

Is it better ?

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug947337

Oups. I attached a patch for an other bug, sorry for the noise.

Comment 18

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

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #1)
> Steps to reproduce:
>  - populate the device with some data if needed. |cd $GAIA; make
> reference-workload-medium| with the device plugged in.
>  - Open the contacts app on the device
>  - Wait a bit for contacts to be fully populate.
>  - Try to use the shortcut bar on the right side
> 
> Expected result:
>  - The view navigate to the appropriate list of contacts
> 
> Actual result:
>  - Sometimes it work, most of the time it does not.

Using these STR most of the time I get the expected result. Occasionally after scrolling around it does get into a state where clicking on the letters on the right doesn't work, but it doesn't happen that often for me.

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #3)
> I dumped a few events there and there is sometimes strange values with
> negative pageY. Or even sometimes the pageY values are bigger than the size
> of the screen. Not sure if this is the root cause of the bug there, but in
> all cases this stuff probably does not help.

Where are you dumping the pageY that you see this value?

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #1)
> > Steps to reproduce:
> >  - populate the device with some data if needed. |cd $GAIA; make
> > reference-workload-medium| with the device plugged in.
> >  - Open the contacts app on the device
> >  - Wait a bit for contacts to be fully populate.
> >  - Try to use the shortcut bar on the right side
> > 
> > Expected result:
> >  - The view navigate to the appropriate list of contacts
> > 
> > Actual result:
> >  - Sometimes it work, most of the time it does not.
> 
> Using these STR most of the time I get the expected result. Occasionally
> after scrolling around it does get into a state where clicking on the
> letters on the right doesn't work, but it doesn't happen that often for me.
> 
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #3)
> > I dumped a few events there and there is sometimes strange values with
> > negative pageY. Or even sometimes the pageY values are bigger than the size
> > of the screen. Not sure if this is the root cause of the bug there, but in
> > all cases this stuff probably does not help.
> 
> Where are you dumping the pageY that you see this value?

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_shortcuts.js#L128

Comment 20

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

Attachment: Patch to make bad pageY values happen more reliably

Ok, so I can force the bad pageY values to happen more reliably by applying the attached patch and dragging around in the contacts alphabet shortcuts. The reason they happen is that we receive input events in the parent process between the time that the child calls UpdateScrollOffset and the time that the child finishes painting and sends us a layer transaction. During this time we have a giant async scroll offset ("giant" depending on how far the scroll offset jumped) which is technically correct to get the right visual behaviour. For input transformation though it is not correct. I haven't tested Vivien's patch but it looks like it would fix this, but also break other scenarios.

I will think about what the right fix here is.

Also just to note, I don't think the bad pageY values here would affect _tapping_ on the shortcut bar, since in that case there are no touchmove events to speak of. It will only affect _dragging_ because that is when we have a stream of input events getting interleaved with the scroll offset jumping.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> Created attachment 8345373 [details] [diff] [review]
> Patch to make bad pageY values happen more reliably
> 
> Ok, so I can force the bad pageY values to happen more reliably by applying
> the attached patch and dragging around in the contacts alphabet shortcuts.
> The reason they happen is that we receive input events in the parent process
> between the time that the child calls UpdateScrollOffset and the time that
> the child finishes painting and sends us a layer transaction. During this
> time we have a giant async scroll offset ("giant" depending on how far the
> scroll offset jumped) which is technically correct to get the right visual
> behaviour. For input transformation though it is not correct. I haven't
> tested Vivien's patch but it looks like it would fix this, but also break
> other scenarios.
> 

Just curious, what could it break?

> I will think about what the right fix here is.
> 
> Also just to note, I don't think the bad pageY values here would affect
> _tapping_ on the shortcut bar, since in that case there are no touchmove
> events to speak of. It will only affect _dragging_ because that is when we
> have a stream of input events getting interleaved with the scroll offset
> jumping.

I agree that tapping will likely not be impacted.

Comment 22

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

Comment on attachment 8345236 [details] [diff] [review]
bug947337

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

Minusing because I believe this will regress other scenarios. I discussed this with Botond and Markus and have another solution, coming up.

Comment 23

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

Attachment: Only accept new scroll offsets in layers updates

Vivien, can you check if this fixes the pageY problem for you? I'm not yet convinced the pageY problem is the root cause of all the contacts shortcut scrolling problems so those may still exist.

Eventually we can move this new flag into the FrameMetrics and then we don't need to keep it in APZC at all.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8345429 [details] [diff] [review]
Only accept new scroll offsets in layers updates

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

Nice and simple, I like it!

If this will be the permanent solution, we should consider renaming UpdateScrollOffset to something else, as it no longer updates the scroll offset. Maybe ScrollOffsetChanged?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +586,5 @@
>    // function can be used, or the monitor can be held and then |mState| updated.
>    ReentrantMonitor mMonitor;
>  
>  private:
> +  // Flag to capture whether or not UpdateScrollOffset was called; if it was

Let's say "Flag to capture whether or not UpdateScrollOffset was called since the last layers update".

Comment 25

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> Just curious, what could it break?

I believe it would affect input events while a legitimate async scroll transform is in place. For example, say you do a pan, and then before gecko has a chance to repaint, you click on something. The touch events from the click will have coordinates as if the pan never happened, and so will get targetted to the wrong element. Note that this will only happen *sometimes* because in general the RequestContentRepaint events from the APZC panning will be processed before the touch events from the click, but there are cases where there is still an async transform that hasn't been processed yet.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8345429 [details] [diff] [review]
Only accept new scroll offsets in layers updates

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

Seems to work for me \o/

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8345429 [details] [diff] [review]
Only accept new scroll offsets in layers updates

Sorry I tried with my patch on top of it. I still see the wrong pageY with it.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #27)
> Comment on attachment 8345429 [details] [diff] [review]
> Only accept new scroll offsets in layers updates
> 
> Sorry I tried with my patch on top of it. I still see the wrong pageY with
> it.

I also see bug 942752 with it.

Comment 29

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

I saw a wrong pageY value come out once with my patch after hammering on it for a while. Based on the log I suspect it is possible for the layers update with the new scroll offset to arrive *before* the call to UpdateScrollOffset, and that is when this occurs. I'm not entirely sure what code path triggers that but I will continue looking.

Comment 30

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> If this will be the permanent solution, we should consider renaming
> UpdateScrollOffset to something else, as it no longer updates the scroll
> offset. Maybe ScrollOffsetChanged?

Not to mention removing the now-unused parameter of UpdateScrollOffset :).

Comment 31

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

Markus, do you know how hard it would be to add a flag on the FrameMetrics like we were discussing on IRC? I don't know if layout stores the kind of information that we need ("this layout was triggered by a content scrollTo") or if it's even possible. If it turns out to be simple to do though I think that would solve the problem I'm seeing now regardless of the cause.

Comment 32

[Markus Stange [:mstange]]

Attachment: Add mWasScrolledByContent flag to frame metrics and set it to false when scrolling via APZCCallbackHelper

Layout doesn't record the information yet, but something like this should do it. I haven't tested it yet, though.
This lets us specify an origin when calling ScrollToCSSPixelsApproximate in APZCCallbackHelper and stores it in an mOriginOfLastScroll field on the scroll frames. APZCCallbackHelper calls this with aOrigin == nsGkAtoms::apzc, and RecordFrameMetrics sets metrics.mWasScrolledByContent = (origin != nsGkAtoms::apzc).
I got rid of nsIDOMWindowUtils::ScrollToCSSPixelsApproximate so that we don't need to pipe aOrigin through it, but I don't know whether nsLayoutUtils::FindScrollableFrameFor finds the right frame when scrolling the root.

Kats, does this work for you?

Comment 33

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

Attachment: WIP - use mWasScrolledByContent flag

Thanks, Markus, that works beautifully! The attached WIP patch applies on top of it and uses the flag in APZC.

Vivien, can you try applying attachment 8345933 [details] [diff] [review] and this patch and see if that fixes the problem? It fixes it for me but I can only intermittently reproduce the problem to begin with so I can't confirm that it fixes it. If you find it fixes the problem I'll clean up the patches and get them ready for landing.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm rebuilding. I will keep you up as soon my build is finished.

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #34)
> I'm rebuilding. I will keep you up as soon my build is finished.

Hammering a bit on the contacts app I still see that (but this sounds harder than usual).

I/GeckoDump( 3152): Event: -43030
I/GeckoDump( 3152): Event: 296
I/GeckoDump( 3152): Event: 296
I/GeckoDump( 3152): Event: 43511
I/GeckoDump( 3152): Event: 287
I/GeckoDump( 3152): Event: -13821
I/GeckoDump( 3152): Event: 248
I/GeckoDump( 3152): Event: 14
I/GeckoDump( 3152): Event: 14
I/GeckoDump( 3152): Event: 124
I/GeckoDump( 3152): Event: 141
I/GeckoDump( 3152): Event: -32916
I/GeckoDump( 3152): Event: 176
I/GeckoDump( 3152): Event: 194
I/GeckoDump( 3152): Event: 4228
I/GeckoDump( 3152): Event: 183
I/GeckoDump( 3152): Event: 183
I/GeckoDump( 3152): Event: 172
I/GeckoDump( 3152): Event: 2341
I/GeckoDump( 3152): Event: 2341
I/GeckoDump( 3152): Event: 239
I/GeckoDump( 3152): Event: 234
I/GeckoDump( 3152): Event: 11022
I/GeckoDump( 3152): Event: 11022
I/GeckoDump( 3152): Event: 11022

I also can reproduce fairly easily the issue from bug 942752.

Sorry :/

Comment 36

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

*sigh*

I'll spin up a build for my hamachi to see if i can repro there more easily. if that doesn't work I'll try helix.

In the meantime I would still like to get this patch to kill UpdateScrollOffset in, so I'll spin off another bug for that.

Comment 37

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

Huzzah! I can repro on hamachi. Will debug.

Comment 38

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

I put cleaned up patches on bug 949132. With those patches I can't reproduce the bad pageY values while scrolling around in the contacts shortcuts. I do see something that looks like bug 942752 in that I see tap highlight show up and persist on random contacts as I'm panning around. I'm not yet convinced that is caused by bad pageY values.

Comment 39

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

Comment on attachment 8345933 [details] [diff] [review]
Add mWasScrolledByContent flag to frame metrics and set it to false when scrolling via APZCCallbackHelper

Obsoleted by bug 949132

Comment 40

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

Comment on attachment 8346027 [details] [diff] [review]
WIP - use mWasScrolledByContent flag

Ditto

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

Vivien, can you see this problem on hamachi or some other phone?

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #41)
> Vivien, can you see this problem on hamachi or some other phone?

I will try in a few hours.

Comment 43

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

Attachment: Additional logging

Vivien, when you get a chance, please reproduce the problem with this patch applied and attach the full logcat. Hopefully it will provide a clue as to why we're still getting bad transforms.

Comment 44

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

Vivien, I'm still unable to reproduce this problem on recent code. Can you apply the patch from comment 43 and repro?

Comment 45

[Botond Ballo [:botond]]

I see the negative coordinate values without the patches from bug 949132, but not with them. This is consistent with our belief that those patches should fix the problem. Duping; Vivien, feel free to undupe if you still see the negative values with those patches.

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

We can't just dupe, because this bug is 1.3+, and bug 949132 has been -'d. Either we keep them separate and have this bug land the solution to 1.3, or we 1.3+ bug 949132, until then I'll reopen.

Comment 47

[Milan Sreckovic [:milan] (needinfo for best results)]

After IRC conversation, will make bug 949132 1.3+ and dupe this one back to it.

Comment 48

[Etienne Segonzac (:etienne)]

Attachment: weird-highlight.MOV

Here's a video of a button outside the scrolling div being highlighted while scrolling. On a build from yesterday (so after bug 949132's landing).

Comment 49

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

Your thumb was pretty close to the button - to me it looks like the event fluffing code sent the touchdown to the button and so it got highlighted. If you think there's still an issue here please file a new bug with details. I'd rather not reopen this one because (a) I'm not convinced that this is caused by bad touch event coordinates from the APZC and (b) I'd rather start investigating this with a clean slate and not have people worry about the previous 50 comments on this bug.

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> Your thumb was pretty close to the button - to me it looks like the event
> fluffing code sent the touchdown to the button and so it got highlighted.

I reproduce this issue so many times in the past and so it is clear to me that it is not fluffling related. Also fluffing will likely not highlight this button while the finger is still on the list and the list is panning since it would have mean that the list was not the start of the gesture and then I assumme that panning will not have happened.

> If
> you think there's still an issue here please file a new bug with details.
> I'd rather not reopen this one because (a) I'm not convinced that this is
> caused by bad touch event coordinates from the APZC and (b) I'd rather start
> investigating this with a clean slate and not have people worry about the
> previous 50 comments on this bug.
> 

As I stated in this bug, the proposed patch makes the issue goes away for me. So I'm pretty convinced that it is the same root cause. So it unclear to me why we should start from a new bug.

Comment 51

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #50)
> I reproduce this issue so many times in the past and so it is clear to me
> that it is not fluffling related. Also fluffing will likely not highlight
> this button while the finger is still on the list and the list is panning
> since it would have mean that the list was not the start of the gesture and
> then I assumme that panning will not have happened.

If the finger starts on the list, panning will happen on the list. But if the touchdown is dispatched to the button because of the fluffing code then I would expect the button to be highlighted. But you could be right, I don't know exactly what happens there.

> As I stated in this bug, the proposed patch makes the issue goes away for
> me. So I'm pretty convinced that it is the same root cause. So it unclear to
> me why we should start from a new bug.

I don't believe your proposed patch is correct - there are many ways to fix any given bug and just because it fixes the issue you're seeing doesn't mean it won't introduce other regressions. We've spent a lot of time figuring out what the right transforms are supposed to be in that code and I'm pretty convinced that they're correct.

Seeing as I can't reproduce this issue and you haven't provided the information I asked for in in comment 43 I can't really do anything here. Unassigning from myself.

Comment 52

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #51)
> 
> I don't believe your proposed patch is correct - there are many ways to fix
> any given bug and just because it fixes the issue you're seeing doesn't mean
> it won't introduce other regressions. We've spent a lot of time figuring out
> what the right transforms are supposed to be in that code and I'm pretty
> convinced that they're correct.

I'm not arguing at all that my patch is correct. I'm just saying that the issue is still present and by having run the proposed patch during the break I have not encounter any issues - which does not prove that there is no issues though.

> 
> Seeing as I can't reproduce this issue and you haven't provided the
> information I asked for in in comment 43 I can't really do anything here.
> Unassigning from myself.

I was trying to rebuild yesterday to give you the infos but I encounter a build issue in the JS runtime :/

Comment 53

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: log.apzc.txt

Here is some logging.

I have also added a currentY: XXX value in the contacts app so you can see when strange values are returned.

Comment 54

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

Comment on attachment 8361153 [details]
log.apzc.txt

Thanks for the log. I think I understand what is going on now. The main problem is that when we calculate the transformToGecko in GetInputTransforms, we use the GetCurrentAsyncTransform which computes the async transform relative to mLastContentPaintMetrics. In fact what we want to use there is the async transform relative to mLastPaintRequestMetrics (or more precisely, the most recent metrics we requested a repaint before the transformed touch event gets back to TabChild - this may not be mLastPaintRequestMetrics because of the paint throttler). I'll see if I can figure out a good fix for this.

Comment 55

[Peter Dolanjski [:pdol]]

PM retriaged this and we believe it is a blocker.

Comment 56

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

Attachment: Part 1/3 - Refactoring to reduce an unnecessary codepath

The only functional change here is that ZoomToRect's call to repaint endZoomToMetrics is now subject to the deduplication code (which should be fine, I think).

Comment 57

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

Attachment: Part 2/3 - Track the last metrics actually dispatched to gecko

This part is needed because we might have called RequestContentRepaint, but the corresponding paint might not yet have been inserted into the message queue to gecko because of the paint throttler. If this were not the case then we would just be able to use mLastPaintRequestMetrics instead.

Comment 58

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

Attachment: Part 3/3 - The missing transform

Yay complicated transforms!

I haven't tested this code yet at all. I want to first figure out how to reliably reproduce the problem Vivien was seeing, which should be possibly by inserting a few sleep calls in the right places. My first attempt at doing that failed and I still couldn't reproduce it so I will try some more.

Comment 59

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

Attachment: Part 3/3 - The missing transform

I had that transform backwards. Fixed now. I was able to force the condition to repro and verify that with these patches it seems to behave better.

Vivien, can you try out these patches and see if it fixes the problem for you? I wrote these patches against 1.3 code but there are versions rebased to master at https://github.com/staktrace/gecko-dev/commits/geckotransform (top 3 commits).

Comment 60

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #59)
> Created attachment 8362693 [details] [diff] [review]
> Part 3/3 - The missing transform
> 
> I had that transform backwards. Fixed now. I was able to force the condition
> to repro and verify that with these patches it seems to behave better.
> 
> Vivien, can you try out these patches and see if it fixes the problem for
> you? I wrote these patches against 1.3 code but there are versions rebased
> to master at https://github.com/staktrace/gecko-dev/commits/geckotransform
> (top 3 commits).

I applied the patches on top of m-c, applied the usual dump for the contacts app but filtered so it only returns values < 0 and > window.innerHeight.
 
Both good and mitigate results:
 - It seems that the negative values or the values way too big are gone!
 - I still see sometimes some values between window.innerHeight < n < the screen height. I think that pageY is supposed to be relative to the document based on https://developer.mozilla.org/en-US/docs/Web/API/event.pageY
 - The highlight issue from https://bugzilla.mozilla.org/show_bug.cgi?id=947337#c48 is still here.

So it seems like the completely insane values are gone, which is a good stuff. I won't mind if you want to close this bug once the patches has landed and continue to investigate the 2 others issues into different bugs.

Comment 61

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #60)
>  - It seems that the negative values or the values way too big are gone!

Yay!

>  - I still see sometimes some values between window.innerHeight < n < the
> screen height. I think that pageY is supposed to be relative to the document
> based on https://developer.mozilla.org/en-US/docs/Web/API/event.pageY

Hm. pageY is relative to the document, yes. Which means any value 0 <= n < documentheight should be valid. This is what I see on desktop if I dump the pageY of mousedown events, for example. So I think pageY values greater than window.innerHeight are fine. I'm not entirely sure if the shortcut bar is in a separate document or is position:fixed or something so I'm not sure if the values coming out are right or not. But as long as they are in the same range as the non-APZ values I'm happy.

>  - The highlight issue from
> https://bugzilla.mozilla.org/show_bug.cgi?id=947337#c48 is still here.

Yeah I strongly suspect this is because the hardware is generating some wacky events (in bug 957188 comment 11 I reported seeing large variations in the events that the hardware sends us) and combining with the event fluffing code. But yeah, a follow-up bug for this would be good.

> So it seems like the completely insane values are gone, which is a good
> stuff. I won't mind if you want to close this bug once the patches has
> landed and continue to investigate the 2 others issues into different bugs.

Sounds good.

Comment 62

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

Attachment: Logging/sleep to catch the bad case when it happens

This is just a patch I had locally that makes it more likely to hit the bad scenario, and to detect when we do hit it. Putting it here in case it comes in handy in the future.

Comment 63

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #61)
> >  - I still see sometimes some values between window.innerHeight < n < the
> > screen height. I think that pageY is supposed to be relative to the document
> > based on https://developer.mozilla.org/en-US/docs/Web/API/event.pageY
> 
> Hm. pageY is relative to the document, yes. Which means any value 0 <= n <
> documentheight should be valid. This is what I see on desktop if I dump the
> pageY of mousedown events, for example. So I think pageY values greater than
> window.innerHeight are fine. I'm not entirely sure if the shortcut bar is in
> a separate document or is position:fixed or something so I'm not sure if the
> values coming out are right or not. But as long as they are in the same
> range as the non-APZ values I'm happy.
>

The shortcut bar is a fixed/absolute positioning fwiw.

Comment 64

[Botond Ballo [:botond]]

Comment on attachment 8362693 [details] [diff] [review]
Part 3/3 - The missing transform

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +866,4 @@
>          MT.Inverse()
>          LC.Inverse()
>          LN.Inverse()
>     This combined transformation is returned in the aTransformToApzcOut out-parameter.

nit: A bit further up in this comment (the bugzilla review system doesn't let me see it here), the statement "Layer L is the layer that corresponds to the returned APZC instance" dates back from the time that this comment was above GetTargetAPZC, which did in fact return an APZC instance. Let's update this to "Layer L is the layer that corresponds to |aApzc|".

@@ +871,5 @@
>     Next, if we want user inputs sent to gecko for event-dispatching, we will need to strip
>     out all of the async transforms that are involved in this chain. This is because async
>     transforms are stored only in the compositor and gecko does not account for them when
> +   doing display-list-based hit-testing for event dispatching.
> +   Furthermore, because this these input events are processed by Gecko in a FIFO queue that

s/this these/these

@@ +918,5 @@
>     Additionally, for space-saving purposes, each APZC instance stores its layer's individual
>     CSS transform and the accumulation of CSS transforms to its parent APZC. So the APZC for
>     layer L would store LC and (MC * NC * OC), and the layer P would store PC and (QC * RC).
>     The APZCs also obviously have LT, LN, PT, and PN, so all of the above transformation combinations
>     required can be generated.

It might be worthwhile to add a sentence saying how an APZC for layer X knows or calculates XD.

@@ +943,5 @@
>    gfx3DMatrix transientAsyncUntransform = nontransientAsyncTransform * asyncUntransform;
>  
>    // aTransformToApzcOut is initialized to OC.Inverse() * NC.Inverse() * MC.Inverse() * LC.Inverse() * LN.Inverse()
>    aTransformToApzcOut = ancestorUntransform * aApzc->GetCSSTransform().Inverse() * nontransientAsyncTransform.Inverse();
> +  // aTransformToGeckoOut is initialized to LT.Inverse() * LD * LC * MC * NC * OC

Let's add a comment pointing out that we haven't forgotten about MD, ND, and OD, they're just zero.

Comment 65

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

(In reply to Botond Ballo [:botond] from comment #64)
> Let's add a comment pointing out that we haven't forgotten about MD, ND, and
> OD, they're just zero.

There was already a comment to that effect at the end of the method but it was slightly out-of-date so I updated it to be more accurate. Addressed all the other comments as well and landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5becc07d9dac
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3095f92e1c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ade48b8b9d7

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/053124f7a0e1 for build test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=33354949&tree=Mozilla-Inbound on at least OSX and Linux (Windows hasn't finished yet).

Comment 67

[Preeti Raghunath(:Preeti)]

Kats,

Please provide next steps.

Comment 68

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

Fix tests.
Reland.

Comment 69

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

Attachment: Part 4/3 - Update tests

The seemingly-random destroy calls in other tests were needed as well, or the mock object got leaked. I believe this is a result of my rearranging how RequestContentRepaint worked and more specifically the change in when PostDelayedTask gets called.

The HitTesting2 test was failing because the way we set it up, it would have a nonempty LD transform which would cancel out the LA transform. So I modified the existing assertions to deal with those, and then extended the test to simulate having nonempty (but different) LD and LA transforms.

Comment 70

[Botond Ballo [:botond]]

Comment on attachment 8363426 [details] [diff] [review]
Part 4/3 - Update tests

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

It's fortunate that we could arrange for one paint request to be "dispatched" and another not without bringing additional machinery into the test :)

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +921,5 @@
> +  hit = GetTargetAPZC(manager, ScreenPoint(75, 75), transformToApzc, transformToGecko);
> +  EXPECT_EQ(apzcroot, hit.get());
> +  // transformToApzc doesn't unapply the root's own async transform
> +  EXPECT_EQ(gfxPoint(75, 75), transformToApzc.Transform(gfxPoint(75, 75)));
> +  // but transformToGecko does

Let's be a little more specific here: transformToGecko unapplies the full async transform of -100, and then reapplies the "D" transform of -50, leading to an overall adjustment of +50.

@@ +929,5 @@
> +  hit = GetTargetAPZC(manager, ScreenPoint(25, 25), transformToApzc, transformToGecko);
> +  EXPECT_EQ(apzcroot, hit.get());
> +  // transformToApzc doesn't unapply the root's own async transform
> +  EXPECT_EQ(gfxPoint(25, 25), transformToApzc.Transform(gfxPoint(25, 25)));
> +  // but transformToGecko does

Likewise here.

Comment 71

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

Updated comments and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/823702f98ef6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d85b756d350b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/db88c6a02346
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6586e4aa566b

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/823702f98ef6
https://hg.mozilla.org/mozilla-central/rev/d85b756d350b
https://hg.mozilla.org/mozilla-central/rev/db88c6a02346
https://hg.mozilla.org/mozilla-central/rev/6586e4aa566b

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 74

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

Rebased to aurora and landed:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f6b914730a9c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d7cd7c0cedd3
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/00a111a49239
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/31da64bcc970

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #74)

I had to back these out on Aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/bfdb71aa8953 due to APZC test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=33482134&tree=Mozilla-Aurora and https://tbpl.mozilla.org/php/getParsedLog.php?id=33482801&tree=Mozilla-Aurora

Comment 76

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

Bad rebase on my part; I added an extra apzc->Destroy() at the end of ApzcPan which was causing the problem. Verified locally that taking that out fixes it, but the trees are closed now so I'll reland in the morning.

Comment 77

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f8e37f7c4034
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bb7ca58bda30
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/188a34d73e40
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2f05620ff928