Closed Bug 1257264 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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.
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: nobody → rbarker
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).
Keywords: feature
Whiteboard: [gfx-noted]
https://hg.mozilla.org/mozilla-central/rev/4ba52b16f83a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1260588
You need to log in before you can comment on or make changes to this bug.