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

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: rik, Assigned: spohl)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
mozilla23
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox +)

Details

(Whiteboard: [parity-chrome] [parity-safari], )

Attachments

(1 attachment, 28 obsolete attachments)

27.02 KB, patch
spohl
: review+
Details | Diff | Splinter Review
Lion has this new bouncing behaviour when reaching the end of a document.
+1. This feels super nice in Chrome and Safari. Would love to see it in FF aswell.
Whiteboard: [parity-chrome] [parity-safari]
FWIW, it's worth considering a non-native implementation (maybe even in scrollbox.xml).
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.
> 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)?
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.
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
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.
> 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.
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...
Bug 678392 has (horizontally) implemented this effect nicely when swiping to a page that is the last one to go back to.
Assignee: nobody → spohl.mozilla.bugs
Posted patch Wip (obsolete) — Splinter Review
This patch requires the latest patch for bug 678392 and 817700. Try build to follow.
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/spohl@mozilla.com-6684b1a43038
Posted video Movie showing the bounce effect (obsolete) —
(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
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.
(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?
Posted patch Patch (obsolete) — Splinter Review
Addressed feedback from comment 14 and reduced flickering.
Attachment #698165 - Attachment is obsolete: true
Attachment #698273 - Attachment is obsolete: true
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/spohl@mozilla.com-b6788179339b
(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/spohl@mozilla.com-
> 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.
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.
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).
Vertical is the issue.
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.
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.
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.
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.
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.
Sounds good to me.
Posted patch Patch (obsolete) — Splinter Review
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).
Attachment #698883 - Attachment is obsolete: true
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/spohl@mozilla.com-8cea2f530ca5
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #700002 - Attachment is obsolete: true
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/spohl@mozilla.com-b23060443dab
(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/spohl@mozilla.com-
> b23060443dab

Verified that the bounce effect no longer gets stuck with this release. Looking good.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #700773 - Attachment is obsolete: true
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/spohl@mozilla.com-d765f270f28d
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #702503 - Attachment is obsolete: true
Depends on: 678392
Depends on: 817700
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/spohl@mozilla.com-47a4b87e3a43
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!
> 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.
(In reply to Steven Michaud from comment #39)

Thanks. That is what I figured.
Posted patch Patch (obsolete) — Splinter Review
Updated patch to work with the latest patch in bug 678392. No functional changes.
Attachment #703115 - Attachment is obsolete: true
Posted patch Patch (obsolete) — Splinter Review
Cleanup. No functional changes.
Attachment #705152 - Attachment is obsolete: true
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #705170 - Attachment is obsolete: true
Attachment #705458 - Flags: review?(smichaud)
Attachment #705458 - Flags: review?(smichaud)
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #705458 - Attachment is obsolete: true
Blocks: 836456
Attachment #707893 - Flags: review?(smichaud)
Posted patch Patch (obsolete) — Splinter Review
Updated for current trunk and new patch in bug 678392.
Attachment #707893 - Attachment is obsolete: true
Attachment #707893 - Flags: review?(smichaud)
Attachment #709298 - Flags: review?(smichaud)
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 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?
(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.
(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.
(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.
> 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.
(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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #709298 - Attachment is obsolete: true
Attachment #709298 - Flags: review?(smichaud)
Attachment #711914 - Flags: review?(smichaud)
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.
(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.
Please re-CC me if you need me again :-)

Gerv
Posted patch Patch (obsolete) — Splinter Review
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".
Attachment #711914 - Attachment is obsolete: true
Attachment #711914 - Flags: review?(smichaud)
Attachment #712088 - Flags: review?(smichaud)
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.
Attachment #712088 - Flags: review?(smichaud) → review+
Posted patch Patch (obsolete) — Splinter Review
Addressed feedback. Sending to Felipe for a review of the JS pieces.
Attachment #712088 - Attachment is obsolete: true
Attachment #713171 - Flags: review?(felipc)
Posted patch Patch (obsolete) — Splinter Review
Removed trailing white space.
Attachment #713171 - Attachment is obsolete: true
Attachment #713171 - Flags: review?(felipc)
Attachment #713177 - Flags: review?(felipc)
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?
Attachment #713177 - Flags: review?(felipc) → feedback+
Posted patch Patch (obsolete) — Splinter Review
Addressed review feedback
Attachment #713177 - Attachment is obsolete: true
Attachment #732399 - Flags: review?(felipc)
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).
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).
Depends on: history-swipe
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
Attachment #732399 - Flags: review?(felipc) → review+
Posted patch Part 1: Patch (obsolete) — Splinter Review
Removed isVerticalSwipe from function call. Carrying over r+.
Attachment #732399 - Attachment is obsolete: true
Attachment #737008 - Flags: review+
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).
Attachment #737010 - Flags: review?(smichaud)
Forgot hg qrefresh.
Attachment #737010 - Attachment is obsolete: true
Attachment #737010 - Flags: review?(smichaud)
Attachment #737011 - Flags: review?(smichaud)
Comment on attachment 737011 [details] [diff] [review]
Part 2: Reenable guard for bug 770626 when swipe animations are turned off

Looks fine to me.
Attachment #737011 - Flags: review?(smichaud) → review+
Please note that the bounce behavior will be turned off as long as history swipe animations are turned off (bug 678392).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/366e6a39d71a
https://hg.mozilla.org/mozilla-central/rev/fd6c7792b048
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
I think this patch is (sometimes) breaking scrolling on Google Spreadsheets. I'll keep looking for a reproducible testcase, but it's really annoying.
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)
Depends on: 861669
(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.
Depends on: 836430
Target Milestone: mozilla23 → ---
Target Milestone: --- → mozilla23
Depends on: 861748
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.
(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.
(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.
Blocks: 865563
Blocks: 868166
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.
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?
(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).
(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
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?
(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).
(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.
(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.
Is this still coming in Firefox 23? Otherwise, I'd remove it from the relnotes.
(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.
Flags: needinfo?(lsblakk)
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.
Flags: needinfo?(lsblakk)
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?
This has been left in the relnotes DB in order to come back in a note for ff24 (hopefully)
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [parity-chrome] [parity-safari] → [parity-chrome] [parity-safari][leave open]
Posted patch Part 1: Revert fd6c7792b048 (obsolete) — Splinter Review
Attachment #793299 - Flags: review?(smichaud)
Attachment #793299 - Attachment description: Revert fd6c7792b048 → Part 1: Revert fd6c7792b048
Posted patch Part 2: Revert 366e6a39d71a (obsolete) — Splinter Review
Attachment #737008 - Attachment is obsolete: true
Attachment #737011 - Attachment is obsolete: true
Attachment #793300 - Flags: review?(smichaud)
Depends on: 907275
Comment on attachment 793299 [details] [diff] [review]
Part 1: Revert fd6c7792b048

Sigh.  Too bad :-(
Attachment #793299 - Flags: review?(smichaud) → review+
Attachment #793300 - Flags: review?(smichaud) → review+
Depends on: 874608
Did this (comment 97) propagate to m-c yet?
Yes, it did so via bug 907275.
Ah, thanks!
Depends on: 917761
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?
Flags: needinfo?(roc)
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!).
Flags: needinfo?(roc)
(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.
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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #793299 - Attachment is obsolete: true
Attachment #793300 - Attachment is obsolete: true
Attachment #811316 - Flags: review?(smichaud)
Attachment #811316 - Flags: review?(felipc)
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 :-)
Attachment #811316 - Flags: review?(smichaud) → review+
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #811316 - Attachment is obsolete: true
Attachment #811316 - Flags: review?(felipc)
Attachment #811438 - Flags: review?(masayuki)
Attachment #811438 - Flags: review?(felipc)
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.
Attachment #811438 - Flags: review?(felipc) → review+
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().
Posted patch Patch (obsolete) — Splinter Review
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
Attachment #811438 - Attachment is obsolete: true
Attachment #811438 - Flags: review?(masayuki)
Attachment #812174 - Flags: review?(masayuki)
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!
Attachment #812174 - Flags: review?(smichaud)
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.)
Attachment #812174 - Flags: review?(smichaud) → review+
Posted patch Patch (obsolete) — Splinter Review
Thanks, Steven! Updated comment.
Attachment #812174 - Attachment is obsolete: true
Attachment #812174 - Flags: review?(masayuki)
Attachment #812236 - Flags: review?(masayuki)
(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?
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.
(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.
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
(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?
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
Ah, nice! This makes a lot of sense. I should have the patch ready tomorrow. Thanks Masayuki!
Posted patch Patch (obsolete) — Splinter Review
Addressed feedback and changed WidgetWheelEvent's new member name to mViewPortIsOverscrolled to reflect its meaning better.
Attachment #812236 - Attachment is obsolete: true
Attachment #812236 - Flags: review?(masayuki)
Attachment #815722 - Flags: review?(masayuki)
Posted patch Patch (obsolete) — Splinter Review
Added necessary ifdef's. Carrying over r+.
Attachment #815722 - Attachment is obsolete: true
Attachment #815871 - Flags: review+
Waiting for try run to be green before attempting to push again:
https://tbpl.mozilla.org/?tree=Try&rev=5ab1eaf885cc
Posted patch PatchSplinter Review
Removed one unnecessary #endif and #ifdef. No functional changes. Carrying over r+.
Attachment #815871 - Attachment is obsolete: true
Attachment #815876 - Flags: review+
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)
(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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [parity-chrome] [parity-safari][leave open] → [parity-chrome] [parity-safari]
(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.
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.
Depends on: 930922
No longer blocks: 836456
Depends on: 836456
Duplicate of this bug: 930922
Blocks: 939480
No longer depends on: history-swipe
Why is this bug marked as fixed? I don't see the bouncing effect.
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.
Blocks: 1382560
No longer blocks: 1382560
You need to log in before you can comment on or make changes to this bug.