863514 - Electrolysis: Make gesture support work

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Rotating images does somewhat work, they don't snap into position though.

Back and forward gestures (two finger swipe) however work on Mac and would make e10s much more enjoyable. Can we re-enable gestures for e10s even if they're partially broken right now?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-863514-Don-t-disable-gestures-in-e10s-even-if-th.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/52c1eacc3f77

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52c1eacc3f77

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Adding bug 698371 as a dep to make the Mac history swipe thing work.

Comment 7

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Marco, can you add this bug to the current iteration?

Comment 13

[Marco Mucci [:MarcoM]]

Added to IT 36.1

Comment 15

[Albert Scheiner [:alberts]]

I was just about to file a bug regarding the three-finger swipe for fast scrolling to top/bottom as described in bug 1066841. It's with other gesture related bugs marked as duplicate of this one. Some of them are working already (e.g. bug 1066306 , bug 1068526) ... others are not.
Is there a list of gestures required which also shows which ones have been implemented already?

Comment 16

[Chris Peterson [:cpeterson]]

This bug was assigned to M4 two months ago. Given our new e10s milestones, is gesture support still M4-worthy?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

If I recall correctly, I think the idea was that fixing this would fix some of the OS X issues with side-scrolling (if you're viewing a very wide webpage and attempt to scroll left or right with a Macbook trackpad, you move by only a few pixels with every swipe on OS X it seems).

Comment 20

[Mike Conley (:mconley) (:⚙️)]

billm says that blassey has some context on this bug that I'll want. I should get that from blassey.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

From blassey:

09:48 (blassey) mconley: so... the central problem is we need to see if the content (or the chrome code in the conent process) wants to handle the gesture
09:48 (blassey) before deciding how to handle it in the chrome process
09:48 (blassey) and that needs to happen synchronously as it stands now
09:49 (blassey) so, for example, if you make a sideways swiping gesture on a mac
09:49 bsmedberg has joined ([e-mail])
09:49 (blassey) that could either mean "scroll the page laterally" if the page can scroll laterally, or navigate back/forward otherwise
09:49 (blassey) or... something else if the content is trying to handle it directly

Comment 22

[Mike Conley (:mconley) (:⚙️)]

My tentative, theoretical, mostly unresearched plan here is to have the content process update the parent when focus moves from one scrollable area to another, so that the parent knows about the "scrollability" of the current content that the cursor is over.

The parent stashes that value, and when it comes time to decide what to do with a scroll event, we check that value stashed in the parent synchronously.

I still need to get more of an idea of how these gesture events work in Cocoa-land. I'll see if I can pull mstange over today and ask him in person.

Comment 23

[Tracy Walker [:tracy]]

per request in irc I QA'd gestures to figure out what is working in e10s and what is not.  

Short Summary:  
- Two finger swipe for history is broken in Nightly in both e10s and non-e10s modes. 
- Three finger swipe for history works as expected.
- Three finger swipe for page top/bottom works as expected
- Pinch for zoom level works as expected.
- Twist for image rotation is broken in e10s.

Details:
- Two finger swipe for history is broken in Nightly in both e10s and non-e10s modes.
-- Set System Preference for Trackpad > More Gestures > Swipe between pages selected and set to Scroll left or right with two fingers.
-- In a tab with at least two pages of history, moderately speed slide two fingers from one side of the track pad to other.  Slide left to right to go forward in history.  right to left to go back in history.  
-- This is completely broken in Nightly. (note: release has it backwards)

====================================

- Three finger swipe for history works as expected.
-- Set System Preference for Trackpad > More Gestures > Swipe between pages selected and set to Swipe with three fingers.
-- In a tab with at least two pages of history, moderately speed slide three fingers from one side of the track pad to other.  Slide left to right to go forward in history.  right to left to go back in history. 
-- Works as expected.

====================================

- Three finger swipe for page top/bottom works as expected
-- Set System Preference for Trackpad > More Gestures > App Expose to Swipe down with four fingers. (note: Setting it to Swipe down with three fingers will override the Fx feature)
-- in a page with extensive vertical content, this bug will suffice, three finger swipe up to go to top of page or swipe down to go to bottom of page.
-- Works as expected.

====================================

- Pinch for zoom level works as expected.
-- The feature is disabled by default in Fx. Set the following to fully enable it.

    browser.gesture.pince.in — Value = cmd_fullZoomReduce
    browser.gesture.pinch.in.shift — Value = cmd_fullZoomReset
    browser.gesture.pinch.latched — Value = false
    browser.gesture.pinch.out — Value = cmd_fullZoomEnlarge
    browser.gesture.pinch.out.shift — Value = cmd_fullZoomReset
    browser.gesture.pinch.threshold — Value = 40 to 100 (set it to a number that works for you)

-- Find an image,then Context click View Image to display it alone in a tab.
-- Then two finger pinch/expand to adjust image size.
-- Shift+two finger pinch/expand resets size to default.

====================================

- Twist for image rotation is broken in e10s.
-- Find an image and View Image to display it alone in a tab.
-- Then two finger twist to rotate image according to direction of your twist.  it's a bit tricky
-- Broken in e10s mode. But works when Nightly is set to non-e10s mode.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Hey tracy,

Were you using OS X for your analysis in comment 23? If so, is it possible for you to test Windows as well? I'm afraid I don't have a touch-enabled Windows device at close hand. :/

Comment 25

[Tracy Walker [:tracy]]

Yes, that was just on a MacBookPro track pad.  I have a SurfacePro. I'll investigate what does and doesn't work there and report back.

Comment 26

[Tracy Walker [:tracy]]

Most of these touch controls do not work on SurfacePro. 

Short Summary:  
- Two or three finger swipe for history does not work with e10s enabled nor disabled
- Three finger swipe for page top/bottom does not work with e10s enabled nor disabled
- Pinch for zoom level works as expected in both e10s and non-e10s
- Twist for image rotation does not work in e10s but works in non-e10s

I don't know where the Windows system preferences for touch controls are or if they exist at all.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Thanks tracy!

Comment 28

[Mike Conley (:mconley) (:⚙️)]

So, I've been digging at this with mstange's help for a few days, and here are our findings.

Strap in, it's a hell of a read.

The problem comes down to having the parent know whether or not the user's cursor is over a region that can be panned or not.

This problem is easily solvable once APZ is available on OS X, since the parent process will know what regions can be panned.

APZ is, at this point, however, not yet ready to be shipped on OS X. It's disabled by default, and has some bugs in it. Quoting mstange:

16:03 (mstange) and I know of some bugs with event regions that make layerization worse, and where we just build wrong event regions
16:04 (mstange) so if we were to enable event regions, those bugs would have to be fixed first
16:04 (mconley) mstange: so, by turning it on, it's possible that input events from the user could be dispatched in what appears to be the wrong locations?
16:05 (mstange) they might end up scrolling the wrong layer
16:05 (mstange) and we might be creating more layers than necessary, which would mean more time spent painting and compositing them

So, I think it's pretty safe to assume that APZ is currently out of the game for M6 (though these problems will likely be fixed before e10s ships).

That leaves us with a "bandaid" solution until APZ is available.

So the challenge is to come up with the best "bandaid".

Here are the options that mstange and I have come up with:


Solution #1 (discarded):

When we detect a swipe entering the begin phase, send that first event down to the child (marking it so that we get a reply from the child), and start queuing scrollwheel events in the parent process without dispatching them. Wait until that first event replies back with whether or not the target was on a scrollable region.

If so, replay the queued events while tracking them as potential gestures, using trackScrollWithOptions in the parent process in ChildView.
If not, replay the queued events onto the content process to pan the region.

This was discarded because it looks like OS X will coalesce the events when we replay them, meaning we lose a bunch of information (most importantly, we lose the last event, the one with the "Ended" phase that we use to fire off the gesture event to Gecko). It's not clear to us what OS X uses as its criteria for coalescing - it appears to be undocumented.


Solution #2:

Where the first solution involved holding a queue of events in the parent process, the second one involves using the content process as the queue. We send all scroll wheel events down to the content process (like we almost already do, but also send down the begin/end phase scroll wheel events), and marks them to say that the parent process expects a reply for them. The content process receives these events in order, and once it receives the first one, analyzes whether the event target can be panned. It sends a reply up to the parent with the result.

The parent receives the message, and if the message says "I am on a scrolling region", then it ignores the rest of the events that come back up (or maybe we even skip sending those. It doesn't matter at this point).

If the message says, "I am on a region that can't scroll in that direction", then the parent process begins to track that event as a swipe. trackSwipeWithOptions gets called in the ChildView of the window, and observes the event queue and fires a callback each time an event comes in, analyzing them until it resolves it to a gesture or not (and in the success case, fires a Gecko event for the gesture).

This idea has not been tested, but the coalescing problem from Solution #1 might burn us again since events could be sent up in bursts. Again, it's not clear how or why OS X decides to coalesce events.

We also might get some weird behaviour where in the gap of time between the parent sending that Began-phase event, and starting to track swipe, that the content process uses any intervening events as a pan. This seems like a small glitch-y problem, and not worth worrying too much about for a bandaid, but I thought I'd bring it up.


Solution #3:

Send a synchronous message down to the content process to determine if the region is pannable. Sync messages are bad, but this would get us what we want with what I believe to be a minimal amount of fuss.


Solution #4:

Disable history swiping until APZ is ready.


Solution #5:

Implement our own track swiping mechanism inside the content process. The parent sends down the events (including the Began and Ended phase events), and the child knows if the target is pannable. If so, it starts up our own homebrew version of trackSwipeWithOptions (mstange says we just need to check the cumulative swipe distance and figure out the velocity at the phase Ended event), and then sends a message back up to the parent if it has resolved the events to a gesture.


Solution #5 might be our best bet in terms of correctness. It's not clear to me how difficult it'd be to implement something like "trackSwipeWithOptions" in the content process, but I'd probably want to make it super minimal to ensure that we don't end up doing a bunch of work just for a bandaid.

Anyhow, that's where we are.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Another solution would be to try to fast-track the parts of APZ that we need, but mstange says that this would likely be difficult.

Did I forget anything, Markus?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

:handyman, who is far more comfortable with Obj-C++ and Cocoa than I am, has graciously offered to take this from me.

Comment 31

[David Parks [:handyman]]

Attachment: WIP: Initial Rough POC Patch

This (messy) patch is an initial implementation of the idea that mconley and mstange settled on in comment 28 (its Solution #2).  There are still a few calls to be made on the implementation but they are comparatively minor.  This version works as proof-of-concept.

Remaining issues:
* The main remaining issue is synthetic vs. nonsynth Cocoa events.  Scroll wheel events do not have a special OSX routine for synthetic generation like e.g. mouse events do.  There are theoretically ways to skirt this (by synthesizing the scroll wheel events with Core Graphics or from scratch by manually constructing the event object from scratch -- ideas gleamed from StackOverflow and the Chromium code base) but I wasn't initially able to get either to work.  So this patch uses the event that OSX generated - I cache it in the widget.  There isn't actually anything wrong with doing that but... using events we generated would feel like we had more control over them.  Im trying a few things with synthesized events.

* This patch breaks non-e10s.  I intend to keep the code paths pretty separate (duplicating a bit of code) so that this hack can be easily extricated when we turn on APZ.  So this is a minor issue.

STR:
1. Open an e10s window
2. Go to https://bugzilla.mozilla.org/show_bug.cgi?id=863514 (i.e. a long web page)
2. In the same tab, go next to this page:
data:text/html,<html><div%20style="width:150%;">Ohai</div></html>
3. Scroll horizontally with two-finger swipe.

Expected result:
The tab smoothly scrolls horizontally since it has a horiz scrollbar.  When the bar is all the way to the left and you left-swipe, the tab should navigate back to the previous page.  When navigated to the previous page, you can vertical scroll and horizontal swipe-right to get back to the other page.

Actual result:
The tab may scroll horizontally by a tiny amount and then stop.  Otherwise, it does nothing.  Behavior on the bugzilla page is correct.

Comment 32

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

FYI I now have a patch on bug 1161206 that allows synthesizing native mousescroll events on OS X. I haven't tried it with deltas > 10px (which you said on IRC might be a problem) but I have a test which uses the function and works as expected at least.

Comment 33

[David Parks [:handyman]]

Attachment: Implement gesture workaround for Mac

This is a clean patch that fixes the bug.  It works with e10s and non-e10s.  The most controversial parts are the ESM changes and the event phase stuff.  I've run the ESM changes by smaug (he tentatively approved) and will ask masayuki about when this is final.  The event phase change is due to experimentation -- I've concluded that the NSEventPhaseBegan phase happens once per swipe and the NSEventPhaseMayBegin phase happens zero or one times.  We were using NSEventPhaseMayBegin due to some internal state-machine stuff that I've changed.

This patch still fails test_wheel_scroll.html (all other tests are green).

Comment 34

[David Parks [:handyman]]

Attachment: 2. Make test_wheel_scroll.html pass

Even with this patch, we still fail test_wheel_scroll.html but this fixes problems with the test.  Namely, it fixes some coordinate math and adds NS_WHEEL_START and NS_WHEEL_STOP event generation.  The test fails because the NS_WHEEL_START is being consumed somewhere between when it is synthesized in the widget and when it is sent to the TabParent that it targets, causing the test to stall. The test runs and passes if you manually 'prime' it with a small manual swipe gesture.  The missing NS_WHEEL_START confuses the TabChild into not claiming the motion.

An additional note about the last patch: the reason it uses a cache of NSEvents instead of synthesizing them is that I was unable to get the Cocoa gesture-recognition to work with synthetic events (trackSwipeEventWithOptions does not seem to send future scroll events to the block if it is called on a synthetic event).  This isn't too surprising since synthetic scroll events are poorly supported by the OS (compared to other input events).  (The map isn't exactly complex anyway.)

Comment 35

[Markus Stange [:mstange]]

The reason we were using NSEventPhaseMayBegin is that we'd like overlay scrollbars to appear as soon as two fingers touch the touchpad, not only once you've started moving the fingers.

I haven't looked at the patch yet, but this sounds very promising.

Comment 36

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

Comment on attachment 8607719 [details] [diff] [review]
2. Make test_wheel_scroll.html pass

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

Some drive-by comments

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +641,5 @@
>     * @param aNativeMessage
>     *   On Windows:  WM_MOUSEWHEEL (0x020A), WM_MOUSEHWHEEL(0x020E),
>     *                WM_VSCROLL (0x0115) or WM_HSCROLL (0x114).
> +   *   On Mac: 0 for NSEventPhaseChanged, 1 for NSEventPhaseBegan,
> +   *           2 for NSEventPhaseEnded

These values don't line up with what was used in the JS file

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +15,5 @@
>  
>  function nativeVerticalWheelEventMsg() {
>    switch (getPlatform()) {
>      case "windows": return 0x020A; // WM_MOUSEWHEEL
> +    case "mac": return 0; // NSEventPhaseChanged

typo? should be 0x4 instead of 0

@@ +50,5 @@
> +    msg = nativeHorizontalWheelEventMsg();
> +  } else if (aDeltaY != 0) {
> +    msg = nativeVerticalWheelEventMsg();
> +  } else {
> +    if (getPlatform().indexOf("mac") != 0)

getPlatform() == "mac". and braces please

Comment 37

[David Parks [:handyman]]

Attachment: 1. Implement gesture workaround for Mac

This version folds in the comments by kats and mstange (I now start recognition on either phase=MayBegin or phase=Began) and passes all tests but the test isn't 100% valid on mac.  Explained:

The NS_WHEEL_START/NS_WHEEL_STOP messages are only sent on mac.  I had been using them in the TabChild to track state but the state machine wasn't valid if only NS_WHEEL_WHEEL events are received.  So I added a state to the TabChild, WHEEL_UNTRACKED, that is the only state non-macs visit.  This fixes actual behavior on non-macs, which was busted in the last patch for this reason.

So the test now passes on Linux (the only place we run it).  It should pass on other platforms and even passes on mac... but on mac it shouldn't.  The deal is the same as with the previous patch -- the synthetic NS_WHEEL_START is getting swallowed early.  But now, this means that the TabChild stays in the WHEEL_UNTRACKED state, which works since we aren't actually testing a gesture (ie it follows the pre-patch behavior, which is also the non-mac behavior).  This is the only remaining issue with this bug (that I know of).

Comment 38

[David Parks [:handyman]]

Attachment: 2. Make test_wheel_scroll.html pass

See the last comment for some notes on what's up with the mac version of test_wheel_scroll.

Tests are green (both patches): https://treeherder.mozilla.org/#/jobs?repo=try&revision=93beb42839a7

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8608969 [details] [diff] [review]
1. Implement gesture workaround for Mac

>@@ -2593,16 +2593,17 @@ EventStateManager::DoScrollText(nsIScrol
>   NS_ASSERTION(aEvent->overflowDeltaX == 0 ||
>     (aEvent->overflowDeltaX > 0) == (aEvent->deltaX > 0),
>     "The sign of overflowDeltaX is different from the scroll direction");
>   NS_ASSERTION(aEvent->overflowDeltaY == 0 ||
>     (aEvent->overflowDeltaY > 0) == (aEvent->deltaY > 0),
>     "The sign of overflowDeltaY is different from the scroll direction");
> 
>   WheelPrefs::GetInstance()->CancelApplyingUserPrefsFromOverflowDelta(aEvent);
>+  return actualDevPixelScrollAmount != nsIntPoint(0,0);

I think that with current rules, this is wrong. If DoScrollText() is called during a wheel transaction, even when the actual scroll amount is empty, the scroll event is consumed by the target for preventing that another (unexpected) element is scrolled, right?

And also I wonder, how to cancel accumulated delta values if it's consumed by native gestures? In realistic scenarios, will it be timed out?

>@@ -3110,54 +3111,58 @@ EventStateManager::PostHandleEvent(nsPre
>         case WheelPrefs::ACTION_NONE:
>         default:
>+          *aStatus = nsEventStatus_eIgnore;

I think that this is wrong. *aStatus may be nsEventStatus_ConsumeDoDefault or nsEventState_eIgnore here. So, you shouldn't touch this.

>@@ -2198,24 +2199,40 @@ bool
> TabChild::RecvMouseWheelEvent(const WidgetWheelEvent& aEvent,
>                               const ScrollableLayerGuid& aGuid,
>                               const uint64_t& aInputBlockId)
> {
>   if (gfxPrefs::AsyncPanZoomEnabled()) {
>     nsCOMPtr<nsIDocument> document(GetDocument());
>     APZCCallbackHelper::SendSetTargetAPZCNotification(WebWidget(), document, aEvent, aGuid,
>         aInputBlockId);
>+    WidgetWheelEvent event(aEvent);
>+    event.widget = mWidget;
>+    APZCCallbackHelper::DispatchWidgetEvent(event);
>+    mAPZEventState->ProcessWheelEvent(event, aGuid, aInputBlockId);
>+    return true;
>   }
> 
>   WidgetWheelEvent event(aEvent);
>   event.widget = mWidget;
>-  APZCCallbackHelper::DispatchWidgetEvent(event);
>-
>-  if (gfxPrefs::AsyncPanZoomEnabled()) {
>-    mAPZEventState->ProcessWheelEvent(event, aGuid, aInputBlockId);
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  if (aEvent.message == NS_WHEEL_START) {
>+    mWidget->DispatchEvent(&event, status);
>+    mWheelState = WHEEL_STARTED;
>+  } else {
>+    if (mWheelState != WHEEL_IGNORED) {
>+      mWidget->DispatchEvent(&event, status);
>+    }
>+    if (mWheelState == WHEEL_STARTED) {
>+      mWheelState =
>+        status == nsEventStatus_eConsumeNoDefault ? WHEEL_CLAIMED : WHEEL_IGNORED;

If the delta value is too small and actual scroll amount is empty, this becomes WHEEL_IGNORED. So, I think that this may kill very slow scrolling with high resolution scroll devices.

>+    }
>+  }

Hmm, when aEvent.message == NS_WHEEL_STOP, shouldn't mWheelState be back to WHEEL_UNTRACKED? Don't forget this scenario: user may use two or more pointing devices one of them supports gesture like trackpad another is a simple mouse. Then, handling NS_WHEEL_STOP must be important when the user switches from trackpad to mouse.

>+    /**
>+     * Scrollwheel events may be handled by the content in this process
>+     * or they may be handled as gestures by a parent.  We make
>+     * this decision when the first NS_WHEEL_MOVE event is received --

NS_WHEEL_WHEEL?

>+     * (NS_WHEEL_START has no directional information).  WheelEventState
>+     * tracks where we are in the wheel event stream.
>+     */
>+    enum WheelEventState {
>+      /**
>+       * We are not tracking claims on the wheel and do not bounce
>+       * events back to our parent.  For platforms that do not
>+       * support NS_WHEEL_START/NS_WHEEL_STOP events, we stay in
>+       * this state.
>+       */
>+      WHEEL_UNTRACKED,
>+     /**
>+      * We are tracking claims and the last event was an NS_WHEEL_START.
>+      */
>+      WHEEL_STARTED,
>+     /**
>+      * We have claimed the current sequence of events.
>+      */
>+      WHEEL_CLAIMED,

CLAIMED doesn't sounds natural for me. How about WHEEL_HANDLED?

>+bool TabParent::RecvMouseWheelEvent(const mozilla::WidgetWheelEvent& aEvent,
>+                                    const ScrollableLayerGuid& aGuid,
>+                                    const uint64_t& aInputBlockId)
>+{
>+  if (mIsDestroyed) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  WidgetWheelEvent localEvent(aEvent);
>+  localEvent.widget = widget;
>+  localEvent.refPoint -= GetChildProcessOffset();
>+  widget->TrackWheelEventAsGesture(&localEvent);

The order style name feels odd to me. How about OnWheelEventIgnoredInRemote()? or just OnEventIgnoredInRemove()? Then, we can use the interface method for other events too.

>diff --git a/widget/MouseEvents.h b/widget/MouseEvents.h
>--- a/widget/MouseEvents.h
>+++ b/widget/MouseEvents.h
>@@ -449,16 +449,17 @@ public:
>     , isMomentum(false)
>     , mIsNoLineOrPageDelta(false)
>     , lineOrPageDeltaX(0)
>     , lineOrPageDeltaY(0)
>     , scrollType(SCROLL_DEFAULT)
>     , overflowDeltaX(0.0)
>     , overflowDeltaY(0.0)
>     , mViewPortIsOverscrolled(false)
>+    , mAsyncHandled(false)

So, might be better this to be defined in WidgetEvent. But we should define it here for now.



And I think that I'm not a good person to review widget/cocoa part, please ask review to smichaud too.

>+- (void)maybeTrackAsyncScrollEventAsSwipe:(mozilla::WidgetWheelEvent*)aWheelEvent
>+                          scrollOverflowX:(double)anOverflowX
>+                          scrollOverflowY:(double)anOverflowY
>+                   viewPortIsOverscrolled:(BOOL)aViewPortIsOverscrolled
>+{
>+  ScrollEventMap::iterator itEvent =
>+    mPotentialGestureEvents.find(std::make_pair(aWheelEvent->message, aWheelEvent->time));
>+  NSEvent *aEvent = itEvent != mPotentialGestureEvents.end() ? itEvent->second : nullptr;

aEvent sounds like an argument.

>+  if (!aEvent) {
>+    // Sometimes we get events with the same time stamp and the later
>+    // overwrites the earlier. We allow the earlier event to be collected.
>+    // In this case, we do nothing.
>+    return;

Don't you need to remove the event from mPotentialGestureEvents?

>+  }
>+
>+  mPotentialGestureEvents.erase(itEvent);
>+
>+  if (mTrackingGesture) {
>+    // This event is part of a motion that we have already begun tracking
>+    // asynchronously.
>+    if (nsCocoaUtils::EventPhase(aEvent) != NSEventPhaseBegan) {
>+      // If Cocoa is asynchronously tracking a gesture for us then we
>+      // wheel events sent to NSApp are adopted into that tracking.
>+      [NSApp sendEvent:aEvent];
>+    }
>+    [aEvent release];
>+    return;
>+  }
>+
>+  // Remote content "claims" entire gestures, which avoids the case where
>+  // a scroll can turn into a gesture at the end of the motion.  For that
>+  // reason, we can allow any event to initiate a horizontal gesture.
>+  [self maybeTrackScrollEventAsSwipe:aEvent
>+                     scrollOverflowX:anOverflowX
>+                     scrollOverflowY:anOverflowY
>+              viewPortIsOverscrolled:aViewPortIsOverscrolled
>+                     allowHorizontal:YES];
>+
>+  [aEvent release];

Hmm, looks like releasing aEvent manually is risky. How about to create a stack class which removes the find entry from mPotentialGestureEvents in its constructor and releases the event in its destructor?

>+- (void)cacheWheelEvent:(NSEvent*)aEvent forMessage:(uint32_t)aMsg andTime:(uint64_t)aTime
>+{
>+  ScrollEventKey key = std::make_pair(aMsg, aTime);
>+  // If we receive two events of the same type at the same time for whatever
>+  // reason then make sure we don't leak the previous one.
>+  if (mPotentialGestureEvents[key]) {
>+    [mPotentialGestureEvents[key] release];

Why don't you just ignore current event?

>+  }
>+  mPotentialGestureEvents[key] = aEvent;
>+  [aEvent retain];
>+}
>   if (phase & NSEventPhaseMayBegin) {
>+    mLastWheelMsgWasMayBegin = true;
>     [self sendWheelCondition:YES first:NS_WHEEL_STOP second:NS_WHEEL_START forEvent:theEvent];
>     return;
>   }
> 
>+  // If event is beginning, make sure we fire a START (preceeded by a STOP
>+  // if we haven't sent one since the last START) before we send a motion event.
>+  if ((phase & NSEventPhaseBegan) && !mLastWheelMsgWasMayBegin) {

Can be |else if| ?

>-  if ((wheelEvent.deltaMode == nsIDOMWheelEvent::DOM_DELTA_PIXEL) &&
>+  // If the event is being considered by another process then we
>+  // cannot make this determination yet.
>+  if (!wheelEvent.mAsyncHandled &&
>+      (wheelEvent.deltaMode == nsIDOMWheelEvent::DOM_DELTA_PIXEL) &&
>       (wheelEvent.deltaX != 0.0 || wheelEvent.deltaY != 0.0)) {
>+    // If the event is not an NSEventPhaseBegan event then don't
>+    // allow it to initiate a horizontal gesture.
>     [self maybeTrackScrollEventAsSwipe:theEvent
>                        scrollOverflowX:wheelEvent.overflowDeltaX
>                        scrollOverflowY:wheelEvent.overflowDeltaY
>-                viewPortIsOverscrolled:wheelEvent.mViewPortIsOverscrolled];
>+                viewPortIsOverscrolled:wheelEvent.mViewPortIsOverscrolled
>+                       allowHorizontal:(phase == NSEventPhaseBegan)];

Other parts checks the phase with &. Why here you check it with ==?

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -2080,16 +2080,24 @@ class nsIWidget : public nsISupports {
>      * Cancels all active simulated touch input points and pending long taps.
>      * Native widgets should track existing points such that they can clear the
>      * digitizer state when this call is made.
>      * @param aObserver The observer that will get notified once the touch
>      * sequence has been cleared.
>      */
>     virtual nsresult ClearNativeTouchSequence(nsIObserver* aObserver);
> 
>+    /**
>+     * On multiprocess Mac, gestures are recognized by the OS.  It needs to be
>+     * notified when the child process decided not to handle a scroll wheel
>+     * event, so that the OS can adopt it.  This is a kludge, intended to
>+     * allow gestures while APZ is being completed.
>+     */
>+    virtual void TrackWheelEventAsGesture(mozilla::WidgetWheelEvent* aEvent) {}

As I mentioned above, I don't like this method name. And also you need to update uuid.


Honestly, I understand this patch not perfectly. So, if my review doesn't make sense, let me know the points.

Comment 40

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

Comment on attachment 8608972 [details] [diff] [review]
2. Make test_wheel_scroll.html pass

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

Mostly fine, just a few comments below but I'd like to see the updated version. Also Markus should probably review the nsChildView changes.

::: dom/ipc/TabChild.cpp
@@ +2210,5 @@
>      mAPZEventState->ProcessWheelEvent(event, aGuid, aInputBlockId);
>      return true;
>    }
>  
> +  printf_stderr("TabChild::RecvMouseWheelEvent\n");

remove all printfs

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +37,5 @@
>  // aX and aY are relative to |window|'s top-left. aDeltaX and aDeltaY
>  // are pixel deltas, and aObserver can be left undefined if not needed.
> +// If (aDeltaX, aDeltaY) == (0,0) then aIsStart indicates if this should be
> +// a wheel-start or wheel-stop event.
> +function synthesizeNativeWheel(aElement, aX, aY, aDeltaX, aDeltaY, aObserver, aIsStart) {

I think I'd prefer a separate function to deal with the start/stop behaviour, rather than jamming it into this function. Particularly since it only makes sense in the observer version and not the event version. The "separate function" is just for this JS helper file, you can still use the same sendNativeMouseScroll call to the C++ layer.

::: gfx/layers/apz/test/test_wheel_scroll.html
@@ +79,5 @@
>    var content = document.getElementById('content');
> +  var utils = SpecialPowers.wrap(window)
> +                           .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
> +                           .getInterface(SpecialPowers.Ci.nsIDOMWindowUtils);
> +  var scale = utils.screenPixelsPerCSSPixel;

Instead of all this you can just do:
var scale = window.devicePixelRatio which is the same thing and doesn't need SpecialPowers

@@ +82,5 @@
> +                           .getInterface(SpecialPowers.Ci.nsIDOMWindowUtils);
> +  var scale = utils.screenPixelsPerCSSPixel;
> +  var rect = content.getBoundingClientRect();
> +  var x = (rect.left+100)*scale;
> +  var y = (rect.top+150)*scale;

nit: spaces around operators

::: widget/cocoa/nsChildView.mm
@@ +1212,5 @@
> +    if (aNativeMessage == NSEventPhaseBegan) {
> +      printf_stderr("Sending START\n");
> +      [(ChildView*)mView sendWheelCondition:YES first:NS_WHEEL_STOP second:NS_WHEEL_START forEvent:nsEvent];
> +    } else {
> +      printf_stderr("Sending STOP\n");

More printfs

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8608969 [details] [diff] [review]
1. Implement gesture workaround for Mac

(I'll take a look at this once the comments from masayuki have been addressed)

Comment 43

[David Parks [:handyman]]

Attachment: 1. Implement gesture workaround for Mac

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> Comment on attachment 8608969 [details] [diff] [review]
> <a lot of stuff about the EventStateManager changes>

I'm not too clear on what is going on in the ESM code but I was never happy changing it for this patch, which is supposed to be a self-contained work-around, anyway.  The solution here doesn't require any changes to the ESM --  the TabChild's test is based on the reported overflowDeltaX/Y instead of using the consumeDefault response or viewportIsOverscrolled.

> If the delta value is too small and actual scroll amount is empty, this
> becomes WHEEL_IGNORED. So, I think that this may kill very slow scrolling
> with high resolution scroll devices.

OK, done.
The state transition logic in that method was getting to be pretty confusing so I switched it to a simpler style but the behavior is basically the same.

> 
> >+    }
> >+  }
> 
> Hmm, when aEvent.message == NS_WHEEL_STOP, shouldn't mWheelState be back to
> WHEEL_UNTRACKED? Don't forget this scenario: user may use two or more
> pointing devices one of them supports gesture like trackpad another is a
> simple mouse. Then, handling NS_WHEEL_STOP must be important when the user
> switches from trackpad to mouse.

Hadn't thought of that.  Done.

> And I think that I'm not a good person to review widget/cocoa part, please
> ask review to smichaud too.

Done.
 
> Hmm, looks like releasing aEvent manually is risky. How about to create a
> stack class which removes the find entry from mPotentialGestureEvents in its
> constructor and releases the event in its destructor?

Sure.  Done.
 
> >+- (void)cacheWheelEvent:(NSEvent*)aEvent forMessage:(uint32_t)aMsg andTime:(uint64_t)aTime
> >+{
> >+  ScrollEventKey key = std::make_pair(aMsg, aTime);
> >+  // If we receive two events of the same type at the same time for whatever
> >+  // reason then make sure we don't leak the previous one.
> >+  if (mPotentialGestureEvents[key]) {
> >+    [mPotentialGestureEvents[key] release];
> 
> Why don't you just ignore current event?

My reasoning was that the latest event is most likely to have the latest information, which is the only information I care about.  Of course, the OS is giving us nonsense here so I doubt it really matters either way.

Comment 44

[David Parks [:handyman]]

Attachment: 2. Make test_wheel_scroll.html pass

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> Comment on attachment 8608972 [details] [diff] [review]
> 2. Make test_wheel_scroll.html pass
>
> Mostly fine, just a few comments below but I'd like to see the updated
> version. Also Markus should probably review the nsChildView changes.
> 
> I think I'd prefer a separate function to deal with the start/stop
> behaviour, rather than jamming it into this function. Particularly since it
> only makes sense in the observer version and not the event version. The
> "separate function" is just for this JS helper file, you can still use the
> same sendNativeMouseScroll call to the C++ layer.

Agreed.  Done.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c9753923f80

Comment 45

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

Comment on attachment 8613616 [details] [diff] [review]
2. Make test_wheel_scroll.html pass

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +48,5 @@
>  interface nsIJSRAIIHelper;
>  interface nsIContentPermissionRequest;
>  interface nsIObserver;
>  
> +[scriptable, uuid(9cb233ea-5aea-4e5f-9243-6fa9eaae2004)]

Not sure you actually need to bump the uuid for a comment-only change but it can't hurt I guess. (The alternative is to add "IGNORE IDL" in your commit message somewhere)

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +46,5 @@
> +  var msg;
> +  if (aDeltaX != 0) {
> +    msg = nativeHorizontalWheelEventMsg();
> +  } else if (aDeltaY != 0) {
> +    msg = nativeVerticalWheelEventMsg();

I'd prefer leaving this the way it was originally, so that you don't accidentally pass an undefined |msg| parameter.

@@ +57,5 @@
>  // Synthesizes a native mousewheel event and invokes the callback once the
>  // request has been successfully made to the OS. This does not necessarily
>  // guarantee that the OS generates the event we requested. See
>  // synthesizeNativeWheel for details on the parameters.
> +function synthesizeNativeWheelAndWaitForObserver(aElement, aX, aY, aDeltaX, aDeltaY, aCallback, aIsStart) {

remove aIsStart

@@ +65,5 @@
>          setTimeout(aCallback, 0);
>        }
>      }
> +  };  
> +  return synthesizeNativeWheel(aElement, aX, aY, aDeltaX, aDeltaY, observer, aIsStart);

ditto

@@ +76,5 @@
>    window.addEventListener("wheel", function wheelWaiter(e) {
>      window.removeEventListener("wheel", wheelWaiter);
>      setTimeout(aCallback, 0);
>    });
> +  return synthesizeNativeWheel(aElement, aX, aY, aDeltaX, aDeltaY, null);

remove null arg

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8613615 [details] [diff] [review]
1. Implement gesture workaround for Mac

>@@ -2221,25 +2222,62 @@ bool
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  mWidget->DispatchEvent(&event, status);
>+

following code a little bit hard to read. Could you use early return style? Because you don't do anything after the if/else if/else blocks.

>+  if (mWheelState == WHEEL_UNTRACKED) {
>+    if (event.message == NS_WHEEL_START) {
>+      mWheelState = WHEEL_STARTED;
>+      SendMouseWheelEvent(event, aGuid, aInputBlockId);
>+    }

So, |return true;|

>+  } else if (mWheelState == WHEEL_STARTED) {

Then, this can be just |if (mWheelState == WHEEL_STARTED) {|.

>+    MOZ_ASSERT(event.message != NS_WHEEL_START);
>+    if (event.message == NS_WHEEL_WHEEL) {
>+      // Claim the wheel motion only if this first move event was in a
>+      // direction we can scroll.  DispatchEvent will have set the
>+      // overflowDelta on the event if this is the case.  Otherwise,
>+      // return the wheel events to our parent for gesture recognition
>+      // until the  next wheel start.
>+      if (event.overflowDeltaX == aEvent.deltaX &&
>+          event.overflowDeltaY == aEvent.deltaY) {

Perhaps, creating new method in WidgetWheelEvent is easier to read, e.g.,

IsDeltaValuesConsumed() const
{
  return event.overflowDeltaX != aEvent.deltaX ||
         event.overflowDeltaY != aEvent.deltaY;
}

>+        mWheelState = WHEEL_IGNORED;
>+        SendMouseWheelEvent(event, aGuid, aInputBlockId);
>+      } else {
>+        mWheelState = WHEEL_HANDLED;
>+      }

|return true;| here.

>+    } else {

Then, you can remove this |else| and reduce following indent level.

>+      MOZ_ASSERT(event.message == NS_WHEEL_STOP);
>+      mWheelState = WHEEL_UNTRACKED;
>+      SendMouseWheelEvent(event, aGuid, aInputBlockId);
>+    }

|return true;| here.

>+  } else {

You can remove this |else| too.

>+    if (mWheelState == WHEEL_IGNORED) {
>+      SendMouseWheelEvent(event, aGuid, aInputBlockId);
>+    }
>+    if (event.message == NS_WHEEL_STOP) {
>+      mWheelState = WHEEL_UNTRACKED;
>+    }

I guess that there is a case |mWheelState == WHEEL_IGNORED && event.message == NS_WHEEL_STOP|. If not so, please use early return style.

>+    /**
>+     * Scrollwheel events may be handled by the content in this process
>+     * or they may be handled as gestures by a parent.  We make
>+     * this decision when the first NS_WHEEL_WHEEL event is received --
>+     * (NS_WHEEL_START has no directional information).  WheelEventState
>+     * tracks where we are in the wheel event stream.
>+     */
>+    enum WheelEventState {

nit:

enum WheelEventState
{

>@@ -243,16 +250,20 @@ typedef NSInteger NSEventGestureAxis;
> #endif
> 
>   // Whether this uses off-main-thread compositing.
>   BOOL mUsingOMTCompositor;
> 
>   // The mask image that's used when painting into the titlebar using basic
>   // CGContext painting (i.e. non-accelerated).
>   CGImageRef mTopLeftCornerMask;
>+
>+  BOOL mTrackingGesture;
>+  ScrollEventMap mPotentialGestureEvents;
>+  BOOL mLastWheelMsgWasMayBegin;

I'm not familiar with memory aligning of Objective-C, though, all BOOL should be defined in the end? Anyway, |BOOL mUsingOMTCompositor| is defined before another member... I'd like smichaud to check this.

>@@ -547,16 +563,18 @@ public:
>                             nsString& aCommitted) override;
> 
>   NS_IMETHOD SetPluginFocused(bool& aFocused) override;
> 
>   bool IsPluginFocused() { return mPluginFocused; }
> 
>   virtual nsIntPoint GetClientOffset() override;
> 
>+  virtual void OnEventIgnoredInRemote(mozilla::WidgetWheelEvent* aEvent) override;

Looks like over 80 chars.

virtual void OnEventIgnoredInRemote(
               mozilla::WidgetWheelEvent* aEvent) override;

>+class AutoReleaseScrollEvent

class MOZ_STACK_CLASS AutoReleaseScrollEvent final

>+{
>+  NSEvent *mEvent;

nit: NSEvent* mEvent;

>+public:
>+  AutoReleaseScrollEvent(uint32_t aMsg, uint64_t aTime, ScrollEventMap* events) {

Over 80 chars, perhaps,

AutoReleaseScrollEvent(uint32_t aMsg,
                       uint64_t aTime,
                       ScrollEventMap* aEvents)
{

and |events| should be |aEvents|.


>+    ScrollEventMap::iterator itEvent = events->find(std::make_pair(aMsg, aTime));

ditto, perhaps,

ScrollEventMap::iterator itEvent =
  events->find(std::make_pair(aMsg, aTime));

>+    if (itEvent != events->end()) {
>+      mEvent = itEvent->second;
>+      events->erase(itEvent);
>+    } else {
>+      mEvent = nullptr;
>+    }
>+  }
>+  ~AutoReleaseScrollEvent() {

nit:

~AutoReleaseScrollEvent()
{

>+    if (mEvent) {
>+      [mEvent release];
>+    }
>+  }
>+  NSEvent *GetEvent() {

nit:

NSEvent* GetEvent()
{

>+- (void)maybeTrackAsyncScrollEventAsSwipe:(mozilla::WidgetWheelEvent*)aWheelEvent

Do you really need |mozilla::| here??

>+                          scrollOverflowX:(double)anOverflowX
>+                          scrollOverflowY:(double)anOverflowY
>+                   viewPortIsOverscrolled:(BOOL)aViewPortIsOverscrolled
>+{
>+  AutoReleaseScrollEvent autoRelease(aWheelEvent->message, aWheelEvent->time,
>+                                     &mPotentialGestureEvents);

FYI: You can set WidgetWheelEvent* instead of message and time, but up to you.

>+  NSEvent *event = autoRelease.GetEvent();

nit: NSEvent* event =...

And following logic should be reviewed by smichaud, if it were not his area, I'd be back.

> - (void)handleAsyncScrollEvent:(CGEventRef)cgEvent ofType:(CGEventType)type
> {
>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -2080,16 +2080,24 @@ class nsIWidget : public nsISupports {
>      * Cancels all active simulated touch input points and pending long taps.
>      * Native widgets should track existing points such that they can clear the
>      * digitizer state when this call is made.
>      * @param aObserver The observer that will get notified once the touch
>      * sequence has been cleared.
>      */
>     virtual nsresult ClearNativeTouchSequence(nsIObserver* aObserver);
> 
>+    /**
>+     * On multiprocess Mac, gestures are recognized by the OS.  It needs to be
>+     * notified when the child process decided not to handle a scroll wheel
>+     * event, so that the OS can adopt it.  This is a kludge, intended to
>+     * allow gestures while APZ is being completed.
>+     */
>+    virtual void OnEventIgnoredInRemote(mozilla::WidgetWheelEvent* aEvent) {}

You need to update uuid and you need to request sr after r+ of smichaud.

Anyway, I still don't think that this should be used by wheel event forever. If somebody needs to same thing with other events, they will need to add similar methods!

So, I think that this should be WidgetGUIEvent* and Cocoa widget should check if the given event is WidgetWheelEvent. You can use WidgetEvent::AsWheelEvent() for that.


So, almost all of this looks okay to me. However, I'd like to check next patch again. So, temporarily marking this as r-, but not bad.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8613615 [details] [diff] [review]
1. Implement gesture workaround for Mac

I'd like to put off my review of this until Masayuki is done with it :-)

Comment 48

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8613615 [details] [diff] [review]
1. Implement gesture workaround for Mac

And to reduce confusion I'm going to clear the review request, for the time being.

Comment 50

[Markus Stange [:mstange]]

Comment on attachment 8613616 [details] [diff] [review]
2. Make test_wheel_scroll.html pass

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

Looks good to me, assuming kats' comments are addressed.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

handyman is away for the next while, so I'll try to drive this one over the line.

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX. r?masayuki

Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX

Until APZ is available, we need a way to allow OSX to send scrollwheel
events to remote content, and if that content does not claim the
scrollwheel event, send the event back to OSX for gesture-tracking.
This patch sends all content-targetted scrollwheel events to the
TabChild, which decides if it will claim the rest of the events.  If it
does not claim them then the events are sent back to the widget on in
the parent process, where it is used to either initiate tracking or is
simply sent to the initiated Obj-C block that is doing the tracking.
Once OSX has begun tracking, future events go directly to the Obj-C block instead
of to the NSWindow's usual event handler.

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

This test had a few issues that were exposed by this patch:
* We need to send (synthetic) WHEEL_START/WHEEL_STOP events
* Some imprecise math (additionally, it was not yet HiDPI-ready)

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623903 [details]
MozReview Request: Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX. r?masayuki

Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX

Until APZ is available, we need a way to allow OSX to send scrollwheel
events to remote content, and if that content does not claim the
scrollwheel event, send the event back to OSX for gesture-tracking.
This patch sends all content-targetted scrollwheel events to the
TabChild, which decides if it will claim the rest of the events.  If it
does not claim them then the events are sent back to the widget on in
the parent process, where it is used to either initiate tracking or is
simply sent to the initiated Obj-C block that is doing the tracking.
Once OSX has begun tracking, future events go directly to the Obj-C block instead
of to the NSWindow's usual event handler.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623904 [details]
MozReview Request: Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

This test had a few issues that were exposed by this patch:
* We need to send (synthetic) WHEEL_START/WHEEL_STOP events
* Some imprecise math (additionally, it was not yet HiDPI-ready)

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623903 [details]
MozReview Request: Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX. r?masayuki

Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX. r?masayuki

Until APZ is available, we need a way to allow OSX to send scrollwheel
events to remote content, and if that content does not claim the
scrollwheel event, send the event back to OSX for gesture-tracking.
This patch sends all content-targetted scrollwheel events to the
TabChild, which decides if it will claim the rest of the events.  If it
does not claim them then the events are sent back to the widget on in
the parent process, where it is used to either initiate tracking or is
simply sent to the initiated Obj-C block that is doing the tracking.
Once OSX has begun tracking, future events go directly to the Obj-C block instead
of to the NSWindow's usual event handler.

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623904 [details]
MozReview Request: Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

Bug 863514 - Repair test_wheel_scroll for new mac gesture code. r=mstange.

This test had a few issues that were exposed by this patch:
* We need to send (synthetic) WHEEL_START/WHEEL_STOP events
* Some imprecise math (additionally, it was not yet HiDPI-ready)

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Requesting review from mstange strictly on the maybeTrackAsyncScrollEventAsSwipe logic.

Hey masayuki - I've tried my best to address your review comments. Here is the interdiff: https://reviewboard.mozilla.org/r/11613/diff/1-3/

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8623903 [details]
MozReview Request: Bug 863514 - Implement a workaround for history gestures (horizontal swiping) on OSX. r?masayuki

https://reviewboard.mozilla.org/r/11613/#review10343

::: dom/ipc/TabChild.cpp:2230
(Diff revision 3)
> -  if (AsyncPanZoomEnabled()) {
> -    mAPZEventState->ProcessWheelEvent(event, aGuid, aInputBlockId);
> +  bool stopEvent = event.message == NS_WHEEL_STOP;
> +  bool wheelEvent = event.message == NS_WHEEL_WHEEL;
> +  bool startEvent = event.message == NS_WHEEL_START;

I don't like these variables since other developers need to check when they are true. Please check the message at each point.

::: widget/nsIWidget.h:2135
(Diff revision 3)
> +    /**
> +     * On multiprocess Mac, gestures are recognized by the OS.  It needs to be
> +     * notified when the child process decided not to handle a scroll wheel
> +     * event, so that the OS can adopt it.  This is a kludge, intended to
> +     * allow gestures while APZ is being completed.
> +     */
> +    virtual void OnEventIgnoredInRemote(mozilla::WidgetGUIEvent* aEvent) {}

I'd like you don't include specific reason on Mac OS X for the explanation of an interface method.

::: widget/MouseEvents.h:525
(Diff revision 3)
> +  bool IsDeltaValuesConsumed(const WidgetWheelEvent& aEvent)
> +  {
> +    return overflowDeltaX != aEvent.deltaX ||
> +           overflowDeltaY != aEvent.deltaY;
> +  }

Why do you need aEvent?

::: dom/ipc/TabChild.cpp:2260
(Diff revision 3)
> +      MOZ_ASSERT(event.message == NS_WHEEL_STOP);

I wonder if this is never hits. When user switches to mouse from trackpad, WHEEL_STOP is alwasy dispatched?

::: dom/ipc/TabChild.cpp:2266
(Diff revision 3)
> +    case WHEEL_HANDLED: {
> +      // Nothing to do - the content process is handling this
> +      // wheel event.
> +      break;
> +    }
> +
> +    case WHEEL_IGNORED: {
> +      SendMouseWheelEvent(event, aGuid, aInputBlockId);
> +      break;
> +    }

If NS_WHEEL_START is received when the state is one of them,  it occurs that you don't expect. I think that before this switch statement, shouldn't you initialize the state when you receive NS_WHEEL_START?

I wonder, if window is moved by JS during a gesture, NS_WHEEL_* events may not come this handler or may come this handler accidentally, isn't it?

::: widget/MouseEvents.h:527
(Diff revision 3)
> +    return overflowDeltaX != aEvent.deltaX ||
> +           overflowDeltaY != aEvent.deltaY;

Although, this code is suggested by me, does this work? I'm not sure if this works expectedlly when the double values are not integer.

Comment 61

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/11613/#review10661

> I wonder if this is never hits. When user switches to mouse from trackpad, WHEEL_STOP is alwasy dispatched?

I'm not sure if we dispatch a WHEEL_STOP when switching from mouse to trackpad... I'll find out today.

> Why do you need aEvent?

Turns out I don't. :)

> Although, this code is suggested by me, does this work? I'm not sure if this works expectedlly when the double values are not integer.

I don't think I understand. Can you come up with a specific case where this wouldn't work?

> If NS_WHEEL_START is received when the state is one of them,  it occurs that you don't expect. I think that before this switch statement, shouldn't you initialize the state when you receive NS_WHEEL_START?
> 
> I wonder, if window is moved by JS during a gesture, NS_WHEEL_* events may not come this handler or may come this handler accidentally, isn't it?

Can NS_WHEEL_START even arrive in that case? I would expect that NS_WHEEL_START can only arrive either as the very first wheel event, or after NS_WHEEL_STOP has already been fired, putting us in the WHEEL_UNTRACKED state. Is this not true?

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Ok, so a bit of an update here.

According to kats, some discussions took place in Whistler involving APZC. Specifically, the plan is to turn on APZC on OS X sooner than originally anticipated when this bug got started.

Turning on APZC changes the approach with which we can solve this bug.

mstange tells me that some of the technique in the current patch might be re-used, but that he can probably drive this one to completion with APZC on.

The bug to turn on APZC by default on OS X is bug 1157746.

Comment 63

[Markus Stange [:mstange]]

Small status update: Bug 1157746 landed, so at the moment horizontal scrolling works and swiping is completely broken. I'm currently implementing the plan in https://etherpad.mozilla.org/APZSwiping . I have it mostly working right now, I need to clean up my patches and check whether I'm introducing any test failures.

The phases of my patch stack are roughly:
 1. Make APZ on OS X use PanGestureInput events which carry useful gesture phase information: https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/widget/InputData.h#l257
 2. Implement the swipe animation physics outside of Cocoa and make it use PanGestureInput events, so that we don't have to hold on to NSEvents in the cases where we need to queue up events and process them asynchronously.
 3. Give nsChildView a queue of potential swipe events (similar to what the current patch does, but with PanGestureInput events), add a flag on WidgetWheelEvents for potential swipe start events, make TabChild report swipe start status to the parent process widget.
 4. Let APZ shortcut the wait for the child process if it has a confirmed scroll target for a potential swipe start event, so that we can start swiping before the child processes the wheel event.

Comment 64

[Andrea Guglielmi]

I hope it is the right bug...
After an update (I can't say which one) swipe left/right on OSX (10.11) stopped working.
I opened a thread on MozillaZine (http://forums.mozillazine.org/viewtopic.php?f=23&t=2951291) and after a while I received two suggestions: disable e10s or set layers.async-pan-zoom.enabled to false and both of them worked.
Now I kept e10s enabled and layers.async-pan-zoom.enabled to false.

Comment 65

[Markus Stange [:mstange]]

Fixed by bug 1016035. Everybody who disabled e10s or APZ (layers.async-pan-zoom.enabled) because of this bug can now re-enable it again. :-)

Comment 66

[ZZ]

I've never seen the rotate image gesture work on OSX with e10s enabled. Latest attempt on Firefox 50.0a2 and OSX 10.11. Has anyone checked this?

Comment 67

[Tracy Walker [:tracy]]

Back/Forward swipe works, albeit reverse of what feels right.  ie. two finger swipe right goes back in history, while two finger swipe left goes forward in history.  I feel like it should be the other way around, as that is the way the History buttons are pointing. However, Safari does the same as we do, opposite of what seems intuitive.

Is there a example of an image that "can" be rotated?  I don't see two finger rotate working on sample from bug 1108553; http://people.mozilla.org/~smartell/blog/20130627/1129-0.png

Comment 68

[Tracy Walker [:tracy]]

Ah, the sample image to two finger rotate does work in non-e10s.

I'm testing this latest Nightly 51.0a1, 20160810030202 on Mac OSX 10.11 

re-open this or the duped bug specifically about image rotation, bug 1108853?

Comment 69

[Stephen A Pohl [:spohl]]

(In reply to Tracy Walker [:tracy] from comment #67)
> Back/Forward swipe works, albeit reverse of what feels right.  ie. two
> finger swipe right goes back in history, while two finger swipe left goes
> forward in history.  I feel like it should be the other way around, as that
> is the way the History buttons are pointing. However, Safari does the same
> as we do, opposite of what seems intuitive.

This is intentional and conceptually matches flipping a page in a physical book: Moving a page from the left to the right goes back in the "history" of the book.

Comment 70

[Tracy Walker [:tracy]]

(In reply to Stephen A Pohl [:spohl] from comment #69)
> (In reply to Tracy Walker [:tracy] from comment #67)
> > Back/Forward swipe works, albeit reverse of what feels right.  ie. two
> > finger swipe right goes back in history, while two finger swipe left goes
> > forward in history.  I feel like it should be the other way around, as that
> > is the way the History buttons are pointing. However, Safari does the same
> > as we do, opposite of what seems intuitive.
> 
> This is intentional and conceptually matches flipping a page in a physical
> book: Moving a page from the left to the right goes back in the "history" of
> the book.

Yes, I understand it's meant to be this way.  I don't by default use this gesture, so when trying it to check this, it just felt odd  (maybe my dyslexicia? ;) ) Safari does better at giving the feel of book page turning by sliding the page off screen.

Comment 71

[Stephen A Pohl [:spohl]]

(In reply to Tracy Walker [:tracy] from comment #70)
> Safari does better at giving the feel of book page turning
> by sliding the page off screen.

You might be interested in bug 860493 in that case. :-)

Comment 72

[Markus Stange [:mstange]]

(In reply to Tracy Walker [:tracy] from comment #68)
> Ah, the sample image to two finger rotate does work in non-e10s.
> 
> I'm testing this latest Nightly 51.0a1, 20160810030202 on Mac OSX 10.11 
> 
> re-open this or the duped bug specifically about image rotation, bug 1108853?

Let's un-dupe bug 1108553. This bug is way too long already, and most of the discussion here was about back/forward swiping, which needed a completely different fix from bug 1108553.