Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 9 years ago

STR:

- load a page like https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad740209e8b6&group_state=expanded in desktop mode
- zoom in a little bit (so that the line of "B" build jobs is near the right edge of your screen)
- pan over to the rightmost edge of the page
- tap on one of the jobs near the right edge of the screen

Actual:
One of the jobs near the left gets selected instead. It looks like the job that would be under if your finger if you hadn't scrolled to the right.

I'm pretty sure that we have a transient async transform in this scenario and that's not getting unapplied properly. Most likely this will also be reproducible on pages that are overflow: hidden but zoomable (which might be the case on treeherder).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 1 • 9 years ago

Oh, "transient" may be the wrong term here. I'm not sure if we gave this one a name, I think it is part of the callback transform in APZCCallbackHelper.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 2 • 9 years ago

Interestingly I'm not seeing this on https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b4d99bdb1a&group_state=expanded which I would expect to behave the same.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 3 • 9 years ago

... and it's happening there also now. Maybe the STR need to include switching to a different tab and back, or something like that.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 4 • 9 years ago

I saw weirdness with this on iOS when the async callback was being used, so there likely is some weird bug here.

	Randall Barker [:rbarker] Assignee
	Comment 5 • 9 years ago

This page reproduces the issue in both Fennec and FxOS.

	Randall Barker [:rbarker] Assignee
	Comment 6 • 9 years ago

Hack attempt to fix issue.

	Randall Barker [:rbarker] Assignee
	Comment 8 • 9 years ago

rebased

	Randall Barker [:rbarker] Assignee
	Comment 9 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 10 • 9 years ago

Comment on attachment 8705779 [details] [diff] [review]
0001-Bug-1229752-When-C-APZ-scroll-offset-diverges-from-the-Gecko-scroll-offset-it-is-not-being-applied-properly-to-the-event-position-16010811-f8630e9.patch

I don't really understand the ApplyCallbackTransform change, r? => botond.

The change to PrepareForSetTargetAPZCNotification makes sense to me (do the same thing PresShell::HandleEvent does), but I don't know the expectations of PrepareForSetTargetAPZCNotification, so I'll ask botond to look as well.



>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>+#include "nsLayoutUtils.h"

It's already included.

>-nsIDocument*
>-PresShell::GetTouchEventTargetDocument()

I think you also need to remove the declaration of GetTouchEventTargetDocument from nsPresShell.h

	Randall Barker [:rbarker] Assignee
	Comment 11 • 9 years ago

Carry forward r+ from :tn

	Botond Ballo [:botond]
	Comment 12 • 9 years ago

Comment on attachment 8705848 [details] [diff] [review]
0001-Bug-1229752-When-C-APZ-scroll-offset-diverges-from-the-Gecko-scroll-offset-it-is-not-being-applied-properly-to-the-event-position-16010814-c80e5c9.patch

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

There are several things I don't understand about this patch:

  - What is the relationship between the ApplyCallbackTransform changes and 
    the PrepareForSetTargetAPZCNotification changes? They seem like independent
    changes (which should probably go into different patches) to me, but
    perhaps there's a connection I'm missing.

  - Why do we need to re-target the event in PrepareForSetTargetAPZCNotification?
    There was some IRC discussion about the event's coordinates beings outside
    of the RD's bounds, but I don't see why that should be possible if the
    coordinates don't have the resolution unapplied yet.

  - Why do we need to multiply the callback transforms by the RCD's resolution
    in ApplyCallbackTransform? Ignoring for a moment that we're going from
    applying a single callback transform to applying them in a loop (this is the
    one part of the patch I _do_ understand), that one transform that we were
    applying previously will now be applied with the resolution multiplied to
    its value. Is this an intentional change?

  - (Much more mundanely: ) Why do we need to move GetTouchEventTargetDocument
    from nsPresShell to nsLayoutUtils? We're still calling it on an nsPresShell
    at the new call site.

I plan to do a Fennec build and play around with the test case to try to get a better understanding of these things. In the meantime, if anyone would like to post a summary of the changes that helps me understand any of the above, that would be very helpful.

	Randall Barker [:rbarker] Assignee
	Comment 13 • 9 years ago

