926274 - Swipe back gesture very hard to trigger on recent nightlies

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Reuben Morais [:reuben]]

The two or three (depending on OS setting) swipe gesture to Go Back has regressed over the past couple of days.

GOOD - 6b101d4c6d24
BAD - 73f37c7a3860

pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6b101d4c6d24&tochange=73f37c7a3860

Suspicious csets:

f7def963cdcc	André Reinald — Bug 868648: Make window overlay scrollbars appear/disappear when 2 fingers down/up trackpad. Credits to Markus for his help getting this right. r=mstange,masayuki,smichaud
b784c6dafa7d	Stephen Pohl — Bug 673875: Reproduce the bounce behavior when reaching the top/bottom of the page on OSX. r=smichaud,felipe,masayuki 

and maybe 

5dc3c476f55e	Josh Aas — Bug 925016: Always use method_exchangeImplementations when swizzling methods on OS X. r=smichaud

Comment 1

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #0)
> The two or three (depending on OS setting) swipe gesture to Go Back has
> regressed over the past couple of days.

Sorry, regressed here means it's super hard to activate. Should be easy to notice the difference, go to a long page (http://reddit.com/), follow a link, scroll down, then try to quickly use the swipe gesture to go back.

Comment 2

[Reuben Morais [:reuben]]

b784c6dafa7d it is. Stephen, any ideas?

Comment 3

[Reuben Morais [:reuben]]

Specifically, undoing just this chunk makes the problem go away:

diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
--- a/content/events/src/nsEventStateManager.cpp
+++ b/content/events/src/nsEventStateManager.cpp
@@ -3426,9 +3426,6 @@ nsEventStateManager::PostHandleEvent(nsP

           nsScrollbarsForWheel::SetActiveScrollTarget(scrollTarget);

+          if (!scrollTarget) {
+            wheelEvent->mViewPortIsOverscrolled = true;
+          }
           wheelEvent->overflowDeltaX = wheelEvent->deltaX;
           wheelEvent->overflowDeltaY = wheelEvent->deltaY;
           WheelPrefs::GetInstance()->

Comment 4

[Stephen A Pohl [:spohl]]

(In reply to Reuben Morais [:reuben] from comment #2)
> Stephen, any ideas?

Hmm, strange. Looking into it.

Comment 5

[Stephen A Pohl [:spohl]]

(In reply to Reuben Morais [:reuben] from comment #3)
> Specifically, undoing just this chunk makes the problem go away:
> 
> diff --git a/content/events/src/nsEventStateManager.cpp
> b/content/events/src/nsEventStateManager.cpp
> --- a/content/events/src/nsEventStateManager.cpp
> +++ b/content/events/src/nsEventStateManager.cpp
> @@ -3426,9 +3426,6 @@ nsEventStateManager::PostHandleEvent(nsP
> 
>            nsScrollbarsForWheel::SetActiveScrollTarget(scrollTarget);
> 
> +          if (!scrollTarget) {
> +            wheelEvent->mViewPortIsOverscrolled = true;
> +          }
>            wheelEvent->overflowDeltaX = wheelEvent->deltaX;
>            wheelEvent->overflowDeltaY = wheelEvent->deltaY;
>            WheelPrefs::GetInstance()->

Is this really true? I would expect this to break history navigation completely and I've confirmed that I can't navigate back with a swipe when I undo this change locally. Is there maybe another chunk that you undid?

At this point, I believe the problem is that we don't allow for an immediate swipe back after we've scrolled vertically. You should, however, be able to swipe back after the scrollbars have disappeared. Is this what you're seeing too? Thanks!

Comment 6

[Reuben Morais [:reuben]]

(In reply to Stephen Pohl [:spohl] from comment #5)
> (In reply to Reuben Morais [:reuben] from comment #3)
> > Specifically, undoing just this chunk makes the problem go away:
> > 
> > diff --git a/content/events/src/nsEventStateManager.cpp
> > b/content/events/src/nsEventStateManager.cpp
> > --- a/content/events/src/nsEventStateManager.cpp
> > +++ b/content/events/src/nsEventStateManager.cpp
> > @@ -3426,9 +3426,6 @@ nsEventStateManager::PostHandleEvent(nsP
> > 
> >            nsScrollbarsForWheel::SetActiveScrollTarget(scrollTarget);
> > 
> > +          if (!scrollTarget) {
> > +            wheelEvent->mViewPortIsOverscrolled = true;
> > +          }
> >            wheelEvent->overflowDeltaX = wheelEvent->deltaX;
> >            wheelEvent->overflowDeltaY = wheelEvent->deltaY;
> >            WheelPrefs::GetInstance()->
> 
> Is this really true? I would expect this to break history navigation
> completely and I've confirmed that I can't navigate back with a swipe when I
> undo this change locally. Is there maybe another chunk that you undid?

Nope, it probably wasn't picked up by my partial build or something silly like that. I just confirmed and undoing that chunk alone breaks navigation, yes.

> At this point, I believe the problem is that we don't allow for an immediate
> swipe back after we've scrolled vertically. You should, however, be able to
> swipe back after the scrollbars have disappeared. Is this what you're seeing
> too? Thanks!

Yes, that's exactly what's going on.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

This fixes the problem reported here. The problem was that when the page was being scrolled vertically, ComputeScrollTarget would return the root scroll frame rather than NULL when swiping back in history. We now check whether the scroll target is either NULL or equal to the root scroll frame.

I have one concern however: We will inevitably break what's been reported in bug 907275 again. On the test page [1], when scrolling the list, ComputeScrollTarget returns NULL, which we take to mean that the ViewPort has been overscrolled. This means that we stop the vertical scroll and cause the page to bounce instead. I've confirmed that Safari does the same, with the difference that while the page is bouncing, the item list is scrolling too. We can't do both (bounce and scroll) at the same time, since we're currently using snapshots for the bounce effect until we've implemented the AsyncPanZoomController for OSX (which will take some time). Masayuki, can you tell me if this is an acceptable breakage?

I've also fixed an oversight in the patches of bug 673875: We should only cancel a current swipe animation when we've performed all checks, including the check that |shouldStartSwipe| is true. Otherwise, a swipe animation can get stuck when it's being interrupted by a vertical scroll.

[1] http://lab.cubiq.org/iscroll/examples/simple/

Comment 8

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

(In reply to Stephen Pohl [:spohl] from comment #7)
> I have one concern however: We will inevitably break what's been reported in
> bug 907275 again. On the test page [1], when scrolling the list,
> ComputeScrollTarget returns NULL, which we take to mean that the ViewPort
> has been overscrolled. This means that we stop the vertical scroll and cause
> the page to bounce instead. I've confirmed that Safari does the same, with
> the difference that while the page is bouncing, the item list is scrolling
> too. We can't do both (bounce and scroll) at the same time, since we're
> currently using snapshots for the bounce effect until we've implemented the
> AsyncPanZoomController for OSX (which will take some time). Masayuki, can
> you tell me if this is an acceptable breakage?

I don't understand well your question. The testcase looks like not working fine with current Nightly build. Additionally, on my environment, swipe animation isn't performed. Do I need to change something in about:config?
# The iscroll.js should handle wheel event rather than DOMMouseScroll event, though.

Comment 9

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

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

Anyway, this patch looks good to me.

Comment 10

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

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

Oopa, aTargetFrame could be nullptr. Please change it as:

+          nsIFrame* rootScrollFrame = !aTargetFrame ? nullptr :
+            aTargetFrame->PresContext()->PresShell()->GetRootScrollFrame();

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Masayuki! Addressed feedback. Carrying over r+.

I will reopen bug 907275 for current trunk and take the remaining discussion over there.

Comment 12

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77a75238baa7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77a75238baa7

Comment 14

[Tracy Walker [:tracy]]

verified with Nightly build 20131024030204