Closed Bug 1367765 (apz-scrollbar-touch-dragging) Opened 7 years ago Closed 7 years ago

APZ: Support async scrollbar touch dragging

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- verified

People

(Reporter: kats, Assigned: botond)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(6 files)

We support async scrollbar dragging with mouse. And we support dragging the scrollbar with touch on the main thread. We should add support for dragging the scrollbar with touch asynchronously as well.

We could probably cheat a little and implement this by generating a DragInputBlock with fake mouse events derived from touch events, if we know the touch event landed on a scrollbar.
Priority: -- → P3
Whiteboard: [gfx-noted]
Alias: apz-scrollbar-touch-dragging
Summary: Support async scrollbar touch dragging → APZ: Support async scrollbar touch dragging
Milan says this bug is on the gfx team's backlog for 57.
Assignee: nobody → botond
Posting some WIP patches. They're fairly buggy, but the general idea is there.
(In reply to Botond Ballo [:botond] from comment #4)
> They're fairly buggy

The first bug I've been investigating is that sometimes, the scrollbar thumb can get stuck in the ":active" state (where it's colored differently) after a touch drag.

This bug seems to be caused by ActiveElementManager. Specifically, AEM can set the NS_EVENT_STATE_ACTIVE content state flag on a touch target [1]. The code has the following comment:

  // If the touch was a click, make mTarget :active right away.
  // nsEventStateManager will reset the active element when processing
  // the mouse-down event generated by the click.

Basically, AEM sets the flag, and counts on EventStateManager (called nsEventStateManager at the time the comment was written) to clear it. However, the expecting clearing does not appear to happen for scroll thumbs, because nsSliderFrame uses a different mechanism for triggering the active style (it sets the "active" attribute [2]).

