673875 - [10.7] Reproduce bounce behaviour when reaching top and bottom of documents.

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Anthony Ricaud (:rik)]

Lion has this new bouncing behaviour when reaching the end of a document.

Comment 1

[Tobias Leingruber (@tbx)]

+1. This feels super nice in Chrome and Safari. Would love to see it in FF aswell.

Comment 2

[Asaf Romano (gone)]

FWIW, it's worth considering a non-native implementation (maybe even in scrollbox.xml).

Comment 3

[Mats Palmgren (inactive)]

Is this bug about implementing a rubber-band bounce back when you over-scroll
a document?  Please be aware that Apple has a patent on that behavior.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> Please be aware that Apple has a patent on that behavior.

Mats, do you have a link to documentation on this (or general instructions on how to find one)?

Comment 5

[Mats Palmgren (inactive)]

Sure, search for "Apple patent 381" should give you many links to such cases.
There are numerous cases against big competitors, Nokia, HTC, Samsung, Motorola,
to name a few, where they've won.  Here's a few links:
http://smedio.com/2011/08/09/scroll-back-and-bounce-apples-patent-position-no-381-to-break-iphone-rivals/
http://www.fosspatents.com/2011/08/apples-favorite-make-android-awkward.html
http://www.engadget.com/2010/03/02/apple-vs-htc-a-patent-breakdown/
http://www.phonearena.com/news/Apple-scores-a-win-against-Motorola-in-Germany-on-overscroll-bounce-patent_id27574

I am not a lawyer, so I may be wrong.  It may for example be OK to implement it
only on OSX and iOS if the APIs on those platforms have support for implementing
this feature.  Please consult a real lawyer first though.

Comment 6

[Mats Palmgren (inactive)]

On 10.7, it does appear that support for implementing this feature is available
in the APIs, see "Fluid Swipe Tracking - API (New since early 2011 seed)" in
https://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html

Comment 7

[Steven Michaud [:smichaud] (Retired)]

We've had basic swipe support in the tree since 668953 landed.  We don't yet have support for swipe animation (including bounces) -- that's bug 678392.  I can't begin to say whether those "bounces" (or these "bounces") are covered by the patent, or what it would mean if they were.

But I'm also inclined to think Apple's wouldn't have any trouble with app vendors implementing them on OS X.

I'll try to find someone from legal to CC on this bug.

Comment 8

[Mats Palmgren (inactive)]

> But I'm also inclined to think Apple's wouldn't have any trouble with app
> vendors implementing them on OS X.

Agreed, it's mostly other (non-Apple) platforms I'm worried about.  Unless the
platform vendor have a license from Apple (or we get one) for use of the feature
in apps on that platform, then I think we can't ship the feature for that platform.

Comment 9

[Mil]

This bug is only for Mac OS X, but really should only be for 10.7. Chrome has had this behaviour implemented before Lion was released to public, Firefox implementation has not even started and Lion has nearly been out for 1 year.
This bug is not for other OS's. I doubt anyone is surprised at the lack of development on this bug...

Comment 11

[José Jeria]

Bug 678392 has (horizontally) implemented this effect nicely when swiping to a page that is the last one to go back to.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Wip

This patch requires the latest patch for bug 678392 and 817700. Try build to follow.

Comment 13

[Mozilla RelEng Bot]

Try run for 6684b1a43038 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6684b1a43038
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6684b1a43038

Comment 14

[José Jeria]

Attachment: Movie showing the bounce effect

(In reply to Stephen Pohl [:spohl] from comment #12)
> Created attachment 698165 [details] [diff] [review]
> Wip
> 

I noticed the following using this bug's URL
* The bounce effect is too big, I can almost bounce the page to the bottom of the window. The effect is quite small in Safari.
* Sometimes the page reloads while triggering the bounce effect
* Dragging down with 2 fingers repetitively will result in a blank white page

Comment 15

[Nick]

Why not just implement the native scrolling libraries for this? Seems like that would address all the OS X related scrolling issues at the same time (auto-hiding scrollbars, bounce effect, etc.). No idea what the implications would be, but seems like that should be the "proper" way to implement this.

Comment 16

[Stephen A Pohl [:spohl]]

(In reply to Nick from comment #15)
> Why not just implement the native scrolling libraries for this?

A lot of this is already handled natively. Are you referring to something specifically (like a library) that we aren't using yet?

Comment 17

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback from comment 14 and reduced flickering.

Comment 18

[Mozilla RelEng Bot]

Try run for b6788179339b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b6788179339b
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b6788179339b

Comment 19

[José Jeria]

(In reply to Mozilla RelEng Bot from comment #18)
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> b6788179339b

Seems to work a bit better now. Though I was able to trigger the following errors:
* The bounce effect gets stuck sometimes. I simply repetitively dragged the page down to trigger this. No URL, nor form elements will react to clicks when the page is stuck in the bounce. Reloading or dragging the page back to its normal state will solve this.
* Sometimes the page does not scroll down when it should, it will instead bounces like it would be at the end of the page. Steps to reproduce:

1) go to the top of a page
2) drag down the page to trigger the bounce effect several times
3) now quickly drag in the opposite direction. The result will be that the bottom of the page will bounce instead of scrolling down

Let me know if you want me to upload a video showing these issues.

Comment 20

[Ben]

First, just want to say great work so far, this is looking promising.

I've noticed that in the current implementation of bounce, when scrolling to the end or beginning, there is still an abrupt hard stop when you scroll to a boundary. If you try out Safari, when it hits the boundary it will scroll slightly past the boundary for a split second and then bounce back to it's resting position. This way a user is instantly sure they have hit a boundary. In the try build it's like hitting a break wall. I find I scroll twice, once to the edge, and then one more time to make sure I'm at the true end.

Comment 21

[Stephen A Pohl [:spohl]]

Thanks Ben.

When you're running into the issue that you describe, are you referring to vertical or horizontal swiping? This bug here deals with vertical swiping. Bug 678392 deals with horizontal swiping (back/forward in history).

Comment 22

[Ben]

Vertical is the issue.

Comment 23

[Stephen A Pohl [:spohl]]

Ah, right. We're currently differentiating between scrolling and swiping and they don't mix very well. As you noticed, scrolling has to come to an end before we can switch to swiping and make the page bounce. This also explains the second issue reported by Jose in comment 19. I'm still looking into a few ways to improve this.

Comment 24

[Stephen A Pohl [:spohl]]

To expand a bit on my previous comment: we would only want to mix scrolling and swiping in the vertical swiping use case. Horizontally, we would actually want to hit the "brick wall" like Ben called it at the end of a scroll. Otherwise, we would start to swipe to the next or previous page when we only intended to scroll to the right or left of the page. We would also be unable to quickly swipe back and forth between pages whenever a page has a horizontal scroll bar.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

If you feel up to it, Stephen, you might want to break out class-dump and gdb and reverse-engineer how Safari handles this :-)

That's how I discovered [NSEvent trackSwipeEventWithOptions:...], and how to use it.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Likewise with Chrome, for which you actually have source code (for Chromium).

But to use gdb for this you'll have to build a debug version of Chromium -- Chrome has all its symbols stripped.

Comment 27

[Stephen A Pohl [:spohl]]