version pushed to inbound

	Randall Barker [:rbarker] Assignee
	Comment 14 • 9 years ago

Uploaded patch to wrong bug.

	Botond Ballo [:botond]
	Comment 15 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #12)
>   - Why do we need to multiply the callback transforms by the RCD's resolution
>     in ApplyCallbackTransform? Ignoring for a moment that we're going from
>     applying a single callback transform to applying them in a loop (this is the
>     one part of the patch I _do_ understand), that one transform that we were
>     applying previously will now be applied with the resolution multiplied to
>     its value. Is this an intentional change?

I now understand this. Posting what I wrote on IRC:

[13:52] <botond> rbarker: ApplyCallbackTransform has two functions: to unapply the RD resolution (if there is one), and to apply the deltas
[13:53] <botond> rbarker: on Fennec, the RD resolution is 1, so the first part is a no-op
[13:54] <botond> rbarker: the point coming into ApplyCallbackTransform has the RCD resolution applied, and the point going out is expected to have the RCD resolution applied, too (because code later, like in GetEventCoordinatesRelativeTo will unapply it)
[13:54] <botond> rbarker: now, the deltas are stored in a coordinate space that has any containing resolution unapplied
[13:54] <botond> rbarker: that is, for scroll frames inside the RCD (with the RCD-RSF being considered inside the RCD), they have the RCD resolution unapplied
[13:55] <botond> rbarker: since ApplyCallbackTransform is adding the delta to a point that has the RCD resolution applied, it needs to apply the RCD resolution to the delta as well
[13:56] <botond> rbarker: so, i'm now convinced that this part of the patch is correct for Fennec
[13:57] <botond> rbarker: (modulo the possibility of there being scroll frames outside the RCD, which shouldn't have the RCD resolution applied to their delta, but i don't think those exist in practice)
[13:57] <botond> rbarker: however, the patch will break FxOS
[13:58] <botond> rbarker: because there, the RCD resolution is the same as the RD resolution, and so we're applying the RD resolution to the deltas being added to a point that has the RD resolution unapplied

Randall and I then both came up with the workaround of using the "cumulative non-root scale resolution" (which is basically "the RCD resolution if the RCD != RD, otherwise 1") instead of the RCD resolution directly.

	Botond Ballo [:botond]
	Comment 16 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #12)
>   - What is the relationship between the ApplyCallbackTransform changes and 
>     the PrepareForSetTargetAPZCNotification changes? They seem like independent
>     changes (which should probably go into different patches) to me, but
>     perhaps there's a connection I'm missing.

There is indeed a connection I was missing, as explained below.

>   - Why do we need to re-target the event in PrepareForSetTargetAPZCNotification?
>     There was some IRC discussion about the event's coordinates beings outside
>     of the RD's bounds, but I don't see why that should be possible if the
>     coordinates don't have the resolution unapplied yet.

I now understand this as well:

[14:26] <botond> rbarker: it's an artefact of there being a large delta between the gecko and apz scroll offsets
[14:27] <botond> rbarker: say we have a screen of height 200 px, and an overflow:hidden document that fills that screen, and we zoom in by 2x
[14:28] <botond> rbarker: we allow scrolling within the original bounds, but to gecko we pretend we haven't scrolled
[14:29] <botond> rbarker: so say we scroll down by 100 px. the gecko scroll offset remains 0, but the apz scroll offset is 100 px
[14:29] <botond> rbarker: now, suppose the user taps at y=150px relative to the top of the screen
[14:29] <botond> rbarker: becuase we've visually scrolled by 100 px, we want that to hit whatever content is at y=250px relative to the top of the document
[14:30] <botond> rbarker: since gecko thinks our scroll offset is 0 px, we therefore need to pretend to gecko that y=250px was touched
[14:30] <botond> rbarker: however, we do this adjustment at a point in time where that value hasn't gone through gecko's _root document_ hit testing yet
[14:30] <botond> rbarker: when it does, gecko compares the 250px to the root document height (= screen height) of 200 px, and says "out of bounds"
[14:31] <botond> rbarker: so it's the delta that's getting us out of bounds, not something to do with the resolution
[14:32] <botond> rbarker: (for without the 100 px adjustment, the original y=150px value would be within the bounds of the 200px screen, regardless of the zoom level)

	Randall Barker [:rbarker] Assignee
	Comment 17 • 9 years ago

