Closed Bug 1272429 Opened 3 years ago Closed 3 years ago

Twitter Tweet detail view does not scroll using trackpad scroll gesture

Categories

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

49 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- fixed
firefox49 + fixed

People

(Reporter: cpeterson, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(1 file)

[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.
Flags: needinfo?(bugmail.mozilla)
Whiteboard: gfx-noted
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.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
Version: unspecified → 49 Branch
Tracking this regression for 49.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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. :)
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.
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!
Attachment #8751923 - Flags: review?(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?
Good thought, yes we can :)
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/
Attachment #8751923 - Flags: review?(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().
Attachment #8751923 - Flags: review?(botond)
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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 :)
Attachment #8751923 - Flags: review+
(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
tracking-e10s: --- → +
Priority: -- → P1
^ 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.
Duplicate of this bug: 1272829
https://hg.mozilla.org/mozilla-central/rev/2954496a6510
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
If the patch from bug 1270955 still needs to uplift to aurora 48, then we should include this fix also.
Flags: needinfo?(bugmail.mozilla)
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
Flags: needinfo?(bugmail.mozilla)
Attachment #8751923 - Flags: approval-mozilla-aurora?
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
Attachment #8751923 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.