1272429 - Twitter Tweet detail view does not scroll using trackpad scroll gesture

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This is a regression from APZ bug 1270955.

STR:
1. Load https://twitter.com/letsencrypt/status/719918777542967297
2. Scroll the page by mousing over the window scroll bar (i.e. not over the web page) and using OS X's trackpad scroll gesture.
3. The page will scroll.
4. Try to scroll the page by mousing over the web page and using OS X's trackpad scroll gesture.

RESULT:
The page does not scroll!

Similar Twitter APZ bug 1163190 was reported last year, but resolved WFM.

Comment 1

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

I'm seeing the opposite behaviour - when I mouse over the scrollbar area I *can* scroll and while I mouse over the web content I can't scroll.

Regardless, there's a bug here, I'll look into it.

Comment 2

[Marcia Knous [:marcia]]

Tracking this regression for 49.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> I'm seeing the opposite behaviour - when I mouse over the scrollbar area I
> *can* scroll and while I mouse over the web content I can't scroll.

That is the same behavior I described. :)

Comment 4

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

Sorry, I misread the STR. Nonetheless, it's a relatively simple fix. My code to search for fixed-pos ancestors ignored scrollable elements that were between the hit layer and the fixed-pos ancestor. Those scrollable elements should take priority over the fixed-pos root.

Comment 5

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

Attachment: MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

Review commit: https://reviewboard.mozilla.org/r/52311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52311/

Comment 6

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

I considered renaming the GetNearestAncestorFixedPosTargetWithSameLayersId function to reflect that it now stops at scrollable ancestors, but I couldn't find a succinct way to express it. Naming suggestions welcome!

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

https://reviewboard.mozilla.org/r/52311/#review49207

::: gfx/layers/apz/src/HitTestingTreeNode.cpp:142
(Diff revision 1)
>  HitTestingTreeNode::GetNearestAncestorFixedPosTargetWithSameLayersId() const
>  {
>    for (const HitTestingTreeNode* n = this;
>         n && n->mLayersId == mLayersId;
>         n = n->GetParent()) {
> +    if (n->GetApzc()) {

Can we unify this function and GetNearestContainingApzcWithSameLayersId() into a function that does a single loop up the hit testing tree, and returns when it encounters a node with an APZC or a fixed-position node?

Comment 8

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

Good thought, yes we can :)

Comment 9

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

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52311/diff/1-2/

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

https://reviewboard.mozilla.org/r/52311/#review49223

::: gfx/layers/apz/src/APZCTreeManager.cpp:1780
(Diff revision 2)
>            }
>          }
>        }
>  
> -      AsyncPanZoomController* result = nullptr;
> -
> +      FrameMetrics::ViewID fpTarget = FrameMetrics::NULL_SCROLL_ID;
> +      AsyncPanZoomController* result = resultNode->GetNearestContainingApzcOrFixedPosTargetWithSameLayersId(&fpTarget);

I'd go a step further and:

  - Move the GetNearest...() function into APZCTM.

  - Call GetTargetNode() in the fixed case inside
    the function, so it can always return an
    APZC\* rather than doing this outparam thing.
    
  - Do the APZCTM_LOG messages inside the function.

We can then name the function something simpler, like GetTargetApzcForNode().

Comment 11

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

(In reply to Botond Ballo [:botond] from comment #10)
> I'd go a step further and:
> 
>   - Move the GetNearest...() function into APZCTM.
> 
>   - Call GetTargetNode() in the fixed case inside
>     the function, so it can always return an
>     APZC\* rather than doing this outparam thing.
>     
>   - Do the APZCTM_LOG messages inside the function.
> 
> We can then name the function something simpler, like GetTargetApzcForNode().

I don't feel too strongly about this, but I'm slightly opposed because it requires exposing "dumb" getters like GetLayersId() and GetFixedPosTarget() on the HitTestingTreeNode. I mean "dumb" in the sense that they are devoid of logic and turn HitTestingTreeNode into more of a "data class" like FrameMetrics/ScrollMetadata is, while putting the logic into the already-complex APZCTreeManager class.

That being said I'd be the first to agree we don't do this well in most of our codebase, or even in other places in these files (for example the aOutHitScrollbar thing just above the code in question...). Anyway I'll get to it later tonight unless you change your mind before then.

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> while putting the logic into the already-complex APZCTreeManager class.

Parts of the logic for selecting an APZC based on a node are already in APZCTreeManager. This way we'd keep the logic in one place.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

https://reviewboard.mozilla.org/r/52311/#review49253

Basically, the reason I don't love this is that the awkward name and signature of HitTestingTreeNode::GetNearestContainingApzcOrFixedPosTargetWithSameLayersId() suggest that it's not a clean interface, which suggests it may not be a win to have an interface cutting across this place in the logic.

That said, I don't feel sufficiently strongly about it to withhold r+, either, so I'll leave it up to you :)

Comment 14

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

(In reply to Botond Ballo [:botond] from comment #13)
> Basically, the reason I don't love this is that the awkward name and
> signature of
> HitTestingTreeNode::
> GetNearestContainingApzcOrFixedPosTargetWithSameLayersId() suggest that it's
> not a clean interface, which suggests it may not be a win to have an
> interface cutting across this place in the logic.

This is also true. I tried a couple of other things and in the end I found your suggestion in comment 10 did end up the cleanest, so I went with that. The try push I had triggered from the mozreview showed a failure on Linux which I repro'd and fixed locally (basically, I can't do "yield waitForAllPaints(driveTest)" because waitForAllPaints might invoke driveTest synchronously, which results in a JavaScript error from the generator). New try push to verify, will land if that's green:

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

Comment 15

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

^ That also had a test failure, on windows. I reproduced it - it happens because on windows, synthesizing a mouse move event to the same coordinates it already was it doesn't generate a mouse move event. So I had to skip that mousemove in the test and then it passed. Will land shortly.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2954496a6510

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2954496a6510

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

If the patch from bug 1270955 still needs to uplift to aurora 48, then we should include this fix also.

Comment 20

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

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

Approval Request Comment
[Feature/regressing bug #]: bug 1270955
[User impact if declined]: scrollable things *inside* fixed-position items are not scrollable, which is pretty bad on sites like twitter
[Describe test coverage new/current, TreeHerder]: patch includes a test. been on m-c for almost a week now
[Risks and why]: relatively low risk, i would expect any regressions to be caught pretty quickly.
[String/UUID change made/needed]: none

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8751923 [details]
MozReview Request: Bug 1272429 - When hit-testing layers nested inside scrollable content inside fixed-pos items, make sure to hit the scrollable layers. r?botond

Needed for bug 1270955

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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