Thanks for the pointers, Steven! I'm looking over our event handling in nsChildView.mm and a solution may be obvious once I have a better understanding of it. If not, it'll still be helpful to understand what we're doing before reverse-engineering other browsers.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Sounds good to me.

Comment 29

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed bounce effect getting stuck sometimes (reported by Jose in comment 19).
Fixed quick switching between vertical and horizontal swiping. If a vertical swipe was being processed, a swipe forward would not display the swipe animation anymore.

Looking into the fast switching of swipe to scroll now (second issue reported in comment 19).

Comment 30

[Mozilla RelEng Bot]

Try run for 8cea2f530ca5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8cea2f530ca5
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8cea2f530ca5

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch allows for switching between vertical scroll and swiping (bounce effect) without a hard stop (reported in comment 20). Also made some general changes to our event handling that should allow for the remaining issues to be resolved more easily.

Not yet implemented:
1. Bounce effect with momentum events, i.e. the user 'flicks' to the top or bottom of the page but no fingers are left on the trackpad when the page actually reaches the top or bottom.
2. Scroll-to-swipe-to-scroll. For example, scrolling to the top of the page until the bounce effect appears and scrolling back down in one motion does not scroll the page down yet.
3. Issue reported in comment 19.
4. When swiping to the top or bottom of the page quickly, the bounce effect starts with an incorrect snapshot, i.e. the page is slightly cut at the top or bottom. I will need to ensure that we've scrolled to the top or bottom of the page before starting the bounce effect.

Comment 32

[Mozilla RelEng Bot]

Try run for b23060443dab is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b23060443dab
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b23060443dab

Comment 33

[José Jeria]

