Closed Bug 926274 Opened 6 years ago Closed 6 years ago

Swipe back gesture very hard to trigger on recent nightlies

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 --- unaffected
firefox27 + verified

People

(Reporter: reuben, Assigned: spohl)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
(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.
b784c6dafa7d it is. Stephen, any ideas?
Flags: needinfo?(spohl.mozilla.bugs)
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()->
(In reply to Reuben Morais [:reuben] from comment #2)
> Stephen, any ideas?

Hmm, strange. Looking into it.
Flags: needinfo?(spohl.mozilla.bugs)
(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!
Flags: needinfo?(reuben.bmo)
(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.
Flags: needinfo?(reuben.bmo)
Attached patch Patch (obsolete) — Splinter Review
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/
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #817299 - Flags: review?(masayuki)
(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 on attachment 817299 [details] [diff] [review]
Patch

Anyway, this patch looks good to me.
Attachment #817299 - Flags: review?(masayuki) → review+
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();
Attached patch PatchSplinter Review
Thanks, Masayuki! Addressed feedback. Carrying over r+.

I will reopen bug 907275 for current trunk and take the remaining discussion over there.
Attachment #817299 - Attachment is obsolete: true
Attachment #817796 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/77a75238baa7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
verified with Nightly build 20131024030204
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.