[1] http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/gfx/layers/apz/util/ActiveElementManager.cpp#122
[2] http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/xul/nsSliderFrame.cpp#706
(In reply to Botond Ballo [:botond] from comment #6)
> Basically, AEM sets the flag, and counts on EventStateManager (called
> nsEventStateManager at the time the comment was written) to clear it.
> However, the expecting clearing does not appear to happen for scroll thumbs,
> because nsSliderFrame uses a different mechanism for triggering the active
> style (it sets the "active" attribute [2]).

I fixed this by disabling the AEM activation for xul:thumb elements.

The next issue I'm looking into is that touch-tapping the thumb results in the thumb being repositioned to be centred at the tap point, whereas no such repositioning happens in response to a mouse click.
(In reply to Botond Ballo [:botond] from comment #7)
> The next issue I'm looking into is that touch-tapping the thumb results in
> the thumb being repositioned to be centred at the tap point, whereas no such
> repositioning happens in response to a mouse click.

We were activating the "scroll-to-click" code. For mouse clicks, we were restricting this to cases where the click location was outside the thumb, but there was no such restriction for touch-start events. It was straightforward to add one.

With this change, my patches seem to be working well.

One thing I still need to get working is the case where a scroll frame is not layerized at the time you touch the scrollbar. In such a case, we can't take the "APZ initiation" codepath (bug 1349750), but the patches currently only work for that codepath.
(In reply to Botond Ballo [:botond] from comment #8)
> One thing I still need to get working is the case where a scroll frame is
> not layerized at the time you touch the scrollbar. In such a case, we can't
> take the "APZ initiation" codepath (bug 1349750), but the patches currently
> only work for that codepath.

After thinking about this some more, getting this working would be fairly involved. It would essentially require queueing the events as touch events in InputQueue, and converting them to mouse events once the main thread confirms the drag block (whereas right now they are converted to mouse events before entering the queue).

I think for now, I'll just post the patches in their current state, where touch-dragging an unlayerized scroll frame will perform main-thread dragging. It will still trigger layerization, so that subsequent drags on the scroll frame will be driven by APZ. And keep in mind unlayerized scroll frames are not very common to begin with, since we have an optimization to always layerize the primary (first) async-scrollable frame on a page. We can consider handling the unlayerized case as a future enhancement.
Comment on attachment 8902479 [details]
Bug 1367765 - Factor out a SetupScrollbarDrag() helper function.

https://reviewboard.mozilla.org/r/174060/#review183510
Attachment #8902479 - Flags: review?(rhunt) → review+
Comment on attachment 8902480 [details]
Bug 1367765 - Implement scrollbar touch-dragging in APZ.

https://reviewboard.mozilla.org/r/174062/#review183488

::: gfx/layers/apz/src/APZCTreeManager.cpp:1420
(Diff revision 3)
> +        &mHitResultForInputBlock, &hitScrollbarNode);
> +
> +    // Check if this event starts a scrollbar touch-drag. The conditions
> +    // checked are similar to the ones we check for MOUSE_INPUT starting
> +    // a scrollbar mouse-drag.
> +    mInScrollbarTouchDrag = gfxPrefs::APZDragEnabled() && hitScrollbarNode &&

Should this be in a pref specific to touch? I'm okay with it being not, but I just want to make sure that's the plan.

::: gfx/layers/apz/src/APZCTreeManager.cpp:1439
(Diff revision 3)
>    } else if (mApzcForInputBlock) {
>      APZCTM_LOG("Re-using APZC %p as continuation of event block\n", mApzcForInputBlock.get());
>    }
>  
> +  if (mInScrollbarTouchDrag) {
> +    return ProcessTouchInputForScrollbarDrag(aInput, hitScrollbarNode.get(),

This early return will skip the cleanup code later in ProcessTouchInput (the condition when mTouchCounter.GetActiveTouchCount() == 0).

This looks like it could cause mInScrollbarDrag to permanently be true.
Comment on attachment 8905355 [details]
Bug 1367765 - Propagate the mHandledByAPZ flag when dispatching a touch event to the DOM.

https://reviewboard.mozilla.org/r/177146/#review183514
Attachment #8905355 - Flags: review?(rhunt) → review+
Comment on attachment 8905356 [details]
Bug 1367765 - Do not set the 'active' content state flag on scrollbar thumbs in ActiveElementManager.

https://reviewboard.mozilla.org/r/177148/#review183516
Attachment #8905356 - Flags: review?(rhunt) → review+
Comment on attachment 8905357 [details]
Bug 1367765 - Do not scroll-to-click on touchstart if touch point is over thumb.

https://reviewboard.mozilla.org/r/177150/#review183518
Attachment #8905357 - Flags: review?(rhunt) → review+
Attachment #8902480 - Flags: review?(rhunt)
(In reply to Ryan Hunt [:rhunt] from comment #19)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:1420
> (Diff revision 3)
> > +        &mHitResultForInputBlock, &hitScrollbarNode);
> > +
> > +    // Check if this event starts a scrollbar touch-drag. The conditions
> > +    // checked are similar to the ones we check for MOUSE_INPUT starting
> > +    // a scrollbar mouse-drag.
> > +    mInScrollbarTouchDrag = gfxPrefs::APZDragEnabled() && hitScrollbarNode &&
> 
> Should this be in a pref specific to touch? I'm okay with it being not, but
> I just want to make sure that's the plan.

That's a good idea, as it'll enable us to disable touch-dragging with a small code change if an issue is discovered closer to release. I'll add it.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:1439
> (Diff revision 3)
> >    } else if (mApzcForInputBlock) {
> >      APZCTM_LOG("Re-using APZC %p as continuation of event block\n", mApzcForInputBlock.get());
> >    }
> >  
> > +  if (mInScrollbarTouchDrag) {
> > +    return ProcessTouchInputForScrollbarDrag(aInput, hitScrollbarNode.get(),
> 
> This early return will skip the cleanup code later in ProcessTouchInput (the
> condition when mTouchCounter.GetActiveTouchCount() == 0).
> 
> This looks like it could cause mInScrollbarDrag to permanently be true.

(It won't cause mInScrollbarDrag to be _permanently_ true, as it'll be cleared on the next touch-start that's not over a scroll thumb, but you're right that it's more correct to clear it on the touch-end.)
(In reply to Botond Ballo [:botond] from comment #23)
> > Should this be in a pref specific to touch? I'm okay with it being not, but
> > I just want to make sure that's the plan.
> 
> That's a good idea, as it'll enable us to disable touch-dragging with a
> small code change if an issue is discovered closer to release. I'll add it.

In that case please also update about:support to list it separately. (In a follow-up bug is fine)
Also we should make sure we don't accidentally turn on touch interaction with scrollbars on Fennec.
Comment on attachment 8902480 [details]
Bug 1367765 - Implement scrollbar touch-dragging in APZ.

https://reviewboard.mozilla.org/r/174062/#review183788

::: gfx/layers/apz/src/APZCTreeManager.cpp:1558
(Diff revision 4)
> +                        aTouchInput.mTimeStamp,
> +                        aTouchInput.modifiers};
> +  mouseInput.mHandledByAPZ = true;
> +
> +  nsEventStatus result = mInputQueue->ReceiveInputEvent(mApzcForInputBlock,
> +      true, mouseInput, aOutInputBlockId);

Should we be setting targetConfirmed to false here?

It seems like the first input will be processed immediately without waiting for AsyncDragMetrics without this.

::: gfx/layers/apz/src/APZCTreeManager.cpp:1576
(Diff revision 4)
> +  //    - We don't want to apply the callback transform in the main thread,
> +  //      so we remove the scrollid from the guid.
> +  // Both of these match the behaviour of mouse events that target a scrollbar;
> +  // see the code for handling mouse events in ReceiveInputEvent() for
> +  // additional explanation.
> +  aOutTargetGuid->mScrollId = FrameMetrics::NULL_SCROLL_ID;

We don't call InputQueue::IsDragOnScrollbar() for touch inputs.

This is fine because the result is used to determine whether to untransform the input and we know here to not untransform unconditionally.

Does it make sense to call this for consistency between the inputs? I don't have a strong opinion either way.
Comment on attachment 8906803 [details]
Bug 1367765 - Put apz scrollbar touch-dragging behind a pref.

https://reviewboard.mozilla.org/r/178524/#review183796

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:214
(Diff revision 1)
> + * if possible. Only has an effect if apz.drag.enabled is also true.
> + *
> + * \li\b apz.drag.touch.enabled
> + * Setting this pref to true will cause APZ to handle touch-dragging of
> + * scrollbar thumbs. Only has an effect if apz.drag.enabled and
> + * apz.drag.initial.enabled are also true.

Are we only allowing touch dragging when `apz.drag.initial.enabled` is true? I didn't see any code specifically for this.
Attachment #8906803 - Flags: review?(rhunt) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> Also we should make sure we don't accidentally turn on touch interaction
> with scrollbars on Fennec.

I loaded the try build from comment 30 onto a device and verified that I cannot drag the scrollbar via touch so I think this is fine.
(In reply to Ryan Hunt [:rhunt] from comment #33)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:1558
> (Diff revision 4)
> > +                        aTouchInput.mTimeStamp,
> > +                        aTouchInput.modifiers};
> > +  mouseInput.mHandledByAPZ = true;
> > +
> > +  nsEventStatus result = mInputQueue->ReceiveInputEvent(mApzcForInputBlock,
> > +      true, mouseInput, aOutInputBlockId);
> 
> Should we be setting targetConfirmed to false here?
> 
> It seems like the first input will be processed immediately without waiting
> for AsyncDragMetrics without this.

Good catch. We may confirm it right away in SetupScrollbarDrag(), but that happens after the first InputQueue::ReceiveInputEvent() call, so we should pass false there.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:1576
> (Diff revision 4)
> > +  //    - We don't want to apply the callback transform in the main thread,
> > +  //      so we remove the scrollid from the guid.
> > +  // Both of these match the behaviour of mouse events that target a scrollbar;
> > +  // see the code for handling mouse events in ReceiveInputEvent() for
> > +  // additional explanation.
> > +  aOutTargetGuid->mScrollId = FrameMetrics::NULL_SCROLL_ID;
> 
> We don't call InputQueue::IsDragOnScrollbar() for touch inputs.
> 
> This is fine because the result is used to determine whether to untransform
> the input and we know here to not untransform unconditionally.
> 
> Does it make sense to call this for consistency between the inputs? I don't
> have a strong opinion either way.

If we get into ProcessTouchInputForScrollbarDrag(), we know we're in a scrollbar drag, so I don't think an extra check is necessary.
(In reply to Ryan Hunt [:rhunt] from comment #34)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:214
> (Diff revision 1)
> > + * if possible. Only has an effect if apz.drag.enabled is also true.
> > + *
> > + * \li\b apz.drag.touch.enabled
> > + * Setting this pref to true will cause APZ to handle touch-dragging of
> > + * scrollbar thumbs. Only has an effect if apz.drag.enabled and
> > + * apz.drag.initial.enabled are also true.
> 
> Are we only allowing touch dragging when `apz.drag.initial.enabled` is true?
> I didn't see any code specifically for this.

At the time I wrote this, I thought that if we didn't get into the "if (APZDragInitiationEnabled() && ...)" block in SetupScrollbarDrag(), we wouldn't get async scrolling with touch events, because no one would confirm the drag block subsequently.

However, I was mistaken. As with mouse events, the main thread will confirm the drag by calling APZCTreeManager::StartScrollbarDrag(), and henceforth the drag will be handled in APZ.

So, the two prefs are actually orthogonal. I'll adjust this comment accordingly.
Comment on attachment 8902480 [details]
Bug 1367765 - Implement scrollbar touch-dragging in APZ.

https://reviewboard.mozilla.org/r/174062/#review184406
Attachment #8902480 - Flags: review?(rhunt) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79b0b2a82a4d
Factor out a SetupScrollbarDrag() helper function. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/e11695ed7958
Implement scrollbar touch-dragging in APZ. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/aaf870ea6e87
Propagate the mHandledByAPZ flag when dispatching a touch event to the DOM. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/a90f4d33dc80
Do not set the 'active' content state flag on scrollbar thumbs in ActiveElementManager. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/21e6767af285
Do not scroll-to-click on touchstart if touch point is over thumb. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/ba1cf2c8ad66
Put apz scrollbar touch-dragging behind a pref. r=rhunt
Thanks for the reviews, Ryan!
Blocks: 1399582
Verified using Firefox 57.0 beta 4 (under Microsoft Surface Pro 2 device with Win 10 Insider Preview build) that the testcases from https://testrail.stage.mozaws.net/index.php?/runs/view/5202&group_by=cases:section_id&group_order=asc are all passing.

Bug 1353284 is still reproducible, I updated it with my findings.
Status: RESOLVED → VERIFIED
This still doesn't work in v57 release on my test windows 10 computer :/ Anything I should be aware of ? Should I file a separate bug ? Anything I can do to help ?
(In reply to Julien Wajsberg [:julienw] from comment #48)
> This still doesn't work in v57 release on my test windows 10 computer :/
> Anything I should be aware of ? Should I file a separate bug ? Anything I
> can do to help ?

Please file a new bug with the following info:

  - Example website where the problem occurs
  - Graphics section of about:support
  - The symptoms that lead you to believe it's not working

I will look into it as soon as I can. Thanks!
I filed bug 1419565. tldr: it actually works on a clean profile.
You need to log in before you can comment on or make changes to this bug.