Last Comment Bug 678834 - [10.7] Tab swipe scrolling behavior changed for the worse
: [10.7] Tab swipe scrolling behavior changed for the worse
Status: RESOLVED FIXED
[qa+]
: relnote
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on: 703795
Blocks: lion-compatibility 668953
  Show dependency treegraph
 
Reported: 2011-08-14 09:20 PDT by Cheba
Modified: 2013-12-27 14:34 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
part 1, v1: call preventDefault on scroll events processed by scrollbox (1014 bytes, patch)
2011-08-26 15:28 PDT, Markus Stange [:mstange]
enndeakin: review+
Details | Diff | Review
part 2, v1: don't start a swipe if the scroll event has been preventDefaulted (1.47 KB, patch)
2011-08-26 15:29 PDT, Markus Stange [:mstange]
smichaud: review+
Details | Diff | Review

Description Cheba 2011-08-14 09:20:57 PDT
Scrolling tabs (in tabbar) with two fingers on touchpad works differently now. Earlier tabs was scrolling fast all the time I move fingers. Now tabs scroll just a little (about one tab width) in a single scroll animation and stop responding to scroll motion until I remove fingers and start scrolling again.

Vertical scroll works the same as before: tabs scroll all the time I do scrolling on touchpad.
Comment 1 Steven Michaud [:smichaud] (Retired) 2011-08-14 10:29:56 PDT
Forgive my ignorance, but how is scrolling tabs *supposed to* work?

Please describe that in some detail.
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-08-14 10:31:24 PDT
Or to be more precise, please describe in some detail how to scroll tabs (in FF distros where it works properly).
Comment 3 Cheba 2011-08-14 13:11:29 PDT
Open some tabs so that some of them go out of view and scroll arrows on both sides appear. Place cursor over any of visible tabs and scroll.

Vertical scroll works as it's supposed to work. Tabs scroll fast and do so all the time you perform scroll gesture on trackpad (or magic mouse, or I guess any mouse with two scroll wheels). Notice the difference when you do horizontal scroll. That's what reported in this bug.
Comment 4 Johan Larsson 2011-08-21 09:02:24 PDT
With the latest fix from bug 668953, firefox now also moves back & forth through the history stack for the active tab when scrolling horizontally in the tab-bar.

Steps to reproduce:

Scroll horizontally in the tab bar.

Actual results:

1. The tab bar scrolls one tab to the left or right
2. The active tab is going back/forward through the history stack.


Expected results:

1. The tab bar should scroll as it does when scrolling vertically.
2. The history-stack on the active tab should not be altered when doing horizontal swipe gestures in the tab bar.
Comment 5 Markus Stange [:mstange] 2011-08-26 15:28:53 PDT
Created attachment 556146 [details] [diff] [review]
part 1, v1: call preventDefault on scroll events processed by scrollbox

I'm not requesting review because I haven't run this through tryserver yet.
Comment 6 Markus Stange [:mstange] 2011-08-26 15:29:42 PDT
Created attachment 556147 [details] [diff] [review]
part 2, v1: don't start a swipe if the scroll event has been preventDefaulted
Comment 7 Markus Stange [:mstange] 2011-08-27 01:50:05 PDT
Comment on attachment 556146 [details] [diff] [review]
part 1, v1: call preventDefault on scroll events processed by scrollbox

