When panning in Fennec, APZ handoff should not occur when panning changes direction

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

({feature})

Trunk
mozilla48
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Currently in Fennec, if the user starts a scroll from within a scrollable element that is at the end of its scroll, the scroll is handed off until an ancestor that is able to scroll is found. If the user changes direction during the scroll however, the scrollable element will start to scroll. For Fennec, once an APZ target has been selected for a scroll, it should not change for the duration of the scroll.
(Assignee)

Updated

3 years ago
Blocks: 1206874
Do we want to change this behaviour for Fennec ("no immedate handoff" mode) only, or in general?

I understood bug 1247964 comment 7 as suggesting we change it in general, but I may have misread it.
Flags: needinfo?(bugmail.mozilla)
I think in general makes more sense.
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Updated

3 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 3

3 years ago
Created attachment 8731400 [details] [diff] [review]
0001-Bug-1257264-When-panning-with-handoff-disabled-APZ-handoff-should-not-occur-when-panning-changes-direction-16031613-24e124d.patch
(Assignee)

Updated

3 years ago
Attachment #8731400 - Flags: feedback?(botond)
Comment on attachment 8731400 [details] [diff] [review]
0001-Bug-1257264-When-panning-with-handoff-disabled-APZ-handoff-should-not-occur-when-panning-changes-direction-16031613-24e124d.patch

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

With this change, we should be able to restore the original assertion in InputBlockState::SetScrolledApzc().

r+ with comments

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2330,5 @@
>    // to *decrease* as the page follows your finger.
>    ParentLayerPoint displacement = aStartPoint - aEndPoint;
>  
>    ParentLayerPoint overscroll;  // will be used outside monitor block
> +  bool allowScroll = gfxPrefs::APZAllowImmediateHandoff() ||

I'd call this variable "scrollThisApzc". 

I also think this deserves a comment, along the lines of:

  // If the direction of panning is reversed within the same input block,
  // a later event in the block could potentially scroll an APZC earlier
  // in the handoff chain, than an earlier event in the block (because
  // the earlier APZC was scrolled to its extent in the original direction). 
  // If immediate handoff is disallowed, we want to disallow this (to 
  // preserve the property that a single input block only scrolls one APZC), 
  // so we skip the earlier the APZC.
Attachment #8731400 - Flags: feedback?(botond) → feedback+
Comment on attachment 8731400 [details] [diff] [review]
0001-Bug-1257264-When-panning-with-handoff-disabled-APZ-handoff-should-not-occur-when-panning-changes-direction-16031613-24e124d.patch

Meant this as an r+ (with comments).
Attachment #8731400 - Flags: feedback+ → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> I think in general makes more sense.

Randall's patch implements it for Fennec.

We can extend this to the general case with a bit of extra work:

  - Track InputBlockState::mScrolledApzc regardless of the value of
    gfxPrefs::APZAllowImmediateHandoff().

  - Have the restriction on handoff added in Randall's patch apply 
    regardless of the value of gfxPrefs::APZAllowImmediateHandoff().

  - Keep other restrictions on handoff (those added in bug 1230552)
    conditioned on !gfxPrefs::APZAllowImmediateHandoff().

We can do this in a follow-up patch (in this bug, or in another bug, I'm fine either way).
(Assignee)

Comment 7

3 years ago
Created attachment 8731468 [details] [diff] [review]
0001-Bug-1257264-When-panning-with-handoff-disabled-APZ-handoff-should-not-occur-when-panning-changes-direction-r-botond-16031615-b26480d.patch

Address comments. Carry forward r+ from :botond.
Attachment #8731400 - Attachment is obsolete: true
Keywords: feature
Whiteboard: [gfx-noted]

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ba52b16f83a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

3 years ago
Blocks: 1260588
You need to log in before you can comment on or make changes to this bug.