1016035 - Implement back/forward swiping with APZ

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

At the moment, back/forward swiping is kicked of in nsChildView.mm from wheel events that are processed on the main thread. However, when we use async panning for scrolling, the main thread usually doesn't see any wheel events, and the swipe gesture cannot be started.

We may need the APZC to send events to the main thread that are analogous to those we currently get from the Cocoa swipe callback, plus an initial event that lets us prevent normal APZ overscroll in the right cases.

Comment 1

[Florian Bender]

Is there a way back/forward swiping can be reimplemented with APZ (preferably with as little MT processing as possible) that avoids Bug 927702 from happening?

Comment 2

[Markus Stange [:mstange]]

Yes, implementing it with APZ would avoid it, since we don't have access to the buggy API on the APZ thread.

Comment 3

[Markus Stange [:mstange]]

Attachment: demo with nested scrollboxes

Here's an implementation idea. This way we wouldn't have to call back to the main thread before starting the swipe because all the necessary layers and swipe positioning information would already be known to the APZC.

Comment 4

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=433207366c81

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Remove the ability to swipe vertically. r?kats

Bug 1016035 - Remove the ability to swipe vertically. r?kats

This was intended for a snapshot-based overscroll animation which we're not going to use because APZ solves the same problem in a much better way.

Comment 6

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Move some code around. r?kats

Bug 1016035 - Move some code around. r?kats

Comment 7

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

Comment 8

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - More swipe refactoring. r?kats

Bug 1016035 - More swipe refactoring. r?kats

Comment 9

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

Comment 10

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

Comment 11

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Having this event means that we don't have to wait for content to find out whether it's scrollable in the case that no swipe should be happening anyway.

Comment 12

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Comment 13

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Comment 14

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Comment 15

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Comment 16

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Comment 17

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Don't set scroll overflow information on events that APZ already used for scrolling. r?masayuki

Comment 18

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Comment 19

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Set overscroll information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Set overscroll information on potential swipe start events that have been processed by APZ. r?masayuki

Comment 20

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Comment 21

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Comment 22

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Comment 23

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Comment 24

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Remove const in a few places. r?kats

Bug 1016035 - Remove const in a few places. r?kats

The next patch will need to modify the event in the PanGestureState constructor in order to pass information back to nsChildView.

Comment 25

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Comment 26

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653273 [details]
MozReview Request: Bug 1016035 - Remove the ability to swipe vertically. r?kats

https://reviewboard.mozilla.org/r/17405/#review15537

I don't know this code but deleting code is usually pretty safe :)

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

https://reviewboard.mozilla.org/r/17407/#review15541

::: widget/cocoa/nsChildView.mm:4954
(Diff revision 1)
> +    if (!mGeckoChild) {

This seems kinda wrong, because we just used mGeckoChild on the previous line assuming it was non-null...

::: widget/cocoa/nsChildView.mm:4959
(Diff revision 1)
> +    // overflowDeltaX and overflowDeltaY tell us when the user has tried to

Move this comment down to inside the if condition

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653275 [details]
MozReview Request: Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

https://reviewboard.mozilla.org/r/17409/#review15547

::: widget/cocoa/nsChildView.mm:4943
(Diff revision 1)
> +    bool canTriggerSwipe = [self shouldConsiderStartingSwipeFromEvent:theEvent];

Can we move this down to just before where it is used (inside the ifdef)? AFAICT there's no reason it needs to be before DispatchAPZWheelEvent.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653276 [details]
MozReview Request: Bug 1016035 - More swipe refactoring. r?kats

https://reviewboard.mozilla.org/r/17411/#review15549

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

https://reviewboard.mozilla.org/r/17415/#review15551

Looks pretty good but I want to see the rest of the patch queue and let it sink in a bit before I do a second pass.

::: widget/cocoa/SwipeTracker.h:49
(Diff revision 1)
> +  virtual void WillRefresh(mozilla::TimeStamp aTime) override;

Drop the "virtual" since "override" implies it

::: widget/cocoa/SwipeTracker.h:55
(Diff revision 1)
> +  double SwipeSuccessTargetValue() const;

The notion of "SwipeSuccess" could use an explanatory comment. I see this phrase in two method names but it's not obvious what it means.

::: widget/cocoa/SwipeTracker.mm:7
(Diff revision 1)
> +#include "SwipeTracker.h"

Leave a blank line after including SwipeTracker.h

::: widget/cocoa/SwipeTracker.mm:22
(Diff revision 1)
> +using namespace mozilla;

I'd prefer to avoid "using namespace" since it can screw with unified builds. You should just be able to wrap the body of this file in a "namespace mozilla {...}" block and get the same effect.

::: widget/cocoa/SwipeTracker.mm:30
(Diff revision 1)
> +  , mEventPosition(RoundedToInt(aSwipeStartEvent.mPanStartPoint * ScreenToLayoutDeviceScale(1)))

Use ViewAs<>

::: widget/cocoa/SwipeTracker.mm:49
(Diff revision 1)
> +    }

