Closed
Bug 1272429
Opened 9 years ago
Closed 9 years ago
Twitter Tweet detail view does not scroll using trackpad scroll gesture
Categories
(Core :: Panning and Zooming, defect, P1)
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)
58 bytes,
text/x-review-board-request
|
botond
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
[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.
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Whiteboard: gfx-noted
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Version: unspecified → 49 Branch
Reporter | ||
Comment 3•9 years ago
|
||
(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. :)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52311/
Attachment #8751923 -
Flags: review?(botond)
Assignee | ||
Comment 6•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8751923 -
Flags: review?(botond)
Comment 7•9 years ago
|
||
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?
Assignee | ||
Comment 8•9 years ago
|
||
Good thought, yes we can :)
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
(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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
(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
![]() |
||
Updated•9 years ago
|
tracking-e10s:
--- → +
Priority: -- → P1
Assignee | ||
Comment 15•9 years ago
|
||
^ 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•9 years ago
|
||
Comment 18•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 19•9 years ago
|
||
If the patch from bug 1270955 still needs to uplift to aurora 48, then we should include this fix also.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
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+
Updated•9 years ago
|
Comment 22•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•