(In reply to Mozilla RelEng Bot from comment #32)
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> b23060443dab

Verified that the bounce effect no longer gets stuck with this release. Looking good.

Comment 34

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch implements scroll-to-swipe-to-scroll. For example, scrolling to the top of the page until the bounce effect appears and scrolling back down in one motion will now scroll the page down after the bounce animation completes.

Comment 35

[Mozilla RelEng Bot]

Try run for d765f270f28d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d765f270f28d
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d765f270f28d

Comment 36

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch fixes the most important two of the three remaining issues:
1. When swiping to the top or bottom of the page quickly, the bounce effect used to start with an incorrect snapshot, i.e. the page was slightly cut at the top or bottom. We scroll to the top or bottom of the page before starting the bounce effect now.
2. Fixed issue reported in comment 19.

I'm still looking into ways to use momentum events for the bounce effect.

I've tried to avoid any regressions in the fix for bug 678392 (horizontal swiping). If you're testing out the next try build, it would be great if you could verify that horizontal (history) swiping is still working as expected.

Comment 37

[Mozilla RelEng Bot]

Try run for 47a4b87e3a43 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=47a4b87e3a43
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-47a4b87e3a43

Comment 38

[Josiah Bruner [:jsbruner]]

There is something I am curious about and have been curious about in the past. Someone mentioned it briefly here. Why aren't we using native API's for this? This would solve all kinds of issues.

This API, for all the non-objective C developers here, is the NSScrollView. 

https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSScrollView_Class/Reference/Reference.html

This API would fix our scrollbar issue (Bug 636564), as well as this bug. It brings the "bounce" effect, inertial scrolling, and the hiding scrollbar. 

Is there a reason we can't use this API? Is it just too difficult to add?

Also, there is an issue with this right now, unrelated to my above thoughts. When you are inside of a form field (Like the one I am typing in now) and it has scroll contents, scrolling to past the top causes the bounce effect, however it takes you all the way to the top of the page. I am assuming this also happens if you scroll to the bottom and you are not already at that position. There should be no bouncing at all when this happens.

But good job so far!

Comment 39

[Steven Michaud [:smichaud] (Retired)]

> Why aren't we using native API's for this? This would solve all
> kinds of issues.
>
> This API, for all the non-objective C developers here, is the
> NSScrollView.

If you look at the source code for Cocoa widgets (especially at
widget/cocoa/nsChildView.* and widget/cocoa/nsCocoaWindow.*) you'll
see that we don't use *any* native UI objects (like NSScrollView).
Every single control is implemented in cross-platform code (aka Gecko
code).  Even the so-called "native" controls are implemented this way.

This is true on all platforms, and goes way back in Firefox history
(perhaps to the very beginning).  The idea is that we want Firefox to
look and work the same on all platforms.  We fudge this a bit by
giving some controls a "native look" -- but even that is never done
using native objects.

This allows us greater control over our UI -- for example we can do
things that'd be impossible using native objects.  But it also makes
our lives more difficult:  For example when a new version of OS X
comes out with new UI features (like horizontal swiping and HiDPI
mode), it's a lot more work for us to implement them than it is for
purer Cocoa apps like Safari and Chrome.

You can argue the merits of this ... though this bug is *definitely*
not the place to do that.  But the decision was made long ago, and is
deeply embedded in Mozilla code.  So it's not going to change any time
soon, if ever.

Comment 40

[Josiah Bruner [:jsbruner]]

(In reply to Steven Michaud from comment #39)

Thanks. That is what I figured.

Comment 41

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated patch to work with the latest patch in bug 678392. No functional changes.

Comment 42

[Stephen A Pohl [:spohl]]

Attachment: Patch

Cleanup. No functional changes.

Comment 43

[Stephen A Pohl [:spohl]]

Attachment: Patch

Sending this for review. The following patches need to be applied to get full functionality (in this order):
1. bug817700
2. bug678392
3. bug678392-swipeInterface
4. bug678392-animationFix
5. bug673875

Not implemented are:
1. Triggering bounce effect based on momentum scroll events.
2. Issue reported at the bottom of comment 38.

I will either write follow-up patches for these issues, or split them out into new bugs.

Comment 44

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated patch for new patch in bug 678392.

Also implemented a fix for the issue reported at the bottom of comment 38: We only trigger the bounce effect when we're scrolling the page now. Scrolling text fields on the page no longer trigger the page to bounce.

Comment 45

[Stephen A Pohl [:spohl]]

Try build with latest patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5479fcf945a8/try-macosx64/firefox-21.0a1.en-US.mac.dmg

Comment 46

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk and new patch in bug 678392.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 709298 [details] [diff] [review]
Patch

I've started looking at this (mostly at the Cocoa parts, of course).

I haven't finished wrapping my head around it, but I do have one question:

On the face of it you seem to have removed my workaround for bug 770626 (by getting rid of geckoSwipeEventSent).  Was this deliberate, and if so did you check that bug 770626 won't start happening again?

Comment 48

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 709298 [details] [diff] [review]
Patch

Another thing I'm scratching my head over.

Your current patch for bug 678392 has the following change to nsChildView.h (and related changes elsewhere):

-  BOOL *mSwipeAnimationCancelled;
+  void (^mCancelSwipeAnimation)();

But your current patch for this bug reverses it:

-  void (^mCancelSwipeAnimation)();
+  BOOL* mCancelSwipeAnimation;

Why are you doing this?

Comment 49

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #47)

Testing with the testcase from bug 770626, I find that this patch doesn't regress that bug, exactly.  But it does still cause some breakage:  For example "Stay on Page" in the testcase doesn't get obeyed -- you still leave the page.

Comment 50

[Stephen A Pohl [:spohl]]

(In reply to Steven Michaud from comment #48)
> Comment on attachment 709298 [details] [diff] [review]
> Patch
> 
> Another thing I'm scratching my head over.
> 
> Your current patch for bug 678392 has the following change to nsChildView.h
> (and related changes elsewhere):
> 
> -  BOOL *mSwipeAnimationCancelled;
> +  void (^mCancelSwipeAnimation)();
> 
> But your current patch for this bug reverses it:
> 
> -  void (^mCancelSwipeAnimation)();
> +  BOOL* mCancelSwipeAnimation;
> 
> Why are you doing this?

The patch for bug 678392 used to have more functionality in ^mCancelSwipeAnimation when I took over the bug and I slowly removed most of it until it was essentially equivalent to using BOOL* mCancelSwipeAnimation. Because the patch for bug 678392 was already under review when I started the work on this patch here, I didn't retrofit it. I think BOOL* mCancelSwipeAnimation is the way to go though.

Comment 51

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #50)
> (In reply to Steven Michaud from comment #48)

Another place where you can see such a change is in updateAnimation() in browser.js. Since I'm passing an additional flag (NSEventSwipeTrackingClampGestureAmount) to trackSwipeEventWithOptions in nsChildView.mm, I no longer need to clamp aVal in updateAnimation to -1 <= aVal <= 1. Because the patch for bug 678392 was already under review, I didn't retrofit this either. I hope this makes sense.

Comment 52

[Steven Michaud [:smichaud] (Retired)]

> I think BOOL* mCancelSwipeAnimation is the way to go though.

This is fine with me.  I think either approach would work.  I just wanted to make sure the change was deliberate, and had reasons.

Comment 53

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #51)

Makes sense.  And thanks for explaining -- I would also have asked about your no longer clamping aVal if I'd been paying more attention to the non-Cocoa parts of your patch.

I think it's always better to explain too much than too little :-)

Only the most important stuff need be included in code comments.  But it's good to have a fuller story in bug comments, where people can track it down if they feel the need.

Comment 54

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for the latest patch in bug 678392. The main change in bug 678392 was that gHistorySwipeAnimation and gGestureSupport were moved from browser.js to browser-gestureSupport.js. No functional changes in this patch.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 711914 [details] [diff] [review]
Patch

Stephen, you still need to deal with the problem I discussed in comment #47 and comment #49.

Comment 56

[Stephen A Pohl [:spohl]]

(In reply to Steven Michaud from comment #55)
> Comment on attachment 711914 [details] [diff] [review]
> Patch
> 
> Stephen, you still need to deal with the problem I discussed in comment #47
> and comment #49.

Looking at it as we speak. :-) I wanted to double-check my answers before replying.

Comment 57

[Gervase Markham [:gerv]]

Please re-CC me if you need me again :-)

Gerv

Comment 58

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch fixes an oversight that comment 47 and 49 made me aware of: geckoSwipeEventSent was removed intentionally, since our event handling now deals with reentrant calls with NSEventPhaseEnded correctly. This is done by checking the gestureAmount on each call of the callback. If it is equal to 0.0, we cancel the swipe animation. However, I must only have verified the case where "leave the page" is clicked in the modal dialog of bug 770626. In the case of "stay on page" the animation overlay would remain on top of the page because "stay on page" wouldn't trigger a pageshow event. The result was that the user seemed to go back in the browser history, but he wouldn't actually be able to interact with the page (clicking on buttons, entering text in text fields etc.). We now listen for "DOMModalDialogClosed" events and stop the animation when it occurs.

There are two other minor changes in this patch:
1. Removed a comment in nsChildView.mm that referred to geckoSwipeEventSent which is no longer present.
2. Small improvement in HSA_handleEvent: There is no need to call isAnimationRunning() before calling stopAnimation() for case "pageshow" and "popstate".

Comment 59

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 712088 [details] [diff] [review]
Patch

This looks fine to me, but I have some nits.

You should get someone else to review the JavaScript parts -- maybe felipec again.  I only looked carefully at the Cocoa widgets parts.

+      if (mCancelSwipeAnimation && *mCancelSwipeAnimation == NO) {
+        // If a swipe is currently being tracked kill it -- it's been interrupted
+        // by another gesture or legacy scroll wheel event.
+        *mCancelSwipeAnimation = YES;
+        mCancelSwipeAnimation = nil;
+        [self sendSwipeEndEvent:anEvent allowedDirections:0];
+      }

+  if (mCancelSwipeAnimation && *mCancelSwipeAnimation == NO) {
+    // If a swipe is currently being tracked kill it -- it's been interrupted
+    // by another gesture or legacy scroll wheel event.
+    *mCancelSwipeAnimation = YES;
+    mCancelSwipeAnimation = nil;
+  }

The comment should go outside the "if (mCancelSwipeAnimation ...)" block.  And in both cases the last part of the comment should be dropped ("or legacy scroll wheel event").

+    // Since this tracking handler can be called asynchronously, mGeckoChild
+    // might have become NULL here (our child widget might have been
+    // destroyed).
+    if (animationCanceled || !mGeckoChild || gestureAmount == 0.0) {

You should add something to the comment to explain that checking for gestureAmount == 0.0 works around bug 770626, which happens when DispatchWindowEvent() triggers a modal dialog, which spins the event loop and confuses the OS, which makes several re-entrant calls to this handler.

Comment 60

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback. Sending to Felipe for a review of the JS pieces.

Comment 61

[Stephen A Pohl [:spohl]]

Attachment: Patch

Removed trailing white space.

Comment 62

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 713177 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +187,5 @@
>      if (canGoForward)
>        aEvent.allowedDirections |= isLTR ? aEvent.DIRECTION_RIGHT :
>                                            aEvent.DIRECTION_LEFT;
>  
> +    let isVerticalSwipe = false;

instead of keeping this information in this variable and then passing it around as a paramenter, why not store it as a property in gHistorySwipeAnimation?

@@ +188,5 @@
>        aEvent.allowedDirections |= isLTR ? aEvent.DIRECTION_RIGHT :
>                                            aEvent.DIRECTION_LEFT;
>  
> +    let isVerticalSwipe = false;
> +    let window = gBrowser.contentWindow;

a shorthand in the scope of browser.js for `gbrowser.contentWindow` is `content`, so you can use that below instead of creating this variable

@@ +578,2 @@
>      if (this.isAnimationRunning()) {
> +      if (!aIsVerticalSwipe || this._lastSwipeDir != "") {

can you add a comment explaining this if condition?

@@ +1032,5 @@
> +    }
> +    finally {
> +      img.src = url;
> +      return img;
> +    }

just curious if this is now needed due to this patch or just improving this code?

::: widget/cocoa/nsChildView.mm
@@ +3222,5 @@
> +    PRUint32 allowedDirectionsCopy = allowedDirections;
> +    // Since this tracking handler can be called asynchronously, mGeckoChild
> +    // might have become NULL here (our child widget might have been
> +    // destroyed).
> +    // Checking for gestureAmount == 0.0 also works around bug 770626, which

bug 770626 is marked as fixed. is this comment still valid?

Comment 63

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed review feedback

Comment 64

[Stephen A Pohl [:spohl]]

Comment on attachment 713177 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +1032,5 @@
> +    }
> +    finally {
> +      img.src = url;
> +      return img;
> +    }

Just improving this code.

::: widget/cocoa/nsChildView.mm
@@ +3222,5 @@
> +    PRUint32 allowedDirectionsCopy = allowedDirections;
> +    // Since this tracking handler can be called asynchronously, mGeckoChild
> +    // might have become NULL here (our child widget might have been
> +    // destroyed).
> +    // Checking for gestureAmount == 0.0 also works around bug 770626, which

Checking for gestureAmount == 0.0 is the new (preferred) way of fixing bug 770626 (see comment 58 for more info).

Comment 65

[Stephen A Pohl [:spohl]]

The landing of bug 678392 has partially reintroduced bug 770626 for people who turn on the history swipe animation feature. A new fix will be checked in as part of the patch here (see comment 64).

Comment 66

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 732399 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +204,3 @@
>  
>      this._doUpdate = function GS__doUpdate(aEvent) {
> +      gHistorySwipeAnimation.updateAnimation(aEvent.delta, isVerticalSwipe);

the isVerticalSwipe parameter does not need to be passed anymore

Comment 67

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Removed isVerticalSwipe from function call. Carrying over r+.

Comment 68

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Reenable guard for bug 770626 when swipe animations are turned off

Reenables guard for bug 770626 when swipe animations are turned off. Also removes an assert that fires too often in legitimate scenarios (gestures interrupting other gestures).

Comment 69

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Reenable guard for bug 770626 when swipe animations are turned off

Forgot hg qrefresh.

Comment 70

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 737011 [details] [diff] [review]
Part 2: Reenable guard for bug 770626 when swipe animations are turned off

Looks fine to me.

Comment 71

[Stephen A Pohl [:spohl]]

Please note that the bounce behavior will be turned off as long as history swipe animations are turned off (bug 678392).

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/366e6a39d71a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6c7792b048

Comment 73

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/366e6a39d71a
https://hg.mozilla.org/mozilla-central/rev/fd6c7792b048

Comment 74

[Hawken Rives]

I think this patch is (sometimes) breaking scrolling on Google Spreadsheets. I'll keep looking for a reproducible testcase, but it's really annoying.

Comment 75

[Hawken Rives]

Also annoying is the way that the page become lower-resolution when you hit the bounce behavior. (both bugs on a retina MBP running 10.8.3)

Comment 76

[Stephen A Pohl [:spohl]]

(In reply to Hawken Rives from comment #75)
> Also annoying is the way that the page become lower-resolution when you hit
> the bounce behavior. (both bugs on a retina MBP running 10.8.3)

This is tracked as bug 836430.

Comment 77

[Recall]

I can't see how this is resolved as fixed? Am I missing a trick here because the bounce behaviour only happens if I manually drag past the end point. comment 20 describes it exactly. I am using Mountain Lion and MPB 15 2012. I have enabled snapshots.

Comment 78

[Stephen A Pohl [:spohl]]

(In reply to Recall from comment #77)
> I can't see how this is resolved as fixed? Am I missing a trick here because
> the bounce behaviour only happens if I manually drag past the end point.
> comment 20 describes it exactly. I am using Mountain Lion and MPB 15 2012. I
> have enabled snapshots.

See bug 836456.

Comment 79

[Recall]

(In reply to Stephen Pohl [:spohl] from comment #78)
> (In reply to Recall from comment #77)
> > I can't see how this is resolved as fixed? Am I missing a trick here because
> > the bounce behaviour only happens if I manually drag past the end point.
> > comment 20 describes it exactly. I am using Mountain Lion and MPB 15 2012. I
> > have enabled snapshots.
> 
> See bug 836456.

Ahh ok, thanks for clearing that up.

Comment 80

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 81

[Albert Scheiner [:alberts]]

Sorry if I asked something that's already answered, but I couldn't find anything in the comments above. I am on a Mac OS 10.7.5 and the latest Aurora, but I can't see any bounce effect when I scroll to the beginning or end of this page.
Do I have to set anything manually?

Comment 82

[Stephen A Pohl [:spohl]]

(In reply to albert from comment #81)
> Sorry if I asked something that's already answered, but I couldn't find
> anything in the comments above. I am on a Mac OS 10.7.5 and the latest
> Aurora, but I can't see any bounce effect when I scroll to the beginning or
> end of this page.
> Do I have to set anything manually?

The bounce behavior isn't turned on by default yet (bug 860493) because it's not quite finished yet (see dependent bugs). You can manually turn it on in about:config by setting browser.snapshots.limit to something greater than 0 (5 will most likely be the default).

Comment 83

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Hawken Rives from comment #74)
> I think this patch is (sometimes) breaking scrolling on Google Spreadsheets.
> I'll keep looking for a reproducible testcase, but it's really annoying.

I just noticed this too. Here is a Google spreadsheet that shows the behaviour. Instead of making the spreadsheet scroll, we get the bounce:

https://docs.google.com/spreadsheet/ccc?key=0AmwZL-gS9ZMmdDhNX3pGNDNHcmcxQ3BxOTFsQ3FfZlE#gid=0

Comment 84

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Why is this bug RESOLVED? The implementation is not complete yet right? Shouldn't this bug at least stay open until the feature works correctly?

Comment 85

[Stephen A Pohl [:spohl]]

(In reply to Stefan Arentz [:st3fan] from comment #84)
> Why is this bug RESOLVED? The implementation is not complete yet right?
> Shouldn't this bug at least stay open until the feature works correctly?

This bug laid the foundation for the bounce behavior. The remaining work is tracked as follow-up bugs (see dependent bugs).

Comment 86

[Stephen A Pohl [:spohl]]

(In reply to Stefan Arentz [:st3fan] from comment #83)
> (In reply to Hawken Rives from comment #74)
> > I think this patch is (sometimes) breaking scrolling on Google Spreadsheets.
> > I'll keep looking for a reproducible testcase, but it's really annoying.
> 
> I just noticed this too. Here is a Google spreadsheet that shows the
> behaviour. Instead of making the spreadsheet scroll, we get the bounce:
> 
> https://docs.google.com/spreadsheet/ccc?key=0AmwZL-
> gS9ZMmdDhNX3pGNDNHcmcxQ3BxOTFsQ3FfZlE#gid=0

See bug 861669.

Comment 87

[Albert Scheiner [:alberts]]

(In reply to Stephen Pohl [:spohl] from comment #82) 
> The bounce behavior isn't turned on by default yet (bug 860493) because it's
> not quite finished yet (see dependent bugs). You can manually turn it on in
> about:config by setting browser.snapshots.limit to something greater than 0
> (5 will most likely be the default).

Great! Thanks Stephen.

Comment 88

[Florian Bender]

Is this still coming in Firefox 23? Otherwise, I'd remove it from the relnotes.

Comment 89

[u279076]

(In reply to Florian Bender from comment #88)
> Is this still coming in Firefox 23? Otherwise, I'd remove it from the
> relnotes.

Please advise as I'll need to add this to Firefox 23.0b1 criteria in the coming days.

Comment 90

[Stephen A Pohl [:spohl]]

This is currently blocked by bug 860493, which in turn is blocked by (at least) bug 817700. Since bug 636564 is of higher priority it's doubtful that we'll be able to get to this and enable it for FF23.

Comment 91

[u279076]

Thanks Stephen, please add qawanted if/when this is ready to be tested. For now I'm taking it off my radar.

Lukas, can we get the tracking/status/relnote flags updated to reflect this state?

Comment 92

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been left in the relnotes DB in order to come back in a note for ff24 (hopefully)

Comment 93

[Stephen A Pohl [:spohl]]

(In reply to Phil Ringnalda (:philor) from comment #73)
> https://hg.mozilla.org/mozilla-central/rev/366e6a39d71a
> https://hg.mozilla.org/mozilla-central/rev/fd6c7792b048

These need to be reverted for causing bug 907275. Will be posting patches that work with current trunk shortly.

Comment 94

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Revert fd6c7792b048

Comment 95

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Revert 366e6a39d71a

Comment 96

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 793299 [details] [diff] [review]
Part 1: Revert fd6c7792b048

Sigh.  Too bad :-(

Comment 97

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce98deb07663
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c78f04b8640

Comment 98

[Florian Bender]

Did this (comment 97) propagate to m-c yet?

Comment 99

[Stephen A Pohl [:spohl]]

Yes, it did so via bug 907275.

Comment 100

[Florian Bender]

Ah, thanks!

Comment 101

[Stephen A Pohl [:spohl]]

I was going to do this in a follow-up bug, but since this has been backed out I'd like to revise the patches as follows: Instead of animating a screenshot for the vertical bounce like we do for the history swipe animations (bug 678392) I'd like to animate the actual 'page'. This means that animations etc. would keep playing when scrolling past the top or bottom of the page. This matches what Safari is doing.

I was thinking something like this could do the trick:
1. Get a hold of the root frame.
2. Change the position of the frame in the same way we used to animate the box element with the snapshot before.
3. Add a frame with a grey background.
4. Place the new frame where we moved the root frame away from and size it appropriately.

To move the root frame (or scroll frame obtained from the root frame) I've been trying the following, with no success so far:

  nsIFrame* frame = presShell->GetRootFrame();
  if (frame) {
    nsIFrame* scrollFrame = (nsIFrame*)frame->GetScrollTargetFrame();
    scrollFrame->MovePositionBy(nsPoint(0,50));
  }

dholbert suggested that this position change may get lost in subsequent reflows.

roc, do you think this approach sounds reasonable? Anything that you'd like to suggest, change or add?

Comment 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 775469 has existing work on adding overscrolling support. It relies on off-main-thread-compositing (which we have on Mac now) and AsyncPanZoomController scrolling (which we don't have on Mac, but we want!).

Comment 103

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #102)
> Bug 775469 has existing work on adding overscrolling support. It relies on
> off-main-thread-compositing (which we have on Mac now) and
> AsyncPanZoomController scrolling (which we don't have on Mac, but we want!).

If we go this route then we need to support OMTC basic on mac or not support this feature for black listed users (RadeonX1000 and people running 32-bit for plugin compatibility). Long term having overscroll be done in the compositor is much nicer.

Comment 104

[Stephen A Pohl [:spohl]]

Thanks roc and BenWa! I get the feeling that for now, we might be better off just fixing the issue that caused the patches here to be backed out (comment 93). By filing follow-up bugs, we can better track the work and priorities going forward.

Comment 105

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch combines the two earlier parts into one.

To avoid bug 907275 (which caused the earlier patches to be backed out, see comment 93), we had to correct the way we detect whether or not the WheelEvent targeted the 'page' (rather than a text field on the page for example). This change occurred in nsEventStateManager::ComputeScrollTarget. Steven, if you could review this piece, I'd appreciate it. This change basically fixes bug 861669 (I've verified that the issue there no longer occurs with this change). 

Another improvement included in this patch is that we no longer force a synchronous scroll to the top or bottom of the page when we start the bounce effect. Instead, we wait until the asynchronous scroll has reached the top or bottom of the page. This results in a smoother experience. This change occurred in GS_handleEvent and GS__setupSwipeGesture in browser-gestureSupport.js. Felipe, if you could review this part, I'd appreciate it.

Comment 106

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 811316 [details] [diff] [review]
Patch

The changes to nsChildView.mm and nsEventStateManager::ComputeScrollTarget() look fine to me.  But I'm not very familiar with the nsEventStateManager code, so you should probably also get someone else to review that part.  I'd suggest Masayuki Nakano, who seems to have made the most recent substantial changes to nsEventStateManager::ComputeScrollTarget().

But I do have a couple of style nits:

...
-  if (overflow != 0.0 && deltaX != 0.0 &&
+  if (overflowX != 0.0 && deltaX != 0.0 &&
       fabsf(deltaX) > fabsf(deltaY) * 8) {
...
+  } else if (overflowY != 0.0 && deltaY != 0.0 &&
+           fabsf(deltaY) > fabsf(deltaX) * 2) {
...

In both cases you put the comment that describes what happens in the conditional block below its first line.  I think the code is more readable if such comments are above the first line.

   NPCocoaEvent cocoaEvent;
-	
+
   nsMouseEvent geckoEvent(true, NS_MOUSE_BUTTON_UP, mGeckoChild, nsMouseEvent::eReal);

This is a whitespace change in completely unrelated code.  Yes, I know we hate tabs in our code, but please don't do this :-)

Comment 107

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Steven! Addressed your feedback.

Adding Masayuki for review of the nsEventStateManager::ComputeScrollTarget portion. FYI: The changes to nsEventStateManager::ComputeScrollTarget are loosely based on the top part of nsLayoutUtils::IsViewportScrollbarFrame.

Comment 108

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 811438 [details] [diff] [review]
Patch

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

r=felipe on the browser part.

For a small extra I'd suggest using a direction = "horizontal" or "vertical" property instead of the isVerticalSwipe boolean.

Comment 109

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

Comment on attachment 811438 [details] [diff] [review]
Patch

> nsIScrollableFrame*
> nsEventStateManager::ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                          WheelEvent* aEvent,
>                                          ComputeScrollTargetOptions aOptions)

I think it's wrong you to change this method.

I think that you need to check if the scroll target of *default action* is same as root or not.

However, this method is called for computing other targets too (see ComputeScrollTargetOptions).

So, you should set the value at after calling this method in nsEventStateManager::PostEventHandler() with the result.

Or, the scroll target must be stored in nsMouseWheelTransaction. So, at nsMouseWheelTransaction::BeginTransaction(), set the result to a static bool variable. Then, PostEventHandler() can just set the value to the event after calling this method.

Either way is okay.

>diff --git a/widget/MouseEvents.h b/widget/MouseEvents.h
>--- a/widget/MouseEvents.h
>+++ b/widget/MouseEvents.h
>@@ -313,17 +313,18 @@ private:
> 
> public:
>   WidgetWheelEvent(bool aIsTrusted, uint32_t aMessage, nsIWidget* aWidget) :
>     WidgetMouseEventBase(aIsTrusted, aMessage, aWidget, NS_WHEEL_EVENT),
>     deltaX(0.0), deltaY(0.0), deltaZ(0.0),
>     deltaMode(nsIDOMWheelEvent::DOM_DELTA_PIXEL),
>     customizedByUserPrefs(false), isMomentum(false), isPixelOnlyDevice(false),
>     lineOrPageDeltaX(0), lineOrPageDeltaY(0), scrollType(SCROLL_DEFAULT),
>-    overflowDeltaX(0.0), overflowDeltaY(0.0)
>+    overflowDeltaX(0.0), overflowDeltaY(0.0),
>+    viewPortIsScrollTargetParent(false)
>   {
>   }
> 
>   // NOTE: deltaX, deltaY and deltaZ may be customized by
>   //       mousewheel.*.delta_multiplier_* prefs which are applied by
>   //       nsEventStateManager.  So, after widget dispatches this event,
>   //       these delta values may have different values than before.
>   double deltaX;
>@@ -394,16 +395,21 @@ public:
>   // NOTE: deltaX, deltaY and deltaZ may be modified by nsEventStateManager.
>   //       However, overflowDeltaX and overflowDeltaY indicate unused original
>   //       delta values which are not applied the delta_multiplier prefs.
>   //       So, if widget wanted to know the actual direction to be scrolled,
>   //       it would need to check the deltaX and deltaY.
>   double overflowDeltaX;
>   double overflowDeltaY;
> 
>+  // Whether or not the parent of the currently scrolled frame is the ViewPort.
>+  // This is false in situations when an element on the page is being scrolled
>+  // (such as a text field), but true when the 'page' is being scrolled.
>+  bool viewPortIsScrollTargetParent;

Could you name this |mViewPortIsScrollTargetParent|? We discussed the naming rule of event class members. Then, we decided that we should use current our coding rule for new members. I.e., "m" prefix should be used.

Additionally, copy the value in AssignWheelEventData().

Comment 110

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Felipe and Masayuki!

This patch addresses Felipe's feedback. I've changed the name of the _isVerticalSwipe member to _direction as suggested.

Masayuki, I've

Comment 111

[Stephen A Pohl [:spohl]]

Comment on attachment 812174 [details] [diff] [review]
Patch

Hmm, not sure why the previous comment posted before I was done. Here it is again:

Thanks, Felipe and Masayuki!

This patch addresses Felipe's feedback. I've changed the name of the |_isVerticalSwipe| member to |_direction| as suggested.

Masayuki, I've changed the member name to |mViewPortIsScrollTargetParent| and also made sure that we copy the value in WidgetWheelEvent::AssignWheelEventData. However, I had to add the detection logic to nsEventStateManager::DispatchLegacyMouseScrollEvents rather than nsEventStateManager::PostHandleEvent like you suggested. I've confirmed that this works properly, but I may be missing something. I appreciate your feedback here.

While working on the feedback, I noticed that this patch breaks fast swiping, i.e. starting a new swipe before the previous animation completed. This is because after starting the animation, any subsequent scroll target was now the animation overlay, rather than the page. I've fixed this by calling maybeTrackScrollEventAsSwipe regardless of the value of |mViewPortIsScrollTargetParent|, but we pass the value as argument to maybeTrackScrollEventAsSwipe. There, we check that |aViewPortIsScrollTargetParent| is either true, or that an animation is currently active. Steven, if you could just check that this change is okay, I'd appreciate it. Thanks!

Comment 112

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 812174 [details] [diff] [review]
Patch

This looks fine to me.

+  // We should only track scroll events as swipe if the viewport is the parent
+  // of the scroll target, or we're already tracking scroll events and we're
+  // being interrupted by a new event.
+  if (!aViewportIsScrollTargetParent && !mCancelSwipeAnimation) {
+    return;
+  }

But I think the logic would be clearer if you added the following (in parentheses) to your comment:

(mCancelSwipeAnimation is NULL here if we haven't yet started tracking scroll events or if we've stopped tracking them.)

Comment 113

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Steven! Updated comment.

Comment 114

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

(In reply to Stephen Pohl [:spohl] from comment #111)
> However, I had to add the detection
> logic to nsEventStateManager::DispatchLegacyMouseScrollEvents rather than
> nsEventStateManager::PostHandleEvent like you suggested. I've confirmed that
> this works properly, but I may be missing something. I appreciate your
> feedback here.

Hmm, that's very strange. You refers the new WheelEvent member after the event is back to widget. That means the wheel event is passed after nsEventStateManager::PostHandleEvent(). Therefore, the value should be initialized if you don't press any modifiers and web page doesn't consume the wheel event.

> While working on the feedback, I noticed that this patch breaks fast
> swiping, i.e. starting a new swipe before the previous animation completed.
> This is because after starting the animation, any subsequent scroll target
> was now the animation overlay, rather than the page.

Sounds very odd. So, did you mean that even if swipe animation has already started, nsChildView.mm dispatches WheelEvent? And then, the WheelEvents which are dispatched after swipe animation has been started returns unexpected value?

If so, I think that you *must not* dispatch wheel event after starting the animation because the coming events for subsequent of swipe animation *should not* be handled by web pages. The native event should only cause dispatching gesture event rather than legacy mouse scroll event and wheel event. For example, don't you assume that any interception of the swipe animation never occurs? If web pages can handle wheel event (and legacy mouse scroll events), it means the web page can anything even during swipe animation from the event handler(s).

If you stop dispatching wheel event during swipe animation, then, as I suggested in the previous my comment, does checking the result of ComputeScrollTarget() for default action work perfectly?

Comment 115

[Stephen A Pohl [:spohl]]

Thanks for your feedback, Masayuki. I will respond to it in detail tomorrow. I did want to point out though that we do expect animations to be interruptible. This is the case during 'fast-swiping', i.e. when a user quickly goes back and forth in history before an animation has completed or when a user repeatedly causes the page to bounce before a bounce completes. This matches what Safari does.

Comment 116

[Stephen A Pohl [:spohl]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #114)
> (In reply to Stephen Pohl [:spohl] from comment #111)
> > However, I had to add the detection
> > logic to nsEventStateManager::DispatchLegacyMouseScrollEvents rather than
> > nsEventStateManager::PostHandleEvent like you suggested. I've confirmed that
> > this works properly, but I may be missing something. I appreciate your
> > feedback here.
> 
> Hmm, that's very strange. You refers the new WheelEvent member after the
> event is back to widget. That means the wheel event is passed after
> nsEventStateManager::PostHandleEvent(). Therefore, the value should be
> initialized if you don't press any modifiers and web page doesn't consume
> the wheel event.

This does not seem to be the case. We call nsEventStateManager::DispatchLegacyMouseScrollEvents indirectly via http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6903, and maybeTrackScrollEventAsSwipe in nsChildView.mm is called in response to the dispatched event(s). nsEventStateManager::PostHandleEvent is called later at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6915. In other words, initializing the WheelEvent's |mViewPortIsScrollTargetParent| member in nsEventStateManager::DispatchLegacyMouseScrollEvents seems necessary to make this information available to maybeTrackScrollEventAsSwipe in nsChildView.mm because initializing it in nsEventStateManager::PostHandleEvent would be too late.

Previously, initializing WheelEvent's |mViewPortIsScrollTargetParent| in ComputeScrollTarget worked because we also call ComputeScrollTarget in nsEventStateManager::DispatchLegacyMouseScrollEvents, but I do understand your concern that this may not be the right thing to do.

> > While working on the feedback, I noticed that this patch breaks fast
> > swiping, i.e. starting a new swipe before the previous animation completed.
> > This is because after starting the animation, any subsequent scroll target
> > was now the animation overlay, rather than the page.
> 
> Sounds very odd. So, did you mean that even if swipe animation has already
> started, nsChildView.mm dispatches WheelEvent?

As I mentioned in my previous comment 115, we expect these events to continue being dispatched via nsEventStateManager::DispatchLegacyMouseScrollEvents since this allows for fast-swiping.

Comment 117

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

(In reply to Stephen Pohl [:spohl] from comment #116)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #114)
> > (In reply to Stephen Pohl [:spohl] from comment #111)
> > > However, I had to add the detection
> > > logic to nsEventStateManager::DispatchLegacyMouseScrollEvents rather than
> > > nsEventStateManager::PostHandleEvent like you suggested. I've confirmed that
> > > this works properly, but I may be missing something. I appreciate your
> > > feedback here.
> > 
> > Hmm, that's very strange. You refers the new WheelEvent member after the
> > event is back to widget. That means the wheel event is passed after
> > nsEventStateManager::PostHandleEvent(). Therefore, the value should be
> > initialized if you don't press any modifiers and web page doesn't consume
> > the wheel event.
> 
> This does not seem to be the case. We call
> nsEventStateManager::DispatchLegacyMouseScrollEvents indirectly via
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#6903, and maybeTrackScrollEventAsSwipe in nsChildView.mm is called in
> response to the dispatched event(s). nsEventStateManager::PostHandleEvent is
> called later at
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#6915.

Exactly.

> In other words, initializing the WheelEvent's
> |mViewPortIsScrollTargetParent| member in
> nsEventStateManager::DispatchLegacyMouseScrollEvents seems necessary to make
> this information available to maybeTrackScrollEventAsSwipe in nsChildView.mm
> because initializing it in nsEventStateManager::PostHandleEvent would be too
> late.

No, after nsEventStateManager::PostHandleEvent(), i.e., after quitting PresShell::HandleEventInternal(), there it's NOT too late.

> Previously, initializing WheelEvent's |mViewPortIsScrollTargetParent| in
> ComputeScrollTarget worked because we also call ComputeScrollTarget in
> nsEventStateManager::DispatchLegacyMouseScrollEvents, but I do understand
> your concern that this may not be the right thing to do.

Yes. The result of ComputeScrollTarget() in DispatchLegacyMouseScrollEvents() is a scrollable element which is the nearest ancestor element under mouse cursor. However, actual scroll target of the default action which handles PostHandleEvent() may be different from it. For example, when you scroll <body> of the document consecutively in short term, even if other scrollable element such as <iframe> or <textarea> is scrolled into under the mouse cursor, following wheel event keep scrolling the <body> element. I think that the latter is what you want to know.

> > > While working on the feedback, I noticed that this patch breaks fast
> > > swiping, i.e. starting a new swipe before the previous animation completed.
> > > This is because after starting the animation, any subsequent scroll target
> > > was now the animation overlay, rather than the page.
> > 
> > Sounds very odd. So, did you mean that even if swipe animation has already
> > started, nsChildView.mm dispatches WheelEvent?
> 
> As I mentioned in my previous comment 115, we expect these events to
> continue being dispatched via
> nsEventStateManager::DispatchLegacyMouseScrollEvents since this allows for
> fast-swiping.

> I did want to point out though that we do expect animations to be interruptible. This is the case
> during 'fast-swiping', i.e. when a user quickly goes back and forth in history before an animation
> has completed or when a user repeatedly causes the page to bounce before a bounce completes. This
> matches what Safari does.

Hmm, I don't understand. Looks like your patch handles gesture events, neither legacy scroll events nor wheel events. So, dispatching gesture event during animation, isn't it?

If you dispatch legacy mouse scroll events and wheel event during animation, web page can handle it. For example, the webpage may consume the event. Then, any default action shouldn't be performed. Additionally, another example, the webpage may destroy the page or try to load another page (it's not realistic scenario, but it's possible).

I guess that you do not want such webpage's (webapp's) interruption. If so, you must not dispatch wheel event during animation.

Comment 118

[Stephen A Pohl [:spohl]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #117)
> (In reply to Stephen Pohl [:spohl] from comment #116)
> > Previously, initializing WheelEvent's |mViewPortIsScrollTargetParent| in
> > ComputeScrollTarget worked because we also call ComputeScrollTarget in
> > nsEventStateManager::DispatchLegacyMouseScrollEvents, but I do understand
> > your concern that this may not be the right thing to do.
> 
> Yes. The result of ComputeScrollTarget() in
> DispatchLegacyMouseScrollEvents() is a scrollable element which is the
> nearest ancestor element under mouse cursor. However, actual scroll target
> of the default action which handles PostHandleEvent() may be different from
> it. For example, when you scroll <body> of the document consecutively in
> short term, even if other scrollable element such as <iframe> or <textarea>
> is scrolled into under the mouse cursor, following wheel event keep
> scrolling the <body> element. I think that the latter is what you want to
> know.

Right. The problem with COMPUTE_DEFAULT_ACTION_TARGET is that whenever we hit the edge of the page and try to overscroll (i.e. when we want to start the bounce effect), ComputeScrollTarget returns NULL. This makes sense, since the frame is no longer scrollable in that direction. Looking at our ComputeScrollTargetOptions enum, it seems like COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET is my only viable choice. Is there a better alternative?

> > > > While working on the feedback, I noticed that this patch breaks fast
> > > > swiping, i.e. starting a new swipe before the previous animation completed.
> > > > This is because after starting the animation, any subsequent scroll target
> > > > was now the animation overlay, rather than the page.
> > > 
> > > Sounds very odd. So, did you mean that even if swipe animation has already
> > > started, nsChildView.mm dispatches WheelEvent?
> > 
> > As I mentioned in my previous comment 115, we expect these events to
> > continue being dispatched via
> > nsEventStateManager::DispatchLegacyMouseScrollEvents since this allows for
> > fast-swiping.
> 
> > I did want to point out though that we do expect animations to be interruptible. This is the case
> > during 'fast-swiping', i.e. when a user quickly goes back and forth in history before an animation
> > has completed or when a user repeatedly causes the page to bounce before a bounce completes. This
> > matches what Safari does.
> 
> Hmm, I don't understand. Looks like your patch handles gesture events,
> neither legacy scroll events nor wheel events. So, dispatching gesture event
> during animation, isn't it?
> 
> If you dispatch legacy mouse scroll events and wheel event during animation,
> web page can handle it. For example, the webpage may consume the event.
> Then, any default action shouldn't be performed. Additionally, another
> example, the webpage may destroy the page or try to load another page (it's
> not realistic scenario, but it's possible).
> 
> I guess that you do not want such webpage's (webapp's) interruption. If so,
> you must not dispatch wheel event during animation.

I could skip the dispatching of the wheelEvent right before our call to maybeTrackScrollEventAsSwipe when an animation is in progress, but if I do that, any calls to maybeTrackScrollEventAsSwipe will return early because both overflowX and overflowY will be 0 (since the event didn't get dispatched and neither wheelEvent.overflowDeltaX nor wheelEvent.overflowDeltaY got populated). If this is what we want to do, I would probably have to add code to maybeTrackScrollEventAsSwipe to detect the direction of the event when an animation is already in progress and skip the early return. Should I do this?

Comment 119

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

(In reply to Stephen Pohl [:spohl] from comment #118)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #117)
> > (In reply to Stephen Pohl [:spohl] from comment #116)
> > > Previously, initializing WheelEvent's |mViewPortIsScrollTargetParent| in
> > > ComputeScrollTarget worked because we also call ComputeScrollTarget in
> > > nsEventStateManager::DispatchLegacyMouseScrollEvents, but I do understand
> > > your concern that this may not be the right thing to do.
> > 
> > Yes. The result of ComputeScrollTarget() in
> > DispatchLegacyMouseScrollEvents() is a scrollable element which is the
> > nearest ancestor element under mouse cursor. However, actual scroll target
> > of the default action which handles PostHandleEvent() may be different from
> > it. For example, when you scroll <body> of the document consecutively in
> > short term, even if other scrollable element such as <iframe> or <textarea>
> > is scrolled into under the mouse cursor, following wheel event keep
> > scrolling the <body> element. I think that the latter is what you want to
> > know.
> 
> Right. The problem with COMPUTE_DEFAULT_ACTION_TARGET is that whenever we
> hit the edge of the page and try to overscroll (i.e. when we want to start
> the bounce effect), ComputeScrollTarget returns NULL. This makes sense,
> since the frame is no longer scrollable in that direction. Looking at our
> ComputeScrollTargetOptions enum, it seems like
> COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET is my only viable choice. Is there
> a better alternative?

If ComputeScrollTarget() returns null at using COMPUTE_DEFAULT_ACTION_TARGET, then, it means that there is no scrollable frame even if it climes to the root document's root element. I.e., you can/should assume that the scroll target is as root element.

If you use COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET, it never becomes root element if the nearest ancestor scrollable element under mouse cursor is not root element. E.g., when mouse cursor is over <textarea>.

> > > > > While working on the feedback, I noticed that this patch breaks fast
> > > > > swiping, i.e. starting a new swipe before the previous animation completed.
> > > > > This is because after starting the animation, any subsequent scroll target
> > > > > was now the animation overlay, rather than the page.
> > > > 
> > > > Sounds very odd. So, did you mean that even if swipe animation has already
> > > > started, nsChildView.mm dispatches WheelEvent?
> > > 
> > > As I mentioned in my previous comment 115, we expect these events to
> > > continue being dispatched via
> > > nsEventStateManager::DispatchLegacyMouseScrollEvents since this allows for
> > > fast-swiping.
> > 
> > > I did want to point out though that we do expect animations to be interruptible. This is the case
> > > during 'fast-swiping', i.e. when a user quickly goes back and forth in history before an animation
> > > has completed or when a user repeatedly causes the page to bounce before a bounce completes. This
> > > matches what Safari does.
> > 
> > Hmm, I don't understand. Looks like your patch handles gesture events,
> > neither legacy scroll events nor wheel events. So, dispatching gesture event
> > during animation, isn't it?
> > 
> > If you dispatch legacy mouse scroll events and wheel event during animation,
> > web page can handle it. For example, the webpage may consume the event.
> > Then, any default action shouldn't be performed. Additionally, another
> > example, the webpage may destroy the page or try to load another page (it's
> > not realistic scenario, but it's possible).
> > 
> > I guess that you do not want such webpage's (webapp's) interruption. If so,
> > you must not dispatch wheel event during animation.
> 
> I could skip the dispatching of the wheelEvent right before our call to
> maybeTrackScrollEventAsSwipe when an animation is in progress, but if I do
> that, any calls to maybeTrackScrollEventAsSwipe will return early because
> both overflowX and overflowY will be 0 (since the event didn't get
> dispatched and neither wheelEvent.overflowDeltaX nor
> wheelEvent.overflowDeltaY got populated). If this is what we want to do, I
> would probably have to add code to maybeTrackScrollEventAsSwipe to detect
> the direction of the event when an animation is already in progress and skip
> the early return. Should I do this?

I think that you should set deltaX value to overflowX and deltaY value to overflowY before calling maybeTrackScrollEventAsSwipe. That is same behavior if a dispatched wheel event doesn't cause scroll.

FYI: By the fix of bug 868648, touching two fingers causes NS_WHEEL_START event and leaving two fingers causes NS_WHEEL_END event. It might cause you needing to change something in nsChildView.mm.

Comment 120

[Stephen A Pohl [:spohl]]

Ah, nice! This makes a lot of sense. I should have the patch ready tomorrow. Thanks Masayuki!

Comment 121

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback and changed WidgetWheelEvent's new member name to mViewPortIsOverscrolled to reflect its meaning better.

Comment 122

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/751bcb37cdb6

Comment 123

[Stephen A Pohl [:spohl]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/1e935a380fb2

Comment 124

[Stephen A Pohl [:spohl]]

Attachment: Patch

Added necessary ifdef's. Carrying over r+.

Comment 125

[Stephen A Pohl [:spohl]]

Waiting for try run to be green before attempting to push again:
https://tbpl.mozilla.org/?tree=Try&rev=5ab1eaf885cc

Comment 126

[Stephen A Pohl [:spohl]]

Attachment: Patch

Removed one unnecessary #endif and #ifdef. No functional changes. Carrying over r+.

Comment 127

[Stephen A Pohl [:spohl]]

Try is green. Pushed to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b784c6dafa7d

Comment 128

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

https://hg.mozilla.org/mozilla-central/rev/b784c6dafa7d

Comment 129

[Cheba]

I'm not sure if it's how it's meant to be…

Horizontal swipe is not registered if scrollbar is visible.

Steps to reproduce:

1. Scroll the page so that scrollbar got visible.
2. Swipe horizontally.

Expected result:

Navigation back/forth or swipe indication if history is empty.

Actual result:

Nothing happens.


3. Wait for scrollbar to hide.

Now both animations and actions are preformed as expected.


Build: Firefox Nightly 27.0a1 (2013-10-12)

Comment 130

[Stephen A Pohl [:spohl]]

(In reply to Cheba from comment #129)
> I'm not sure if it's how it's meant to be…
> 
> Horizontal swipe is not registered if scrollbar is visible.

This is intentional. This is similar to Safari. When you have browsing history available in Safari, start scrolling vertically and then swipe back, you will not see an animation of the previous page but the 'swipe indication' instead.

Comment 131

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #130)
> (In reply to Cheba from comment #129)
> > I'm not sure if it's how it's meant to be…
> > 
> > Horizontal swipe is not registered if scrollbar is visible.
> 
> This is intentional. This is similar to Safari. When you have browsing
> history available in Safari, start scrolling vertically and then swipe back,
> you will not see an animation of the previous page but the 'swipe
> indication' instead.

Hmm, this is only true if the swipe back is initiated without leaving the trackpad. If the swipe back is triggered separately (but while the scrollbars are still showing) we should be able to navigate back. I'll look into it and file a new bug if necessary.

Comment 132

[Cheba]

You've described Safari behaviour perfectly. Though, even when you swipe horizontally after scrolling without leaving the trackpad Safari still moves the page bit to the side. It doesn't navigate back and doesn't show previous page but reveals linen background. Firefox doesn't react at all to that it continues tracking only vertical gesture (i.e. does scrolling without moving the page to horizontally). So in Safari is feels a bit more natural while in Firefox it looks like FF doesn't register the gesture at all.

Comment 134

[José Jeria]

Why is this bug marked as fixed? I don't see the bouncing effect.

Comment 135

[Stephen A Pohl [:spohl]]

This bug is marked as fixed because the patches have landed. However, the bounce behavior as well as history swipe animations (bug 678392) are currently turned off by default (bug 860493). At the very least, we have to implement bug 939480 before we can think about turning the bounce behavior on by default.