Closed
Bug 926274
Opened 8 years ago
Closed 8 years ago
Swipe back gesture very hard to trigger on recent nightlies
Categories
(Core :: Widget: Cocoa, defect)
Core
Widget: Cocoa
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)
3.12 KB,
patch
|
spohl
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
(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.
Reporter | ||
Comment 2•8 years ago
|
||
b784c6dafa7d it is. Stephen, any ideas?
Flags: needinfo?(spohl.mozilla.bugs)
Reporter | ||
Comment 3•8 years ago
|
||
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()->
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Reuben Morais [:reuben] from comment #2) > Stephen, any ideas? Hmm, strange. Looking into it.
Flags: needinfo?(spohl.mozilla.bugs)
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Reporter | ||
Comment 6•8 years ago
|
||
(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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
(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•8 years ago
|
||
Comment on attachment 817299 [details] [diff] [review] Patch Anyway, this patch looks good to me.
Attachment #817299 -
Flags: review?(masayuki) → review+
Comment 10•8 years ago
|
||
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();
Assignee | ||
Comment 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a75238baa7
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77a75238baa7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•8 years ago
|
Comment 14•8 years ago
|
||
verified with Nightly build 20131024030204
You need to log in
before you can comment on or make changes to this bug.
Description
•