else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

::: widget/cocoa/SwipeTracker.mm:162
(Diff revision 1)
> +    }

else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

Might even be good to extract a helper to do this removal and setting the flag to false, and call it from both Destroy() and here

::: widget/cocoa/SwipeTracker.mm:177
(Diff revision 1)
> +  mWidget.SwipeFinished(); 

trailing ws

::: widget/cocoa/SwipeTracker.mm:183
(Diff revision 1)
> +  nsIWidgetListener* widgetListener = mWidget.GetWidgetListener();

This seems a little scary to me, since it's not obvious to me that you'll be getting the same refresh driver always. You might end up registering the observer on one refresh driver and unregistering it on another, causing a leak. Usually in cases like this I try to keep a refptr to the target (the refresh driver in this case) in the class, so that I know I'm unregistering on the same object that I registered it on. I'm not sure if you can have a refptr to the refresh driver but having one to the presshell should be ok. It would at least guard against the presShell in the widget changing out from under you between the register and unregister.

::: widget/cocoa/nsChildView.mm:2583
(Diff revision 1)
> +    RoundedToInt(aSwipeStartEvent.mPanStartPoint * ScreenToLayoutDeviceScale(1));

ViewAs<>

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653279 [details]
MozReview Request: Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

https://reviewboard.mozilla.org/r/17417/#review15579

Doesn't look like the IDL changes need a UUID bump here but the hg push hook will probably yell at you when you try to land this unless you add IGNORE IDL to the commit message.

::: browser/base/content/browser-gestureSupport.js:67
(Diff revision 1)
> +        this._setupSwipeGesture();

For consistency with the next couple of case statements, move the preventDefault() up to before the setupSwipeGesture() call.

Comment 33

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17407/#review15541

> This seems kinda wrong, because we just used mGeckoChild on the previous line assuming it was non-null...

The idea here is: Dispatching the event can run arbitrary JS, which might close the window and thus destroy the widget, which nulls out mGeckoChild. It's not pretty but that's how other parts of this code have handled this problem, and I'm just moving code here.

Comment 34

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17415/#review15551

> else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

We're not necessarily leaking stuff. If the refresh driver has gone away, it has dropped its reference to us. If it's still alive and we just can't get at it anymore, then we're in trouble.
Matt Woodrow tells me it should be safe to keep an nsRefPtr<nsRefreshDriver>, so I'm going to do that and avoid this case.

> else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.
> 
> Might even be good to extract a helper to do this removal and setting the flag to false, and call it from both Destroy() and here

I'll extract it.

> This seems a little scary to me, since it's not obvious to me that you'll be getting the same refresh driver always. You might end up registering the observer on one refresh driver and unregistering it on another, causing a leak. Usually in cases like this I try to keep a refptr to the target (the refresh driver in this case) in the class, so that I know I'm unregistering on the same object that I registered it on. I'm not sure if you can have a refptr to the refresh driver but having one to the presshell should be ok. It would at least guard against the presShell in the widget changing out from under you between the register and unregister.

Good Point. nsICanvasRenderingContextInternal also has an nsRefPtr<nsRefreshDriver>, so it should be safe to use.

Comment 35

[:kip (Kearwood Gilbert)]

Comment on attachment 8653277 [details]
MozReview Request: Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

https://reviewboard.mozilla.org/r/17413/#review15611

This looks good, Ship it!

Comment 36

[Kartikaya Gupta (email:[e-mail])]