This passes tryserver.
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-08-29 11:05:07 PDT
(In reply to comment #4)

I frankly don't understand these steps-to-reproduce.  Please provide more detail.

Markus, is this something fixed by your patch?
Comment 9 Steven Michaud [:smichaud] (Retired) 2011-08-29 11:13:16 PDT
Comment on attachment 556147 [details] [diff] [review]
part 2, v1: don't start a swipe if the scroll event has been preventDefaulted

If Neal is fine with part 1, then I'm fine with part 2.

I briefly tested this patch on OS X 10.7.1, and it does seem to fix this bug.

I have to admit I don't really understand why part 1 works, though.
Comment 10 Markus Stange [:mstange] 2011-08-29 11:37:36 PDT
(In reply to Steven Michaud from comment #8)
> I frankly don't understand these steps-to-reproduce.  Please provide more
> detail.

If you open lots of tabs in a Firefox browser window, the tab bar will overflow and provide scroll arrows. But clicking the arrows isn't the only way to scroll through tabs; you can also just use horizontal mouse wheel scrolling.

The regression reported in this bug is that horizontal touchpad scrolling in the tabbar gets stuck after the first scroll event and doesn't continue until after you lift you finger from the touchpad / magic mouse. That's because we start swipe tracking after the first scroll event, and consequently -[ChildView scrollWheel:] isn't called anymore, so we don't send scroll events to Gecko any more.

Gecko internally translates nsMouseScrollEvents into DOMMouseScroll events that can be processed in JavaScript. That's what the tabbar scrollbox does: It listens for DOMMouseScroll events and scrolls per-tab in response to the events. (This happens in scrollbox.xml.)

> Markus, is this something fixed by your patch?

Yes, the patches together fix the bug as reported. (At least that's what I thought until I wrote the explanation below!)

The "overflow" that's reported back on nsMouseScrollEvents only takes scrolling into account that was handled as part of Gecko's "default handling" of scroll events, i.e. any scrolling that was done under nsEventStateManager::PostHandleEvent. But the default handling doesn't do any scrolling in the tab bar case, because the XUL scrollbox has overflow:hidden set on it (i.e. it's marked as not-default-scrollable).

So the reported overflow is positive (because no scrolling took place as part of the default handling) and we start swipe-tracking.

But we don't want to do swipe-tracking in the tabbar case. We want all scroll events to pass through to the tab bar normally.

The patches do this: The scrollbox marks all scroll events it processes explicitly as "don't use Gecko's default scroll handling" using preventDefault(), and the change to nsChildView.mm makes us not start swipe-tracking in that case.
Whether an event has been preventDefault()ed is reflected in the event's state which is reported back from nsChildView::DispatchWindowEvent.

But now that I look at the patch again, I think I'm having one exclamation mark too many. Specifically, I think PRBool canceled = !mGeckoChild->Dispatch... should be
PRBool cancelled = mGeckoChild->Dispatch... (without the negation).
Let me look at this again.
Comment 11 Steven Michaud [:smichaud] (Retired) 2011-08-29 13:10:29 PDT
(In reply to comment #10)

RE my reply to comment #4:

Comment #4 appears (on the face of it) to describe a different bug than comment #0 and comment #3 (though possibly one that's related).  So I was asking for more detail about what's reported in comment #4.  I understand well enough what's reported in comment #3.

As for the rest of what you say, I await further developments :-)
Comment 12 Johan Larsson 2011-08-29 23:00:46 PDT
(In reply to Steven Michaud from comment #11)
> (In reply to comment #10)
> 
> RE my reply to comment #4:
> 
> Comment #4 appears (on the face of it) to describe a different bug than
> comment #0 and comment #3 (though possibly one that's related).  So I was
> asking for more detail about what's reported in comment #4.  I understand
> well enough what's reported in comment #3.
> 
> As for the rest of what you say, I await further developments :-)

Steps to reproduce: Scroll horizontally in tab bar with magic mouse.

Result: The currently active tab goes back one page (if scrolled right) and goes forward (if scrolled left and there is a page to go forward to).

I don't know how I can be any clearer than that but I also recorded a video of the behaviour I'm seeing when scrolling horizontally in the tab bar. :)

http://www.youtube.com/watch?v=TVsVVfsEqG8

The mouse is at the top right of the video and here I'm simply scrolling horizontally, back and forth.

Please let me know if I can make it even clearer. :)
Comment 13 Markus Stange [:mstange] 2011-08-30 02:15:06 PDT
Comment on attachment 556147 [details] [diff] [review]
part 2, v1: don't start a swipe if the scroll event has been preventDefaulted

OK, this patch is simply unnecessary. Scroll events always come back to nsChildView with status nsEventStatus_eConsumeNoDefault. That's either set when preventDefault() is called on them during DOM event handling, or at the end of default handling: http://hg.mozilla.org/mozilla-central/file/33031c875984/content/events/src/nsEventStateManager.cpp#l3264

Part 1 fixes this bug on its own. That's because the event's scrollOverflow is set during default handling, and if default handling is skipped, scrollOverflow stays zero. And for a zero scrollOverflow we don't start a swipe.
Comment 14 Steven Michaud [:smichaud] (Retired) 2011-08-31 15:36:55 PDT
(In reply to comment #12)

Thanks, this is much clearer -- especially with the video.

But I can't reproduce this with two-finger scrolling on the trackpad,
and I don't yet have a Magic Mouse to test with.

Needless to say this isn't what was reported in comment #0, though it
may be related.
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-08-31 15:37:57 PDT
Markus, is there a reason you haven't yet landed your part1 patch?
Comment 16 Johan Larsson 2011-09-01 02:00:09 PDT
(In reply to Steven Michaud from comment #14)

For what it's worth - I can reproduce this on the trackpad of my Macbook Air (separate computer from the iMac that recorded the video).

I was sent to this bug from bug 668953.
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-09-01 07:39:25 PDT
> I was sent to this bug from bug 668953.

Yes.  I was the one who sent you.  At that time I didn't understand
your STR, but thought it might be related to this bug.

> For what it's worth - I can reproduce this on the trackpad of my
> Macbook Air (separate computer from the iMac that recorded the
> video).

If Markus doesn't land his patch soon, I'll do a tryserver build with
the patch, and you can test that.  If his patch also fixes your
problem then everything's fine.  If not, I'll ask you to open a new
bug, and we can deal with your problem there.
Comment 18 Steven Michaud [:smichaud] (Retired) 2011-09-06 14:21:13 PDT
Johan, here's a tryserver build made with part 1 of Markus' patch.  Please try it out and let us know if it fixes your bug:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-afc1aabf720d/try-macosx64/firefox-9.0a1.en-US.mac.dmg
Comment 19 Johan Larsson 2011-09-06 22:56:29 PDT
(In reply to Steven Michaud from comment #18)

Fantastic, it fixes my bug.

I've only been able to test it on my Macbook Air with its integrated trackpad.

I will be able to test it with my Magic Mouse, on my iMac, tomorrow.
Comment 20 Markus Stange [:mstange] 2011-09-08 06:34:47 PDT
(In reply to Steven Michaud from comment #15)
> Markus, is there a reason you haven't yet landed your part1 patch?

Lack of time. Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8a9ade7fefb
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-08 14:03:36 PDT
http://hg.mozilla.org/mozilla-central/rev/a8a9ade7fefb
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:50:12 PDT
qa+ for verification in Firefox 8.

Note You need to log in before you can comment on or make changes to this bug.