Carry forward r+ from :tnikkel

	Botond Ballo [:botond]
	Comment 18 • 9 years ago

Comment on attachment 8706535 [details] [diff] [review]
0001-Bug-1229752-When-C-APZ-scroll-offset-diverges-from-the-Gecko-scroll-offset-it-is-not-being-applied-properly-to-the-event-position-16011111-4770432.patch

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

r+ with comments addressed

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +478,5 @@
> +    // resolution to the scroll offset before adding to the point.
> +    // GetCumulativeNonRootScaleResolution will ensure we get the resolution
> +    // for Fennec while the value will remain 1.0 for other platforms where
> +    // the Root Document and the Root Content Document are the same.
> +    float rootContentDocumentResolution = 1.0f;

I'd call this variable 'nonRootResolution', and suggest revising the comment as follows:

   // This represents any resolution on the Root Content Document (RCD) 
   // that's not on the Root Document (RD). That is, on platforms where 
   // RCD == RD, it's 1, and on platforms where RCD != RD, it's the RCD
   // resolution. 'input' has this resolution applied, but the scroll
   // deltas retrieved below do not, so we need to apply them to the
   // deltas before adding the deltas to 'input'. (Technically, deltas
   // from scroll frames outside the RCD would already have this 
   // resolution applied, but we don't have such scroll frames in 
   // practice.)

@@ +499,5 @@
> +    // overflow: hidden. Technically pan and zooming are disabled, however on
> +    // mobile it is permitted. This is accomplished by allowing the APZ scroll to
> +    // get out of sync with the gecko scroll. In order for the event position
> +    // to be in the correct location for gecko, the APZ scroll offset must be
> +    // added to the position.

There is already a comment in ScrollFrameTo() (above) that explains the treatment of zoomed overflow:hidden scroll frames; there's no need to restate that here. However, the pre-existing "XXX" comment should be revised, as this patch makes it out of date.

Suggested revised comment:

  // XXX: Walk up the frame tree from the frame of this content element
  // to the root of the frame tree, and apply any apzCallbackTransform
  // found on the way. This is only approximately correct, as it does
  // not take into CSS transforms, nor differences in structure between
  // the frame tree (which determines the transforms we're applying) 
  // and the layer tree (which determines the transforms the *want* to
  // apply).

@@ +658,5 @@
>  {
> +#if defined(MOZ_ANDROID_APZ)
> +  // Re-target so that the hit test is performed relative to the frame for the
> +  // Root Content Document instead of the Root Document which are different in
> +  // Android.

Please add:

 // See bug 1229752 comment 16 for an explanation of why this is necessary.

	Randall Barker [:rbarker] Assignee
	Comment 19 • 9 years ago

Updated to address comments.

	Botond Ballo [:botond]
	Comment 20 • 9 years ago

Comment on attachment 8706611 [details] [diff] [review]
0001-Bug-1229752-When-C-APZ-scroll-offset-diverges-from-the-Gecko-scroll-offset-it-is-not-being-applied-properly-to-the-event-position-r-botond-tnikkel-16011114-035fa40.patch

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

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +488,5 @@
>      // Now apply the callback-transform.
> +    // XXX: Walk up the frame tree from the frame of this content element
> +    // to the root of the frame tree, and apply any apzCallbackTransform
> +    // found on the way. This is only approximately correct, as it does
> +    // not take into CSS transforms, nor differences in structure between

"take into account" (my typo, sorry)

@@ +490,5 @@
> +    // to the root of the frame tree, and apply any apzCallbackTransform
> +    // found on the way. This is only approximately correct, as it does
> +    // not take into CSS transforms, nor differences in structure between
> +    // the frame tree (which determines the transforms we're applying)
> +    // and the layer tree (which determines the transforms they *want* to

s/they/we (also my typo)

	Randall Barker [:rbarker] Assignee
	Comment 21 • 9 years ago

Fix some typos.

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a49de3aacab9

	Randall Barker [:rbarker] Assignee
	Comment 23 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a49de3aacab9