(In reply to Markus Stange [:mstange] from comment #33)
> The idea here is: Dispatching the event can run arbitrary JS, which might
> close the window and thus destroy the widget, which nulls out mGeckoChild.
> It's not pretty but that's how other parts of this code have handled this
> problem, and I'm just moving code here.

Ah I see. Worth a comment then. I assume we're not doing a kungFuDeathGrip for a good reason here.

Comment 37

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #36)
> (In reply to Markus Stange [:mstange] from comment #33)
> > The idea here is: Dispatching the event can run arbitrary JS, which might
> > close the window and thus destroy the widget, which nulls out mGeckoChild.
> > It's not pretty but that's how other parts of this code have handled this
> > problem, and I'm just moving code here.
> 
> Ah I see. Worth a comment then. I assume we're not doing a kungFuDeathGrip
> for a good reason here.

Well, the old code didn't have the comment, and a later patch is removing the line anyway, so I'm not going to add the comment.

We might still need a few kunFuDeathGrips in the nsChildView methods that send events. I haven't really thought about all the implications there.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

https://reviewboard.mozilla.org/r/17425/#review15643

::: widget/BasicEvents.h:120
(Diff revision 1)
> +  bool mCanTriggerSwipe : 1;

Why don't you add this flag to WidgetWheelEvent directly? BaseEventFlags should be used only when the flag is used by two or more event classes.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15647

::: dom/events/EventStateManager.cpp:3134
(Diff revision 1)
> -          // If we don't handle the wheel event, all of the delta values must
> +          if (!wheelEvent->mFlags.mHandledByAPZ) {

nit:

if (wheelEvent->mFlags.mHandledByAPZ) {
  break;
}

is better.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653287 [details]
MozReview Request: Bug 1016035 - Set overscroll information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17433/#review15649

::: dom/events/EventStateManager.cpp:3074
(Diff revision 1)
> +      if (wheelEvent->mFlags.mHandledByAPZ && wheelEvent->mFlags.mCanTriggerSwipe &&
> +          !ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {
> +        // No scrollframe is available for scrolling in the requested direction.
> +        wheelEvent->mViewPortIsOverscrolled = true;
> +        wheelEvent->overflowDeltaX = wheelEvent->deltaX;
> +        wheelEvent->overflowDeltaY = wheelEvent->deltaY;
> +        WheelPrefs::GetInstance()->
> +          CancelApplyingUserPrefsFromOverflowDelta(wheelEvent);
> +      }

Why don't you include this into the above |if (wheelEvent->mFlags.mHandledByAPZ| block or the case ACTION_NONE below? I like the latter better because this handles default action as ACTION_NONE. When other developers changes the code in case ACTION_NONE, this block should be checked by their eyes.

Comment 41

[Markus Stange [:mstange]]

Comment on attachment 8653273 [details]
MozReview Request: Bug 1016035 - Remove the ability to swipe vertically. r?kats

Bug 1016035 - Remove the ability to swipe vertically. r?kats

This was intended for a snapshot-based overscroll animation which we're not going to use because APZ solves the same problem in a much better way.

Comment 42

[Markus Stange [:mstange]]

Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

Bug 1016035 - Move some code around. r?kats

Comment 43

[Markus Stange [:mstange]]

Comment on attachment 8653275 [details]
MozReview Request: Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

Comment 44

[Markus Stange [:mstange]]

Comment on attachment 8653276 [details]
MozReview Request: Bug 1016035 - More swipe refactoring. r?kats

Bug 1016035 - More swipe refactoring. r?kats

Comment 45

[Markus Stange [:mstange]]

Comment on attachment 8653277 [details]
MozReview Request: Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

Comment 46

[Markus Stange [:mstange]]

Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

Comment 47

[Markus Stange [:mstange]]

Comment on attachment 8653279 [details]
MozReview Request: Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Having this event means that we don't have to wait for content to find out whether it's scrollable in the case that no swipe should be happening anyway.

IGNORE IDL because I'm only changing comments in the IDL files.

Comment 48

[Markus Stange [:mstange]]

Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Comment 50

[Markus Stange [:mstange]]

Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Comment 51

[Markus Stange [:mstange]]

Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Comment 52

[Markus Stange [:mstange]]

Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Comment 53

[Markus Stange [:mstange]]

Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Comment 54

[Markus Stange [:mstange]]

Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Comment 55

[Markus Stange [:mstange]]

Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Comment 56

[Markus Stange [:mstange]]

Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Comment 57

[Markus Stange [:mstange]]

Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Comment 58

[Markus Stange [:mstange]]

Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Comment 59

[Markus Stange [:mstange]]

Comment on attachment 8653292 [details]
MozReview Request: Bug 1016035 - Remove const in a few places. r?kats

Bug 1016035 - Remove const in a few places. r?kats

The next patch will need to modify the event in the PanGestureState constructor in order to pass information back to nsChildView.

Comment 60

[Markus Stange [:mstange]]

Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Comment 61

[Markus Stange [:mstange]]

Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

https://reviewboard.mozilla.org/r/17425/#review15663

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15665

::: dom/events/EventStateManager.cpp:3140
(Diff revision 2)
> +            if (ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {

This line is over 80 characters.

::: dom/events/EventStateManager.cpp:3140
(Diff revision 2)
> +            if (ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {
> +              // Something is scrollable in the requested direction, so setting
> +              // scroll overflow would be wrong.
> +              break;

I'm confused now. Why do we need to check scroll target when the default action is none? And then, what was performed (handled) by APZ?

Comment 64

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17429/#review15665

> I'm confused now. Why do we need to check scroll target when the default action is none? And then, what was performed (handled) by APZ?

Yeah, sorry, it is very confusing. I'd appreciate any tips on how to make this code easier to understand.

There are three cases here:
 1. No APZ: The behavior should be the same as before.
 2. APZ for events without mCanTriggerSwipe: APZ handles scrolling and nobody looks at the scroll overflow fields.
 3. APZ for events with mCanTriggerSwipe: For these events, APZ will handle the scrolling, but it delays the actual scrolling until after it gets a response from content. And the content response depends on event.TriggersSwipe(), which looks at the scroll overflow values. That's why the event needs to know whether something is scrollable even if it has already been processed by APZ. (So in that case "mHandledByAPZ" means "APZ looked at this event and might do scrolling based on it", and not "APZ has already scrolled in response to this event".)

The patch two before this one and the patch three after this one add the event.TriggersSwipe() checks, if you want to look at them.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/17429/#review15665

> Yeah, sorry, it is very confusing. I'd appreciate any tips on how to make this code easier to understand.
> 
> There are three cases here:
>  1. No APZ: The behavior should be the same as before.
>  2. APZ for events without mCanTriggerSwipe: APZ handles scrolling and nobody looks at the scroll overflow fields.
>  3. APZ for events with mCanTriggerSwipe: For these events, APZ will handle the scrolling, but it delays the actual scrolling until after it gets a response from content. And the content response depends on event.TriggersSwipe(), which looks at the scroll overflow values. That's why the event needs to know whether something is scrollable even if it has already been processed by APZ. (So in that case "mHandledByAPZ" means "APZ looked at this event and might do scrolling based on it", and not "APZ has already scrolled in response to this event".)
> 
> The patch two before this one and the patch three after this one add the event.TriggersSwipe() checks, if you want to look at them.

Ah, I see. Could you add comment before the if statement?

Comment 66

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

https://reviewboard.mozilla.org/r/17407/#review15695

Comment 67

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

https://reviewboard.mozilla.org/r/17415/#review15701

Comment 68

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

https://reviewboard.mozilla.org/r/17419/#review15703

Comment 69

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

https://reviewboard.mozilla.org/r/17421/#review15707

::: widget/cocoa/nsChildView.h:555
(Diff revision 2)
> -  void MaybeTrackScrollEventAsSwipe(const mozilla::PanGestureInput& aSwipeStartEvent);
> +  struct SwipeInfo {

Does this stuff need to be public? I'd prefer protected or private.

Comment 70

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

https://reviewboard.mozilla.org/r/17423/#review15711

::: widget/cocoa/nsChildView.mm:2770
(Diff revision 2)
> +        SwipeInfo swipeInfo = SendMayStartSwipe(panInput);

Should the event still be sent if the event's deltas are zero? I feel like the SendMayStartSwipe call should be inside the if condition still (although before the DispatchEvent)

Comment 71

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

https://reviewboard.mozilla.org/r/17427/#review15713

Comment 72

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

https://reviewboard.mozilla.org/r/17431/#review15715

Comment 73

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17423/#review15711

> Should the event still be sent if the event's deltas are zero? I feel like the SendMayStartSwipe call should be inside the if condition still (although before the DispatchEvent)

Looking at this again, I think it would be better to just remove the if condition completely. If aCanTriggerSwipe is true, we know that this is an event that we want to send to Gecko, regardless of what it looks like (though in reality it will always be an NS_WHEEL_WHEEL event with a deltaX != 0).

Comment 74

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17429/#review15665

> Ah, I see. Could you add comment before the if statement?

Sure.

Comment 75

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

https://reviewboard.mozilla.org/r/17437/#review15727

Comment 76

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

https://reviewboard.mozilla.org/r/17439/#review15731

Comment 77

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

https://reviewboard.mozilla.org/r/17435/#review15735

Comment 78

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

https://reviewboard.mozilla.org/r/17441/#review15737

Comment 79

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

https://reviewboard.mozilla.org/r/17445/#review15755

::: gfx/layers/apz/src/InputBlockState.cpp:457
(Diff revision 2)
> +    aInitialEvent.mHorizontalScrollStartSuccess =

As discussed on IRC I think a better approach would be to use the return value of APZCTreeManager::ReceiveInputEvent to send this information back to the widget. This should map closely to the existing semantics of that function and will avoid having to mutate the PanGestureInput. This will also make it unnecessary to un-const all of the things.

Comment 80

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653292 [details]
MozReview Request: Bug 1016035 - Remove const in a few places. r?kats

https://reviewboard.mozilla.org/r/17443/#review15757

Clearing review flag on this patch for now (hopefully that's what MozReview does)

Comment 81

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

https://reviewboard.mozilla.org/r/17447/#review15759

Comment 82

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17429/#review15665

> Sure.

I'm changing the code around a little to make this easier to grasp.

Comment 83

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/17421/#review15707

> Does this stuff need to be public? I'd prefer protected or private.

I'm making it protected.

Comment 84

[Markus Stange [:mstange]]

Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Comment 85

[Markus Stange [:mstange]]

Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Comment 86

[Markus Stange [:mstange]]

Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Comment 87

[Markus Stange [:mstange]]

Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Comment 88

[Markus Stange [:mstange]]

Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Comment 89

[Markus Stange [:mstange]]

Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Comment 90

[Markus Stange [:mstange]]

Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Comment 91

[Markus Stange [:mstange]]

Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Comment 92

[Markus Stange [:mstange]]

Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Comment 93

[Markus Stange [:mstange]]

Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Comment 94

[Markus Stange [:mstange]]

Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Comment 95

[Markus Stange [:mstange]]

Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Comment 96

[Markus Stange [:mstange]]

Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.

Comment 97

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15817

Thanks!

Comment 98

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

https://reviewboard.mozilla.org/r/17445/#review15827

Much better, thanks!

::: widget/InputData.h:347
(Diff revision 3)
> +  bool mRequiresContentResponseIfCannotScrollHorizontallyInStartDirection;

This is quite a mouthful but I can't think of anything better... plus it's kinda amusing :)

Comment 99

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f454eb814549
https://hg.mozilla.org/integration/mozilla-inbound/rev/04156e25e29e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2743b8411a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2274ab11481d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4380c90c05
https://hg.mozilla.org/integration/mozilla-inbound/rev/e118d7c25839
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4dcb289cf3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdbe2c3a592
https://hg.mozilla.org/integration/mozilla-inbound/rev/306cff29c09c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3705791032e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/487165622090
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a65e885bf4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f452ca4f5353
https://hg.mozilla.org/integration/mozilla-inbound/rev/5030ce0ef9b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ac3f094483
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eab0486d697
https://hg.mozilla.org/integration/mozilla-inbound/rev/b88fbd7853b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd30922f9e12
https://hg.mozilla.org/integration/mozilla-inbound/rev/a918fecc2201
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba6e5946b85

Comment 100

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f454eb814549
https://hg.mozilla.org/mozilla-central/rev/04156e25e29e
https://hg.mozilla.org/mozilla-central/rev/e2743b8411a3
https://hg.mozilla.org/mozilla-central/rev/2274ab11481d
https://hg.mozilla.org/mozilla-central/rev/5d4380c90c05
https://hg.mozilla.org/mozilla-central/rev/e118d7c25839
https://hg.mozilla.org/mozilla-central/rev/f4dcb289cf3c
https://hg.mozilla.org/mozilla-central/rev/8fdbe2c3a592
https://hg.mozilla.org/mozilla-central/rev/306cff29c09c
https://hg.mozilla.org/mozilla-central/rev/3705791032e1
https://hg.mozilla.org/mozilla-central/rev/487165622090
https://hg.mozilla.org/mozilla-central/rev/9a65e885bf4f
https://hg.mozilla.org/mozilla-central/rev/f452ca4f5353
https://hg.mozilla.org/mozilla-central/rev/5030ce0ef9b0
https://hg.mozilla.org/mozilla-central/rev/e3ac3f094483
https://hg.mozilla.org/mozilla-central/rev/5eab0486d697
https://hg.mozilla.org/mozilla-central/rev/b88fbd7853b6
https://hg.mozilla.org/mozilla-central/rev/bd30922f9e12
https://hg.mozilla.org/mozilla-central/rev/a918fecc2201
https://hg.mozilla.org/mozilla-central/rev/7ba6e5946b85