Closed Bug 668953 Opened 13 years ago Closed 13 years ago

[10.7] Support two-finger horizontal swipe on OSX Lion

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox6 - wontfix
firefox7 + wontfix
firefox8 + wontfix
firefox9 + fixed

People

(Reporter: mazz0, Assigned: smichaud)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [qa!])

Attachments

(6 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:6.0a2) Gecko/20110701 Firefox/6.0a2
Build ID: 20110701042007

Steps to reproduce:

OS X Lion adds some new gesture styles for going back/forward by effectively scrolling the current page out of the way, just like you scroll around the current page.


Actual results:

Firefox doesn't support it


Expected results:

It should
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
I found same bug. Please fix it. Lion release is set to July 14!
Same here. Hope it get corrected soon, because it's really useful. Without it, Safari is way much faster using it with a MacBook or a desktop Mac using a Magic Trackpad.
There are some other gestures that will be wellcomed too: tap with two fingers to zoom to a section, two finger pinch to zoom...

Firefox integrates also a full-screen mode that even though is useful, is not Lion-like. Maybe it could get revised too.
Full screen bug is Bug 639705.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Support new back/forward gestures in OSX Lion → [10.7] Support new back/forward gestures in OSX Lion
OS: Other → Mac OS X
Version: 6 Branch → Trunk
This issue can be circumvented by setting “swipe left or right with *three* fingers” from the trackpad preferences.
While this may fix it for some, it impacts other functionality in the system that is assigned to the three finger swipe by default. It would be nice if Firefox supported the default two finger swipe.

(In reply to comment #5)
> This issue can be circumvented by setting “swipe left or right with *three*
> fingers” from the trackpad preferences.
If you change the default to three finger, then you loose the back/forward animation in Safari. Not a big deal, just mentioning.

It feels weird though that by default on a new configuration, Firefox does not support any kind of gesture to navigate.
I agree whole heartedly. I *personally* don't care all that much if the fancy page "reveal" navigation is present (as is in Safari), but it really should be a native function for FireFox to embrace all the same gestures as Safari does (by default).
My three finger up/down reveals Mission Control.  I would like the ZOOM/SHRINK with the Thumb/IndexFinger gesture for sure.  But all gestures should be supported.
My three finger up/down reveals Mission Control.  I would like the ZOOM/SHRINK with the Thumb/IndexFinger gesture for sure.  But all gestures should be supported.
This is the top request in SUMO for Lion support.
Assignee: nobody → bgirard
Depends on: 674610
Using Safari now instead of Firefox just because of this simple feature. Using three-finger drag now too so leaving default settings for trackpad in Lion. As of today build versions 6-8 don't support this feature either. Chrome's version 14 build supports it but it goes the wrong way!!!
No new features will be landing in Firefox 6 which is already in the final staged of Beta (think "RC")
Isn't this a bug though, not a feature? It's something that did work, does work on early OS X builds, that doesn't work now for the current OS.
It's definitely too late for Firefox 6 but let's try for Firefox 7.
We're not going to track this for 7, as it isn't really Firefox version specific (and we'd ship 7 as-is if this bug got zero forward progress). Cww will talk to product and engineering to try to get this prioritized due to support impact.

That being said, the "regression" came out between 5 and 6 so if there is a patch we will generally treat this as a regression fix when it comes to approvals.
I'm working on a patch to support back/forward using two finger swipe as top priority. This change will only be applied to 10.7+. Since this is also used for scrolling the page horizontally, the back/forward action will only be triggered if the respective edge of the horizontal scroll bar has been reached.

I suggest that we file a follow up bug to get a behavior that resembles safari's page scrolling back/forward animation to better support the OS X 10.7+ look-and-feel. We should contact the UI team to discuss if we would want this change on other platforms.
(In reply to comment #17)
> I'm working on a patch to support back/forward using two finger swipe as top
> priority.

As an iMac/Magic Mouse user, I'd like to ask whether that will include the equivalent Magic Mouse gesture. (one-finger swipe left/right to go back/forward.)

Or should that be filed as a separate bug?
File a separate bug describing the current behavior and the expected, CC me and I will make it block lion compatibility improvements.
Blocks: 676169
I have a working patch, just fixing up certain cases.
Attached patch patch v1 (obsolete) — Splinter Review
I wasn't sure who to flag for review since this code comes from the initial hg import.

I added nsCocoaFeatures and hope in a follow up patch to replace os version check with this.
Attachment #550592 - Flags: review?(smichaud)
One thing that came up during my testing is how Gecko and webkit (chrome) handle 2 finger horizontal scrolling so differently which directly affects the usability of this patch.

See the testcase for a good example. In both implementation the nested most view that can scroll will handle the event. Once the nested view has reached the edge in webkit the parent view will then respond to the event immediately and begin scrolling. In Gecko the first nested view that can respond to the event will start a 'mouse wheel scroll transaction' where all further events will be sent to that view even after it has reached the edge. The transaction will remain for 1500ms after which it is cleared.

What this means for example in the test case is if you start in a deeply nested view and try to scroll them all and try to perform a forward gesture you will have to scroll each nested view to the edge, pause for 1500ms, repeat until all views are scrolled to the edge before all views reject the scrolling and the event is passed as a forward action. In chrome you can simply scroll all views in one motion and even trigger the forward action.

In a nutshell one way allows the use of horizontal views to quickly control nested views and trigger back/forward however is susceptible to accidentally triggering forward/back by over scrolling. The other way is slower but is less prone to accidental triggering.
Do we have an opinion on which behavior we prefer?
Comment on attachment 550592 [details] [diff] [review]
patch v1

This is an Apple bug (or perhaps a design flaw):  OS X 10.7 doesn't
tell us (or any other app, including Safari) when a two-finger
horizontal gesture is an ordinary scroll event and when it's a swipe.
More on this in a later comment.

Benoit's patch is fine with me (though I do have a couple nits to
pick).  But it has some glitches, which will probably take a
significant amount of time to fix.  So we'll have to decide whether we
want to get a quick but rather clumsy fix (like Benoit's patch) into a
release as soon as possible, or wait until we have something that
functions more smoothly.  I don't think we can reasonably include a
fix for this bug in FF 6 -- so even a quick fix would appear in FF 7,
at the earliest.  More on this in another later comment.

In the meantime I think we should land Benoit's patch on the trunk
right away, so that people can test it.  Especially all the me-too
commenters on this bug :-)

Here are my nits:

There are a bunch of symbols with "TriggeredGesture" in their names.
I think the patch would be easier to understand if these were all
changed to "GestureTriggered".

In nsCocoaFeatures::OSXVersion() we should mask out the top word of
mOSXVersion as "returned" by Gestalt().
Attachment #550592 - Flags: review?(smichaud) → review+
As I mentioned above in comment #26, OS X 10.7 doesn't provide any
support for the two-finger horizontal swipe.  As best I can tell, its
behavior is exactly the same for a two-finger swipe as it is for a
two-finger scroll event -- it calls scrollWheel:,
beginGestureWithEvent: and endGestureWithEvent: on the focused NSView.

So, as things now stand, every app that wants to support the
two-finger horizontal swipe on OS X 10.7 needs to implement its own
solution.  Safari, as you'd expect, already implements it quite well
-- they've already had a lot of time to iron out the wrinkles.  But
it's a different story for Firefox and Chrome.

Google Chrome doesn't yet have support for the two-finger horizontal
swipe.  But the Chromium developers are working on it, and a patch has
been landed in Google Chrome Canary
(http://tools.google.com/dlpage/chromesxs).

Here's the bug:
http://codereview.chromium.org/7488023

You'll notice that Canary's support for the two-finger horizontal
swipe is also somewhat glitchy -- much less smooth than Safari's, or
than Chrome's and Firefox's support for the three-finger horizontal
swipe on OS X 10.6.  More on this in a later comment.

If you look at the patch for the above-mentioned bug, you'll notice
that Chrome's current fix for the two-finger horizontal swipe problem
hooks the OS's calls to beginGestureWithEvent: and
endGestureWithEvent:.  Safari, interestingly, doesn't use either of
these -- instead it hooks the OS's call to process a scroll wheel
event (scrollWheel:).  Here's a stack trace of (part of) what Safari
does to implement the two-finger horizontal swipe:
Attachment #550592 - Attachment is obsolete: true
(Following up comment #27)

Mozilla, Chrome and other app vendors will implement workarounds for
this bug,  but it's only Apple who can really fix it.

Of course it'd be best for OS X to call swipeWithEvent: on the focused
NSView -- as it did for three-finger horizontal swipes on OS X 10.6,
and as it still does for three-finger horizontal swipes on OS X 10.7
if you follow simo's suggestion from comment #5.

But Apple could also provide sample code showing how Safari implements
the two-finger horizontal swipe.
FWIW I highly doubt Apple would add new APIs in a point release for 10.7, though with comment #5 this actually sounds like a bug/oversight in their implementation that has a chance.
Blocks: 676907
(In reply to comment #30)

> FWIW I highly doubt Apple would add new APIs in a point release for
> 10.7

I agree with you.  Apple's always been careful about making API
changes in minor releases, and will (I hope) continue to be so.  But
it'd be very nice if they could come up with a backwards-compatible
API change -- one that would send swipeWithEvent: on a two-finger
horizontal swipe, but wouldn't (for example) break Safari's or
Chrome's or Firefox's reliance on the "old" APIs.

Note also that Apple, in order to support the horizontal two-finger
swipe at the OS level, would need to get more information from the
app, presumably from a callback (on things like where the mouse is in
the current document, and how far it is from any of the document's
edges).
(In reply to Steven Michaud from comment #29)
> Mozilla, Chrome and other app vendors will implement workarounds for
> this bug,  but it's only Apple who can really fix it.

In Safari two finger forward and back gestures are essentially _scrolling_ from page to another. This kind of smooth swiping cannot be implemented with single event, so I suppose it's intentionally left out from the API.

Three finger swipe is a different thing: swipe to left goes back event even when natural scrolling is on.

Suggested patch simulates three finger gesture which is of course better than nothing but does not feel like scrolling so the metaphor is not complete.
(Following up comment #27)

> Here's a stack trace of (part of) what Safari does to implement the
> two-finger horizontal swipe:

Oops, forgot to include this.

(gdb) bt
#0  0x00007fff8a459f92 in -[TabContentView beginSwipeGestureWithEvent:] ()
#1  0x00007fff8a2c68cd in -[BrowserWKView performGestureWithScrollEvent:] ()
#2  0x00007fff8a2c67b2 in -[BrowserWKView scrollWheel:] ()
#3  0x00007fff87d22ad2 in -[NSWindow sendEvent:] ()
#4  0x00007fff8a4a00c5 in -[Window sendEvent:] ()
#5  0x00007fff8a2a77e8 in -[BrowserWindow sendEvent:] ()
#6  0x00007fff87cbaae8 in -[NSApplication sendEvent:] ()
#7  0x00007fff8a25047a in -[BrowserApplication sendEvent:] ()
#8  0x00007fff87c5142b in -[NSApplication run] ()
#9  0x00007fff87ecf52a in NSApplicationMain ()
#10 0x00007fff8a402725 in SafariMain ()
#11 0x0000000101234f24 in ?? ()
(gdb)
Landed patch v2 on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/57446cb82caa

Wont mark the bug close since we're still considering an alternate fix and/or landing on aurora.
(In reply to comment #32)

> In Safari two finger forward and back gestures are essentially
> _scrolling_ from page to another.

This is one way to think about it, but I don't think it's the best
way.

As I see it, Apple has (to use programmers' jargon) "overloaded" the
two-finger horizontal gesture on OS X 10.7 -- depending on the
context, it means either a scroll event or a swipe event.  The biggest
problem is "how do you decide?".  And I think it's best to have the OS
make the decision -- at least by default (with the option for the app
to override it).  Otherwise you risk getting subtly different
behaviors in every app, and rather broken behaviors in at least some
of them.
Since this is holds a very logical argument for Apple to add this to the OS API, has anyone pushed this over to Apple to get a response?
(In reply to Steven Michaud from comment #35)
> (In reply to comment #32)
> 
> > In Safari two finger forward and back gestures are essentially
> > _scrolling_ from page to another.
> 
> This is one way to think about it, but I don't think it's the best
> way.

System Preferences gives the option to make the gesture "Swipe left or right with three fingers" or "Scroll left or right with two fingers", purposefully labeling it as a scroll gesture, not a swipe. In Safari, the gesture behaves as a scroll, since you can "scroll" back halfway, see half of the previous page revealed, then scroll back to the current page. Can swipeWithEvent even produce this functionality?
I have a list of bugs I will be taking to the Mac OS X Updates team shortly.
(In reply to comment #37)

> System Preferences gives the option to make the gesture "Swipe left
> or right with three fingers" or "Scroll left or right with two
> fingers", purposefully labeling it as a scroll gesture, not a swipe

But these two options (and a third called "swipe with two or three
fingers") are all under the heading "Swipe between pages".

> In Safari, the gesture behaves as a scroll, since you can "scroll"
> back halfway, see half of the previous page revealed, then scroll
> back to the current page.

Not always -- sometimes the gesture behaves as a swipe.  And note that
these are mutually exclusive -- a scroll isn't a swipe, and a swipe
isn't a scroll.

> Can swipeWithEvent even produce this functionality?

Of course not.  This is used only for a swipe.

My whole point is that the OS should decide when a two-finger
horizontal gesture is a swipe, and when it's a scroll.  When it's a
swipe, the OS would call swipeWithEvent:.  Otherwise it'd (presumably)
call scrollWheel: (and beginGestureWithEvent: and
endGestureWithEvent:).
Here's a quick review of the glitches I see doing horizontal
two-finger gestures on OS X 10.7, using monolithic pages (not nested
views, like Benoit's testcase from comment #21).

In all of these tests, I first started the browser, closed all
windows, then quit it -- to clear prior history as much as possible.
Then I started the browser again and entered "mozilla.org" in the
location bar (to visit http://www.mozilla.org/), then entered
"mozilla.com" in the location bar (to visit
http://www.mozilla.com/en-US/firefox/new/), then entered
"http://www.mozilla.com/en-US/about/" in the location bar.

I tested with Firefox as patched by Benoit's patch, Google Chrome
Canary, and Safari.  Of course I tested on OS X 10.7 (the release
version).

I've said previously that Safari implements two-finger horizontal
swipes more smoothly than Canary or Firefox (with Benoit's patch).  I
still think this is true, but this time I noticed glitches even in
Safari's implementation.

All three browsers behave best when you maximize the browser window
(when the page being viewed is no wider than the window, and you never
see a horizontal scroll bar).  In this case I didn't notice any
problems with Safari.

But Firefox has the following problems, and section (b) describes a
problem with Canary:

a) If you swipe to one page, you need to wait a couple of seconds
   before you can swipe to another -- if you don't wait nothing
   happens.

b) You won't get a swipe (visit another page) if your two-finger
   horizontal gesture contains any vertical element, even a very small
   one.

   Safari seems to allow a small vertical element.  But if it's too
   large, Safari will just scroll vertically.

   Canary will swipe to another page even if your two-finger gesture
   contains a very large vertical element -- which I suppose is itself
   a bug.

More problems appear, in all browsers, if the browser window is
narrower than the page being viewed, and you (at least sometimes) see
the horizontal scroll bar.

In this case Firefox had this additional problem (which I'm not sure
really is a problem):

a) When you swipe to a page, the page being swiped to also ends up
   scrolled horizontally all the way in the direction of the original
   swipe.

   As I said, I'm not sure this is really a bug.  But neither Canary
   nor Safari do this.

Canary has the following additional problem:

a) When scrolling a page horizontally (in a single gesture), you'll
   end up swiping to the next (or previous) page if you scroll too
   far.

   This doesn't happen in either Firefox or Safari.

Safari mostly works fine, but at least in my tests has the following
rather subtle problem:

a) Sometimes, when trying to swipe rapidly back and forth between
   pages, a swipe will fail (you see a bounce but the same page stays
   visible).
(Following up comment #40)

As comment #40 shows, I'm willing to put time and effort into UI work.
But I don't really trust myself to make UI decisions.  And we need to
decide whether to proceed with Benoit's patch (with perhaps some minor
changes), or to wait until we've had time to achieve a Safari-like
smoothness in our implementation of two-finger horizontal swipes.

In other words, do we release FF 7 with imperfect support for
two-finger horizontal swipes, or do we wait until FF 8 or 9 to
implement better support for two-finger horizontal swipes?

These needn't be mutually exclusive -- we could do both.  But someone
needs to make the call on whether Benoit's patch (or something very
like it) is "good enough" to include in a release (any release).

I don't know whether Alexander Limi is the person to make this
decision.  But I know he's interested in UI stuff (and usability), and
might be able to suggest someone else.
Keywords: uiwanted
(In reply to Steven Michaud from comment #41)
 
> I don't know whether Alexander Limi is the person to make this
> decision.  But I know he's interested in UI stuff (and usability), and
> might be able to suggest someone else.

This is some combination of me, UX, and the feature owner. I don't have a mac (just put in the request) so that I can test this and see what it's like so I'll either need to delegate or you all will have to wait until Monday when I can get in front of a Mac with Lion on it.
Summary: [10.7] Support new back/forward gestures in OSX Lion → [10.7] Support two-finger horizontal swipe on OSX Lion
(In reply to comment #42)

Thanks for the info.

This bug is somewhat urgent (we need to decide whether Benoit's patch,
or something like it, should land on Aurora).  But surely the decision
can wait until Monday.

By the way, I hope and assume you're familiar with OS X generally, and
just haven't used Lion :-)
(In reply to Steven Michaud from comment #43)
> (In reply to comment #42)
> 
> Thanks for the info.
> 
> This bug is somewhat urgent (we need to decide whether Benoit's patch,
> or something like it, should land on Aurora).  But surely the decision
> can wait until Monday.
> 
> By the way, I hope and assume you're familiar with OS X generally, and
> just haven't used Lion :-)

Yes. I've spent more of my Mozilla years on a Mac than PC. I'm of late on a PC because that's where the overwhelming majority of our users are (I'm even on XP about 30% of the time) but I haven't been on Lion yet, though. 

I'm inclined to take an imperfect solution into 7 and get something better for 8 and or 9. But I want to make sure it isn't worse than no solution before I say go. You and Limi should help me make that decision.
When testing this bug try changing the values of |mousewheel.transaction.timeout| to adjust the pause delay. Setting the value to 0 makes our scrolling behavior behave similar to canary.
(In reply to Steven Michaud from comment #27)
> As I mentioned above in comment #26, OS X 10.7 doesn't provide any
> support for the two-finger horizontal swipe.  As best I can tell, its
> behavior is exactly the same for a two-finger swipe as it is for a
> two-finger scroll event -- it calls scrollWheel:,
> beginGestureWithEvent: and endGestureWithEvent: on the focused NSView.

See the AppKit Release Notes section "Fluid Swipe Tracking - API (New since early 2011 seed)" http://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html

In short the API you want is - (BOOL)wantsScrollEventsForSwipeTrackingOnAxis:(NSEventGestureAxis)axis;
(In reply to comment #46)

I'm not sure I understand.  Are you saying that we can do what we
normally do in [ChildView swipeWithEvent:], in the handler passed to
[NSEvent trackSwipeEventWithOptions:...], when isComplete is true and
gestureAmount is either -1 or 1?

If so then thanks -- this is (more or less) the equivalent of the OS
calling [ChildView swipeWithEvent:].

It's interesting, though, the Safari doesn't seem to do this.
(Following up comment #47)

> It's interesting, though, the Safari doesn't seem to do this.

I was wrong -- Safari *does* do this.  Here's a stack trace of the
first call to [NSEvent trackSwipeEventWithOptions:...]:

(gdb) bt
#0  0x00007fff89ce6db1 in -[NSEvent trackSwipeEventWithOptions:dampenAmountThresholdMin:max:usingHandler:] ()
#1  0x00007fff8c024d37 in -[GestureSnapshotSwipeView swipeStackWithScrollEvent:] ()
#2  0x00007fff8bf718cd in -[BrowserWKView performGestureWithScrollEvent:] ()
#3  0x00007fff8bf717b2 in -[BrowserWKView scrollWheel:] ()
#4  0x00007fff899cdad2 in -[NSWindow sendEvent:] ()
#5  0x00007fff8c14b0c5 in -[Window sendEvent:] ()
#6  0x00007fff8bf527e8 in -[BrowserWindow sendEvent:] ()
#7  0x00007fff89965ae8 in -[NSApplication sendEvent:] ()
#8  0x00007fff8befb47a in -[BrowserApplication sendEvent:] ()
#9  0x00007fff898fc42b in -[NSApplication run] ()
#10 0x00007fff89b7a52a in NSApplicationMain ()
#11 0x00007fff8c0ad725 in SafariMain ()
#12 0x00000001050a1f24 in ?? ()
(gdb)
Safari doesn't implement the wantsScrollEventsForSwipeTrackingOnAxis: method anywhere, though.
I had to backout Bug 668953 and Bug 673440 from inbound because one of the two caused a Tp5 and Tp5 RSS regression on OSX 10.6.2. I don't know which of the two, if you have an idea you may reland the other one separately.
Relanded since this wasn't the cause of the Tp5:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ca50e8b3d37
Whiteboard: [inbound]
(In reply to Benoit Girard (:BenWa) from comment #22)
> I added nsCocoaFeatures and hope in a follow up patch to replace os version
> check with this.

The normal way of checking for platform-specific behavior in cross-platform code is to add a new nsLookAndFeel metric. Then you'd have the OnLionOrLater() check in nsLookAndFeel.mm which can use nsToolkit::OnLionOrLater().

I'm also curious why a patch to nsEventStateManager.cpp was landed without review from smaug.
I don't think the patch is correct. It seems to break tilt wheel's behavior of third party's mice, isn't it?

Why don't you dispatch nsCommandEvent with nsWidgetAtoms::Back or nsWidgetAtoms::Forward from nsChildView.mm? Your patch causes both horizontal mouse wheel event and back/forward. Is this intentional?
(In reply to Markus Stange from comment #53)
> I'm also curious why a patch to nsEventStateManager.cpp was landed without
> review from smaug.

I wasn't sure who to ask for review and |hg blame| wasn't helpful since this is old code. I'll ping him for a review.

(In reply to Masayuki Nakano (Mozilla Japan)(away 8/10 - 8/12) from comment #54)
> I don't think the patch is correct. It seems to break tilt wheel's behavior
> of third party's mice, isn't it?
>
> Why don't you dispatch nsCommandEvent with nsWidgetAtoms::Back or
> nsWidgetAtoms::Forward from nsChildView.mm? Your patch causes both
> horizontal mouse wheel event and back/forward. Is this intentional?
 
That's not intentional, I didn't think there were devices with an horizontal scroll wheel. I don't trigger the even from nsChildView.mm since we need to know if none of the views can be scrolled by the event before triggering a back/foward.
(In reply to Benoit Girard (:BenWa) from comment #55)
> That's not intentional,

Um, It might be good thing that the wheel event can consume the back/forward gesture. E.g., on web applications.

> I didn't think there were devices with an horizontal scroll wheel.

You need to check it. I don't think that tilt wheel causes the back/forward even if the page cannot scroll anymore.

> I don't trigger the even from nsChildView.mm since we need to
> know if none of the views can be scrolled by the event before triggering a
> back/foward.

Ah, I see. You can implement NS_QUERY_SCROLLABLE_DIRECTION_AT_POINT event or expanding NS_QUERY_SCROLL_TARGET_INFO event. I recommend the former because the latter implementation is very sensitive, but anyway, it's very complex. Let us find simpler approach...

Cannot you use nsMouseWheelEvent::scrollOverflow? If you can use it, you can dispatch the command event from nsChildView.mm after nsMouseWheelEvent.

And also, did you test iframe case? We have complex wheel transaction mechanism. You might not know it. If so, you should read it.
https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Gecko_honors_the_mouse_cursor_position
https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Mouse_wheel_transaction

I think that the patch should be backed out if it broke tilt wheel behavior.
(In reply to Benoit Girard (:BenWa) from comment #55)
> I didn't think there were devices with an horizontal scroll wheel.

The first generations MacBooks (the ones with a physical button on their trackpads - they weren't multitouch trackpads but only recognized two-finger scrolling) don't trigger the Forward/Back events in Safari, even though they have horizontal two-finger scrolling.  I just tested it on a white mid 2008 MacBook with Lion installed.
Those MacBook trackpads are considered "legacy" devices by the OS, as they don't cause rubber-band scrolling effects, don't have momentum scrolling and cause the legacy scrollbars to appear when they're set to be shown depending on the input device.
http://hg.mozilla.org/mozilla-central/rev/1ca50e8b3d37
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #55)

> (In reply to Markus Stange from comment #53)
>> I'm also curious why a patch to nsEventStateManager.cpp was landed
>> without review from smaug.
>
> I wasn't sure who to ask for review and |hg blame| wasn't helpful
> since this is old code. I'll ping him for a review.

I goofed, too -- I should have thought to ask smaug for a review.

But in any case, I think we should explore the possibility that
Colin's suggestion from comment #46 (and my interpretation of it in
comment #47) will work out.  If it does, it would presumably obsolete
Benoit's original patch.

I'll dig into it to see what I can find.  I should have more to say
later today.
(In reply to Steven Michaud from comment #59)
> If it does, it would presumably obsolete
> Benoit's original patch.
> 

Let me know and I'll be happy to back out my patch.
(Following up comment #59)

> I'll dig into it to see what I can find.  I should have more to say
> later today.

I don't yet have a patch.  But neither have I ruled out the possibility that the approach outlined in comment #47 will work.

One serious limitation of is already apparent -- it will only work in code compiled against the 10.6 SDK (or higher).  So it will only work in 64-bit mode.

Another problem is that, to get it to work, I'll need to know in widget code when the page is scrolled all the way to the left or the right.  In past experience, trying to get this kind of information has been excruciatingly difficult (and is also sometimes considered illegitimate).  Anyone have any suggestions?
(In reply to Steven Michaud from comment #61)
> I'll need to know in widget code when the page is scrolled all the way to
> the left or the right.  In past experience, trying to get this kind of
> information has been excruciatingly difficult (and is also sometimes
> considered illegitimate).

I think that you should dispatch a mouse wheel event first. Then, cannot you use nsMouseScroll::scrollOverflow?
# I'm not sure whether you can dispatch it at that time.
(In reply to comment #62)

Thanks!  I'll try this tomorrow.
I'm making good progress on my patch, but I'm still not quite satisfied with it.  So I'll continue working on it tomorrow.
I want to see your patch, would you attach it?
I'll do that tomorrow :-)
Here it is.

A tryserver build should be available in a few hours.

Colin's suggestion from comment #46 (like the doc it refers to) is
incorrect/irrelevant:  [NSResponder
wantsScrollEventsForSwipeTrackingOnAxis:] has nothing to do with
fixing this bug -- possibly because we don't use NSScrollView in our
browser windows.  But the Apple doc pointed me in the right direction,
for which I'm very grateful.

[NSEvent trackSwipeEventWithOptions:...] doesn't provide the solution
I was originally looking for -- the app still has to decide (on its
own) whether a given horizontal two-finger gesture is either a scroll
or a swipe.  But once that decision is made (once we know we have a
swipe), this method does a nice job of tracking a swipe to completion.

Masayuki, your suggestion to use nsMouseScrollEvent.scrollOverflow
works brilliantly.  But it does have one serious quirk -- Gecko only
sets scrollOverflow for NS_MOUSE_PIXEL_SCROLL events (not for
NS_MOUSE_SCROLL events).

As I mentioned earlier, my patch's code only runs in 64-bit mode.
This is because one of [NSEvent trackSwipeEventWithOptions:...]'s
parameters is a "block"
(http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Blocks/Articles/00_Introduction.html),
and "blocks" are only supported on OS X 10.6 and up, when using the
10.6 SDK or higher (a combination which is currently only available
when we compile for 64-bit mode).

I don't think this is a big problem:  As far as I know Lion requires
64-bit hardware.  And FF defaults to using 64-bit mode on 10.6 and up
-- so that's what the vast majority of Lion users will be doing.

As best I can tell, my patch doesn't have any of the problems I
described in comment #40 -- even the problem with rapidly swiping back
and forth between two pages that I found in Safari.  But my patch
still needs testing.

Masayuki, please let us know if you have any trouble with it.

I'd also like to hear from those who really want support for
two-finger horizontal swipes, and are (presumably) connoisseurs of
Apple's new Lion UIs.

No, my patch doesn't support swipe animation.  But I hope it'll
eventually be possible to use [NSEvent trackSwipeEventWithOptions:...]
to trigger the animation (once the browser can do it).
Attachment #552212 - Flags: review?(bgirard)
By the way, I've been testing my patch on a brand-new MacBook Pro -- which probably has full support for all of Lion's bells and whistles.  Later I'll try with a 3-year-old MacBook Pro.

I also need to test with a Magic Mouse (which I don't yet own).
Comment on attachment 552212 [details] [diff] [review]
Alternate patch (use [NSEvent trackSwipeEventWithOptions:...])

Review of attachment 552212 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch, the code looks great. Your approach is much better then mine. I will try the look and feel of it tonight.

::: widget/src/cocoa/nsChildView.mm
@@ -2979,0 +2984,47 @@
> > +// Support fluid swipe tracking on OS X 10.7 and higher.  We must be careful
> > +// to only invoke this support on a horizontal two-finger gesture that really
> > +// is a swipe (and not a scroll) -- in other words, the app is responsible
> > +// for deciding which is which.  But once the decision is made, the OS tracks
NaN more ...

This code is only getting compiled under 64-bit where CGFloat is actually a double. We should use fabs() instead.
> This code is only getting compiled under 64-bit where CGFloat is
> actually a double. We should use fabs() instead.

Oops, glad you noticed this.

I'll fix it in the next revision, or on landing.
(In reply to Steven Michaud from comment #67)
> Created attachment 552212 [details] [diff] [review]
> Alternate patch (use [NSEvent trackSwipeEventWithOptions:...])
> 
> Here it is.
> 
> A tryserver build should be available in a few hours.
> 
> Colin's suggestion from comment #46 (like the doc it refers to) is
> incorrect/irrelevant:  [NSResponder
> wantsScrollEventsForSwipeTrackingOnAxis:] has nothing to do with
> fixing this bug -- possibly because we don't use NSScrollView in our
> browser windows.

Ahhhh, makes complete sense.

> But the Apple doc pointed me in the right direction,
> for which I'm very grateful.

Awesome!

> [NSEvent trackSwipeEventWithOptions:...] doesn't provide the solution
> I was originally looking for -- the app still has to decide (on its
> own) whether a given horizontal two-finger gesture is either a scroll
> or a swipe.  But once that decision is made (once we know we have a
> swipe), this method does a nice job of tracking a swipe to completion.
> 

Great! I tried the tryserver build. It seems to work quite well. Modulo the animations it's very similar to Safari. Can't wait to see the animations in there! :)
Actually I've noticed one minor but disconcerting problem with my patch:

Sometimes, after you've done a swipe to change from page A to page B (but before the OS has finished tracking the swipe, and so before you've actually changed to page B), you're able to scroll page A.  So it's a bit surprising when the change to page B finally happens.

Scrolling page A should (I think) cancel the swipe to page B.  I think I know how to do this.  I'll try it out tomorrow, and if it works I'll post a revised patch.
This try build seems to work well for me, minus the nice animation of course.
The try build seems good on magic trackpad and magic mouse.  The feel seems similar to safari's on both devices to me.  I haven't tried it on 'legacy' pointing devices yet though.
Comment on attachment 552212 [details] [diff] [review]
Alternate patch (use [NSEvent trackSwipeEventWithOptions:...])

Works great for me as well.
Attachment #552212 - Flags: review?(bgirard) → review+
Attachment #551102 - Attachment is obsolete: true
Here's a revised patch.  It fixes the problems Benoit reported in
comment #69 and I reported in comment #73.  It also cleans up the
comments a bit.

I've tested it on my brand-new MacBook Pro, and it doesn't seem to
have introduced any new problems.

A tryserver build should be available in a few hours.  Once it's
available please try it out -- especially all those who tested my
previous build.
Assignee: bgirard → smichaud
Attachment #552212 - Attachment is obsolete: true
Attachment #552411 - Flags: review?(bgirard)
Whiteboard: [inbound]
Attachment #552411 - Flags: review?(bgirard) → review+
Just now I tested my rev1 patch on my 3-year-old MacBook Pro.  Yesterday I also tested my original patch.  In neither case did I notice any difference in behavior (bewteen the two different MacBook Pros).

I'm not sure my 3-year-old MacBook Pro is old enough to be handled differently by Lion, though.
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/420c7a55b4a0

I landed this directly on trunk to get it out to users as quickly as possible.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

There's been talk of trying to get a fix for this bug into FF 7.  For that to happen, this patch needs to land on aurora by (I think) the beginning of next week.

My patch has moderate risk:  It uses a lightly documented, new API that (as far as I know) isn't yet used by anyone else outside of Apple.  So, even though our tests have gone very well up to this point, there could easily be quirks to [NSEvent trackSwipeEventWithOptions:...] that we haven't yet discovered.

So I'd normally want my patch to go through the standard schedule of testing by users.

But there's also high demand that we support the two-finger horizontal swipe on Lion.  So we should consider landing this on aurora right now, and backing it out if we notice any problems.
Attachment #552411 - Flags: approval-mozilla-aurora?
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

(Following up comment #80)

If need be, I can add code to my patch that would allow it to be preffed off.
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

Steven, release drivers have agreed to give this a shot in late game aurora. This is not something that anyone wants to do but we're willing to try and if anything turns up, do a quick back-out. So, it's a one-shot deal. If you want to get any more eyes on it first, please do that but try to get it landed before the weekend.
Attachment #552411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Here's a tryserver build made with my rev1 patch (from comment #77):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-39738b8a2f28/try-macosx64/firefox-8.0a1.en-US.mac.dmg

Please try it, everyone, and let us know your results.

If nobody finds problems, I'll land my rev1 patch on aurora tomorrow.
This appears to work exactly as I would expect.

The fancy transitions would be a huge bonus but prolly not necessary.
Would it make sense to create a separate bug for the transition animation, so the discussion won't become cluttered? I'd also like to see it but I'm not sure if it can be accomplished and/or if this is a generally wanted feature.
(In reply to comment #85)

> Would it make sense to create a separate bug for the transition animation?

Yes.

Go ahead and open a new bug on that subject.

Mention that it was spun off from this bug, put "[10.7]" at the beginning of the description line, and make it block bug 636455.

I've *no* idea how much work would be involved in adding support for the animation.  But it might be a lot.  And (of course) the more work required, the less likely it'll happen anytime soon.

But the first step is to open a new bug ... and that's easy :-)
OK, I created Bug 678392 which was immediately marked as duplicate of this bug. I'm not sure if this was intended or not.
Fixing this bug introduced a regression on my system. I'm unable to compile m-c on Lion anymore with this error stack: http://pastebin.mozilla.org/1297343

Reverting the patch fixes the regression. Should I file a new bug?
(In reply to Zbigniew Braniecki [:gandalf] from comment #90)
> Fixing this bug introduced a regression on my system. I'm unable to compile
> m-c on Lion anymore with this error stack:
> http://pastebin.mozilla.org/1297343

We need to conditionally define the 10.7 symbols for 10.6 and lower.
Odd.  I had no problems compiling this patch on OS X 10.7.

Which build of 10.7 do you have (mine is the GM, build 11A511)?

And which build of XCode 4.1 (mine is 4B95)?
MacOS Lion: 11A511
XCode 4.1: 4B110

Can you launch App Store to see if you have the update to XCode available?
Actually I already have the XCode update -- I just haven't installed it yet (I'd forgotten).  And you don't have to get it from the App Store -- it's also available from ADC.

I'll install it and see what happens.

But Benoit is right -- to be safe we should only define the 10.7 symbols ourselves on 10.6 and lower.  I'll look into doing that.
> But Benoit is right -- to be safe we should only define the 10.7
> symbols ourselves on 10.6 and lower.  I'll look into doing that.

Actually we'll need to define them when using the 10.6 *SDK* or lower.
It isn't currently possible to tell which SDK we're using in compiled
code, so we'll need to change configure.in to make that possible.

So while not a difficult change	to make, this isn't trivial.

So please, Gandalf, do open new	bug on this, and CC me and Benoit :-)
Oh, and now I see how you can work around your problem, Gandalf -- just use the 10.6 SDK when compiling on OS X 10.7.  Now that I think of it, this is also what I was doing (and why I didn't see your errors).
Can't we use #ifdef __MAC_OS_X_VERSION_MAX_ALLOWED < |lion| to only compile it for 10.6-?

http://developer.apple.com/library/ios/#documentation/DeveloperTools/Conceptual/cross_development/Using/using.html#//apple_ref/doc/uid/20002000-SW6
/Developer/SDKs/MacOSX10.6.sdk/usr/include/AvailabilityInternal.h
I will look into that, but it seems wrong on the face of it -- we do want the same binary to run on Lion (and above), after all.
(In reply to Steven Michaud from comment #99)
> I will look into that, but it seems wrong on the face of it -- we do want
> the same binary to run on Lion (and above), after all.

I think they are just poorly named. Unless we overwrite them (which we don't) they default to the SDK you are building with, which is what you wanted in comment #96. We already use this else where in the code base. See bug 675008.
OK, I'll try that tomorrow, at bug 678423.
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

Landed on aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/bcd26d45856a

I took the liberty of including my patch for bug 678423, since that allows building on OS X 10.7 with the 10.7 SDK.
Depends on: 678607
Comment on attachment 552411 [details] [diff] [review]
Alternate patch rev1 (fix problems)

>+// Support fluid swipe tracking on OS X 10.7 and higher.  We must be careful
>+// to only invoke this support on a horizontal two-finger gesture that really
>+// is a swipe (and not a scroll) -- in other words, the app is responsible
>+// for deciding which is which.  But once the decision is made, the OS tracks
>+// the swipe until it has finished, and decides whether or not it succeeded.
>+// A swipe has the same functionality as the Back and Forward buttons.  For
>+// now swipe animation is unsupported (e.g. no bounces).  This method is
>+// partly based on Apple sample code available at
>+// http://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html
>+// (under Fluid Swipe Tracking API).
>+#ifdef __LP64__
>+- (void)maybeTrackScrollEventAsSwipe:(NSEvent *)anEvent
>+                      scrollOverflow:(PRInt32)overflow
>+{

>+
>+  // Only initiate tracking if the user has tried to scroll past the edge of
>+  // the current page (as indicated by 'overflow' being non-zero).  Gecko only
>+  // sets nsMouseScrollEvent.scrollOverflow when it's processing
>+  // NS_MOUSE_PIXEL_SCROLL events (not NS_MOUSE_SCROLL events).
>+  // nsMouseScrollEvent.scrollOverflow only indicates left or right overflow
>+  // for horizontal NS_MOUSE_PIXEL_SCROLL events.
>+  if (!overflow) {
>+    return;
>+  }

I think that this isn't correct. E.g., only one pixel is overflowed for 10px scroll event, it may cause both scroll and back/forward. I think that you should check whether the scrollOverflow and dispatched delta value are same or not.

And also, I think that we should not set scrollOverflow when nsMouseWheelTransaction consumes the scroll event on the latest scrolled frame. For example, even if the scrollOverflow was same as the dispatched event delta, the viewport could be scrollable when mouse cursor is in scrollable iframe or textarea.
I'm not really sure this is related but timing points to that.

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 in a single scroll animation and stop responding to scroll motion until I remove fingers and start scrolling again.
> 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 in a single scroll animation
> and stop responding to scroll motion until I remove fingers and
> start scrolling again.

This shouldn't be happening.  Please open a new bug on it and make it
block this one.
Depends on: 678834
Just tested this in Aurora.  The delay is noticeable and *very* annoying.  It's just long enough that you have time to question whether or not the gesture "took" (sometimes even enough time that I try the gesture a second time).  If a proper back/forward page swipe animation is coming in our near future, then it's probably not worth worrying about the delay (since that will need to be touched anyway).  However, if this is something we will need to live with for a while, then I think it's a more serious concern.
Target Milestone: mozilla8 → mozilla7
What is the precise difference between the delay using the swipe navigation and keyboard navigation? Is there a way we can test this with any rigor?
(In reply to Asa Dotzler [:asa] from comment #107)
> What is the precise difference between the delay using the swipe navigation
> and keyboard navigation? Is there a way we can test this with any rigor?

Hitting Cmd+[ and Cmd+] is absolutely *instant*.  I can't perceive any delay whatsoever from when the keydown registers and when the browser actually changes the back/forward button states.  The swipe gesture is hard to measure.  Without having any way of triggering the event automatically (to my knowledge), I resorted to stopwatch testing.  Out of eleven results, I discarded one crazy outlier and considered only the remaining ten.  Of those ten, the mean was 0.77 seconds, the median was 0.8 seconds.  So, quite (un)comfortably above the magic 200ms threshold of user recognition.
Daniel, Cheba has opened bug 678834 to deal with the issue mentioned in comment #104.  If this is what you're discussing, the discussion should really be happening at bug 678834.

(Sorry that I forgot to mention bug 678834 in a comment earlier.)
(In reply to Steven Michaud from comment #109)
> Daniel, Cheba has opened bug 678834 to deal with the issue mentioned in
> comment #104.  If this is what you're discussing, the discussion should
> really be happening at bug 678834.
> 
> (Sorry that I forgot to mention bug 678834 in a comment earlier.)

These are different issues.  This is the actual back/forward action, not the tab bar scrolling.
Then perhaps you should open another bug on that subject.  But first carefully compare FF's behavior with Safari.

I've also noticed the "delay" (and so perhaps have others).  But I don't consider it annoying ... and in any case it'll be difficult to do anything about it.
In my experience, the "delay" also happens in Safari, but is less noticeable because of Safari's animations.

Supplying the animations is bug 678392.  And no, that's not going to happen soon.
The delay is exactly the same in Safari (measurably).  However, Safari *responds* immediately.  That's really the important point.  Since Firefox doesn't have a back forward animation, there's nothing that happens in that lull.  In my opinion, we need something, *anything*, to notify the user that something is happening.  770ms of inaction from the UI just seems unacceptable.  Imagine if clicking the back button had a 770ms latency!

I can open a separate bug if you want, but this seems like a UX issue with the work done on *this* bug, rather than a separate issue.
Please open a new bug.  It concerns a side effect of my fix for this bug, so it *is* a separate issue.
Agree with Daniel, something needs to happen immediately, like in Safari.  I'm not even sure you can call the animation in Safari a delay - it's obviously beginning doing something as soon as the gesture occurs, it could be loading the page while playing the animation so that it's ready when the animation is finished.
Please open new bug, and discuss this there.
(In reply to Steven Michaud from comment #116)
> Please open new bug, and discuss this there.

Done in bug 678891.
Quick duplication of my comment on that new bug (for people who haven't subscribed to it):

"I think maybe this should block 668953 - going live with the gesture so laggy could be worse than not implementing it at all, from a user experience point of view.  What do people think?"
(In reply to Steven Michaud from comment #112)
> And no, that's not going to happen soon.

Why? It seems obvious that the animation is critical to the UX of the feature, not only to indicate that something is occurring during the swipe but also to allow the user to cancel the gesture if they invoked it automatically.

The work here is good but it seems nuts to ship this without any sort of animation.
(In reply to comment #119)

Colin, you've worked in the Mozilla tree.  Do you have any
suggestions?  If we're going to do anything quickly, support for it
already needs to exist in the tree.
Oh, and though I've just violated my own rule, discussion of bug 678891 should really take place there.
Tried out the try b(In reply to Steven Michaud from comment #83)
> Here's a tryserver build made with my rev1 patch (from comment #77):
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> 39738b8a2f28/try-macosx64/firefox-8.0a1.en-US.mac.dmg
> 
> Please try it, everyone, and let us know your results.
> 
> If nobody finds problems, I'll land my rev1 patch on aurora tomorrow.

I tried it, but it sliently closes, without producing any error report. It appears that the new patch is not yet available in Aurora. When will it be available?
> I tried it, but it sliently closes, without producing any error
> report.

This is	probably bug 678607.

> It appears that the new patch is not yet available in Aurora. When
> will it be available?

The patch *is* available in aurora nightlies.  Try
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/.
(In reply to Steven Michaud from comment #123)
> > I tried it, but it sliently closes, without producing any error
> > report.
> 
> This is	probably bug 678607.
> 
> > It appears that the new patch is not yet available in Aurora. When
> > will it be available?
> 
> The patch *is* available in aurora nightlies.  Try
> ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/.

My Aurora is up to date (08/15/2011), but the two-finger swipe thing does not work. Are you sure? The dog file in the link you provided was last modified today.
(In reply to Steven Michaud from comment #120)
> (In reply to comment #119)
> 
> Colin, you've worked in the Mozilla tree.  Do you have any
> suggestions?  If we're going to do anything quickly, support for it
> already needs to exist in the tree.

I have not worked on any Mozilla-related code in three and a half years -- whatever knowledge I do have left is probably pretty out of date by this point. If I have any ideas though, I'll bring them up over on bug 678891.
Depends on: 678392
Comment on attachment 551102 [details] [diff] [review]
patch v2 (review comments)

As discussed with Steven Michaud we are going to resurrect this patch because we want to take a fix for the issue. In the future we will want to back out these changes and take an approach similar to attachment 552411 [details] [diff] [review] but unfortunately we can not use that patch until bug 678607 is resolved.
Attachment #551102 - Attachment is obsolete: false
Attachment #551102 - Flags: review?(Olli.Pettay)
I have this issue as well with OS X Lion and now I am stuck with Safari 5.1. And the fullscreen doesn't work at all. The pinch to zoom doesn't work and the mouse gestures don't work as well. Please Mozilla fix this as every one is having the same problem,
(In reply to Benoit Girard (:BenWa) from comment #126)
> Comment on attachment 551102 [details] [diff] [review]
> patch v2 (review comments)
> 
> As discussed with Steven Michaud we are going to resurrect this patch
> because we want to take a fix for the issue. In the future we will want to
> back out these changes and take an approach similar to attachment 552411 [details] [diff] [review]
> [diff] [details] [review] but unfortunately we can not use that patch until
> bug 678607 is resolved.

Hmm, I don't like to break tilt wheel behavior (or mighty mouse's horizontal scrolling) by this patch. This patch has the regression. Such patch shouldn't be applied on branch.
Apparently it was decided at today's aurora meeting to back my patch
out from the branches.

I strongly agree with this.  There've been a number of problems, and I
was never really comfortable with accelerating the development cycle
for a patch that does so many new things.

As comment #80 says, it uses the brand-new API [NSEvent
trackSwipeEventWithOptions:...].  And as I forgot to mention in
comment #80, it also introduces "blocks" to the Mozilla tree.  Both
have been the source of considerable trouble.

However, I'd like to keep my patch on the trunk for a while, even as
Benoit starts working again on his patch.  I may have a fix for bug
678607.  And it may be possible to shorten the delay reported at bug
678891.

The other dependent bug (bug 678834) may be harder to fix, as will
(probably) be several similar bugs that have been reported, but don't
yet have their own bug numbers.

At some point we'll have to decide which patch we want on the trunk
now -- my patch (with the addition of some fixes) or Benoit's
(probably also with the addition of some fixes).  But I don't think it
hurts to develop the two patches in parallel, for a while.
Attachment #553653 - Flags: approval-mozilla-aurora?
Attachment #553654 - Flags: approval-mozilla-beta?
Attachment #553654 - Flags: review?(bgirard)
Attachment #553653 - Flags: review+
Attachment #553654 - Flags: review?(bgirard) → review+
Comment on attachment 553654 [details] [diff] [review]
Back out my patch from beta

Ok'ing for approval with backout discussed in triage on Tuesday.
Attachment #553654 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 553653 [details] [diff] [review]
Back out my patch from aurora

Ok'ing for approval with backout discussed in triage on Tuesday.
Attachment #553653 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 553654 [details] [diff] [review]
Back out my patch from beta

Landed on mozilla-beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/1baae8b3cd70

But should I also back this out on GECKO70_2011081615_RELBRANCH?
> Landed on mozilla-beta:
> http://hg.mozilla.org/releases/mozilla-beta/rev/1baae8b3cd70
>
> But should I also back this out on GECKO70_2011081615_RELBRANCH?

Actually there are several relbranches I'd need to back my patch out from.  It's probably better for people to just create new relbranches.
My patch is now gone from the aurora and beta branches.

But two significant fixes just landed on the trunk (for bug 678607 and bug 678891), and are now available in mozilla-central (trunk) nightlies, starting with today's:

ftp://ftp.mozilla.org/pub/firefox/nightly/2011-08-19-03-07-49-mozilla-central/firefox-9.0a1.en-US.mac-shark.dmg

Please try them out, and let us know what you think.
Just tried it out and it seems to work well! No crashing or other weirdness.
Scrolling back and forth in the browser chrome (namely the tab bar) makes the active tab go back & forth in the history stack.
> Scrolling back and forth in the browser chrome (namely the tab bar) makes the
> active tab go back & forth in the history stack.

Is this bug 678834?
(In reply to Steven Michaud from comment #140)
> > Scrolling back and forth in the browser chrome (namely the tab bar) makes the
> > active tab go back & forth in the history stack.
> 
> Is this bug 678834?

Yes but with the added behaviour from the fix in this bug.


Steps to reproduce:

Scroll horizontally in the tab bar.

Actual results:

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


Expected results:

The history-stack on the active tab should not be altered when doing horizontal swipe gestures in the tab bar.
(In reply to comment #141)

Please comment about this at bug 678834.
No longer blocks: 676169
Comment on attachment 551102 [details] [diff] [review]
patch v2 (review comments)

I assume my review for this patch isn't needed.
Attachment #551102 - Flags: review?(Olli.Pettay)
> I assume my review for this patch isn't needed.

For now it isn't.

But something could still knock my patch out, in which case we'd have to fall back to Benoit's patch.
Target Milestone: mozilla7 → mozilla8
qa?: I'm a bit confused by RESOLVED FIXED but status:WONTFIX for Firefox 7. Does that mean it is fixed for Firefox 8 and will need some verification there?
Whiteboard: [qa?]
Anthony, yes.
Adding qa+ for verification on Firefox 8. Thanks Boris.
Whiteboard: [qa?] → [qa+]
Got here through DUPLICATE tag on Bug 676169.

Just wanted to point out Magic Mouse back-fwd one/two-finger gestures won't work on any of the channels (release/beta/alpha, as of today).
Also, Trackpad back-fwd only works with 3-button option; 2-button doesn't work. (FF 8.0 beta channel, Lion 10.7.1).
(In reply to Artur Adib from comment #149)
> Got here through DUPLICATE tag on Bug 676169.
> 
> Just wanted to point out Magic Mouse back-fwd one/two-finger gestures won't
> work on any of the channels (release/beta/alpha, as of today).

Just tested with the latest aurora build.  Works fine with both the Magic Mouse and the trackpad.
(In reply to Artur Adib from comment #149)
> Got here through DUPLICATE tag on Bug 676169.
> 
> Just wanted to point out Magic Mouse back-fwd one/two-finger gestures won't
> work on any of the channels (release/beta/alpha, as of today).

WFM in both Aurora and Nightly using one-finger swipes. (won't work on beta, as it didn't make it into 8.0.)
Make sure you enabled the "Swipe between pages (Scroll left or right with one finger)" gesture under System Preferences -> Mouse -> More Gestures.

The only missing thing as far as I can tell is the animation.
Daniel, Emanuele: OK my bad, tried current stable, beta; aurora was still 8.x :)

Sorry for the false alarm.

PS: Yeah, animation/visual cue would be very welcome! :)
(In reply to Emanuele Alimonda from comment #152)
> Make sure you enabled the "Swipe between pages (Scroll left or right with
> one finger)" gesture under System Preferences -> Mouse -> More Gestures.

Good, now I know how to disable this.  This has caused me more dataloss in forms in the last week because I try to scroll left/right within a text field and it goes back instead of scrolling in the field.
Dave, please post a testcase of your problem, plus STR.

What happens with the same testcase in Safari?
>What happens with the same testcase in Safari?

safari provides visual feedback of the gesture.
Safari provides not just visual feedback, but the opportunity to abort the gesture if it's not what you wanted to do. This feature shouldn't be released without that. A surprise back action resulting from a horizontal scroll gesture is a misfeature.

In Safari 5.1, if you're about to trigger the back gesture, the current page begins to slide away, revealing the previous page underneath it. It's very obvious what's about to happen. If you decide you don't want to go back, you can scroll back the other direction and the current page slides back into place.

Without that visual, interactive feedback mechanism, this feature should never be released to the public.
>Without that visual, interactive feedback mechanism, this feature should never be >released to the public.

I agree, Asa: I recommend that we pull this out of beta before final release.  The current behavior is often surprising.
Chrome gives feedback with an arrow moving on the screen before going on the previous/next history item.
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #158)
> >Without that visual, interactive feedback mechanism, this feature should never be >released to the public.
> 
> I agree, Asa: I recommend that we pull this out of beta before final
> release.  The current behavior is often surprising.

I thought we had the animations too. I played with a build that was pretty good and contained the transitions. Did that not make it? When is that scheduled?
Once again, guys, we really need a testcase and STR.
> Once again, guys, we really need a testcase and STR.

We should *not* do anything here until we fully understand what we're doing, and what the problem is we're trying to fix.

You don't need to post a reduced testcase (though that would be nice).  But we do need at least one (or more) example URLs, with STR.
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #158)
> >Without that visual, interactive feedback mechanism, this feature should never be >released to the public.

How would you handle the fact that the current behavior conflicts with a 10.7 shortcut though and was one of the top support issues with 10.7?  Without-animations is a stop gap for sure, but at least it unbreaks people.
(In reply to Asa Dotzler [:asa] from comment #160)
> I thought we had the animations too. I played with a build that was pretty
> good and contained the transitions. Did that not make it? When is that
> scheduled?

That's bug 678392; I haven't had the time to finish it yet due to uni exams.
I'll upload the unfinished patches tomorrow.
(In reply to comment #154)

> This has caused me more dataloss in forms in the last week because I
> try to scroll left/right within a text field and it goes back
> instead of scrolling in the field.

Here's a simple form I've been testing with (using today's
mozilla-central nightly on OS X 10.7.2).  Using it, I'm unable to
reproduce the problem you describe.  Are you able to do so?  If so,
please tell us how.

Once again, if you have	some other page on which you're able to
reproduce this problem, please paste in the URL.

Also, what version of FF have you been using?  And what version of OS
X 10.7?  (The current version is 10.7.2, but that only came out
recently.)
(Following up comment #165)

One more thing:

Can you also reproduce your problem using the Back and Forward
buttons?
STR with your form:

1. Type enough text in Box 1 to make it scrollable left to right, and require you to scroll multiple times to get from one end to the other.
2. Use two-finger swipes on the trackpad to scroll the text to the left to get back to the beginning. Because you typed enough text that you have to scroll more than once, you typically keep scrolling until it stops moving when you try to scroll.

Expected results:
- When you get to the end it stops.

Actual results:
- When you get to the end, you perform a Back action and go to the previous page.

Perhaps what's actually needed here is a longer backoff time before switching context from the text box to the overall page if a scroll has recently been performed inside the text box.

FWIW, the exact same thing happens on the page itself if you have a page that requires horizontal scrolling and you accidentally swipe-to-scroll again after you hit the left edge of the page.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0a2) Gecko/20111016 Firefox/9.0a2

OS X 10.7.2
(In reply to Dave Miller [:justdave] from comment #167)
> Perhaps what's actually needed here is a longer backoff time before
> switching context from the text box to the overall page if a scroll has
> recently been performed inside the text box.

^^ the code that deals with deciding whether to scroll the contents of a listbox or iframe or the contents of the entire page might be a good place to look for examples.
FWIW, the actual "dataloss" aspect is bugs in the websites in question, not in Firefox (they're sites that dynamically regenerate stuff on the page with javascript on page load and so forth, and don't properly watch for cache data).  I usually know to be careful on said sites, but the Back action happening when I'm not expecting it to throws a wrench into my "being careful".
(In reply to Dave Miller [:justdave] from comment #167)
> Perhaps what's actually needed here is a longer backoff time before
> switching context from the text box to the overall page if a scroll has
> recently been performed inside the text box.

And that backoff time should apply whether or not the mouse is still inside said text box.  If I accidentally shift the mouse outside the box while I'm in the middle of scrolling it, a sudden Back action would be just as annoying.
(In reply to comment #167 through comment #171)

Thanks, Dave, for the information.

Using your STR from comment #167, I can now reproduce the problem you
report.  I can't (of course) reproduce the data loss, but you've
plausibly described how it would happen on some web sites.  (It would
also happen with the Back and Forward buttons, but it's much easier to
use the horizontal swipe accidentally.)

It's hard to know what we can do about it.

I don't think a "backoff time" (a delay before another swipe is
allowed) is a good solution.  Benoit's patch had one, and I (at least)
found it very annoying -- swiping back or forward more than one page
was very awkward.

So I tend to agree that we need some kind of visual clue that a swipe
is in progress, so that the user can cancel the swipe if it's
unexpected.  (Though I *don't* think the case has yet been made for
dropping support for two-finger horizontal swipes if we	don't provide
such a visual clue.)

With luck, Markus Stange's patches (for bug 678392) are close enough
to being finished that we can use them.  But those patches are
complex, and neither Markus nor I have much time to devote to them.
It may be a while before they're polished enough to include in a
release.

In comment #159, Anthony Ricaud points out another possibility:
Chrome (actually still only Chrome Canary) uses a semi-transparent
arrow, traveling across the browser window, to indicate a swipe in
progress.  I don't know how easy or difficult this would be for us to
implement (specifically, I don't know if support already exists in
cross-platform code for doing something like this).

Markus (or anyone else who knows about these things):  Do you think
it's worthwhile for us to try implementing Chrome's travelling arrow,
if it looks like your animation patch can't be finished in time?
My problem with using an interim solution is that I'm quite sure that a share of people will want to keep it when the real solution is ready to land. Possible flame wars might be the consequence and people wanting the old solution back "or at least an option". We are talking about nightly builds where the behavior is suboptimal at the moment, so I'd say that we wait until the real thing is ready. Just my opinion.
Remember that support for the two-finger horizontal swipe is just about the top support request on Lion.  This has to be factored into our calculations.

But it's true that people might complain if, say, we implemented the traveling arrow and then later replaced it with Safari-style swipe animation.
I've found the Chrome code (in the Chromium source tarball) that
implements the traveling arrow.  It's all Cocoa and CoreAnimation
code, so it looks like it'd be pretty easy for us just to copy it.
The traveling arrow is a small native window (an NSWindow object).
The is code mostly in three files --
chrome_render_widget_host_view_mac_delegate.mm,
history_overlay_controller.h and history_overlay_controller.mm.

It's probably worth while for me to spend a day or two implementing
this in the Mozilla tree, so that's what I'll do.

I'm *not* saying this is what we should do.  I just want to make it
available as a possible alternative.

Markus, let me know if you don't think this is necessary (if you don't
think I should bother).
(In reply to Steven Michaud from comment #172)
> I don't think a "backoff time" (a delay before another swipe is
> allowed) is a good solution.  Benoit's patch had one, and I (at least)
> found it very annoying -- swiping back or forward more than one page
> was very awkward.

That's not quite what I'm asking for and I'm sure that would be annoying.  The backoff should only be if the previous swipe wasn't for a page back action.  If the previous swipe was interpreted as a page back action, then it's quite logical to assume that the next one would be for the same thing and do it without any backoff time.  The backoff time would only happen if the previous swipe did something ELSE. (like scrolling in a text box or horizontally scrolling the window itself)
There already appears to be a backoff time exactly like what I'm suggesting going between scrolling in a text box and scrolling the page, fwiw.

If you have a text box with enough data to scroll on a page that's wide enough to horizontally scroll, but is not currently scrolled all the way to the left...

1) if the text box is already scrolled all the way to the left and I two-finger-swipe over it, the PAGE scrolls.
2) if the text box is not scrolled all the way to the left and I two-finger-swipe over it, the text box scrolls.  If I keep swiping, it scrolls to the end and then stops, the page doesn't scroll at all, even if I keep swiping.  This puts us back to option #1 above, but there's a timer that has to reset before it'll treat it as #1.  If I pause long enough for that timer, then it'll behave like #1 again and scroll the page.

Extending it to apply exactly the same way to the page swipe would work great.  If we're already leftmost in both the text box and the page, then page back.  If we're not, scroll whichever one has the most specific context, then pause for a backoff time before allowing that context to change.
(In reply to comment #176)

> The backoff time would only happen if the previous swipe did
> something ELSE. (like scrolling in a text box or horizontally
> scrolling the window itself)

I don't think this would be too hard to implement.  I'm	still not sure
it's a good idea, but it's worth trying out.  After I've finished with
my test implementation of the traveling arrow, I'll do another test
implementation of this.
(In reply to comment #177)

> 2) if the text box is not scrolled all the way to the left and I
> two-finger-swipe over it, the text box scrolls.  If I keep swiping,
> it scrolls to the end and then stops, the page doesn't scroll at
> all, even if I keep swiping.  This puts us back to option #1 above,
> but there's a timer that has to reset before it'll treat it as #1.
> If I pause long enough for that timer, then it'll behave like #1
> again and scroll the page.

There does seem to be a timer here (though I haven't yet found where
it is in the Mozilla tree).  But I'm not sure I like it -- it can have
unfortunate effects.

For example:

a) Put enough text in a text box to make it scrollable (say the URL
   box at the top of this bug's page).

b) Arrange the page so that it has a horizontal scrollbar, and isn't
   scrolled all the way to the left, but the URL box is	still at least
   partly visible.

c) Hover the mouse over the URL box, and scroll it at least partly to
   the right.

d) With the mouse still hovered over the URL box, use a	two-finger
   swipe to scroll it all the way to the left.

e) Then, before the timer has expired (and with the mouse still over
   the URL box), do another two-finger swipe to the left.

   The page should scroll to the left, and would if you'd waited long
   enough for the timer to expire.  But instead you swipe back to the
   previous page in the history list.
(Following up comment #179)

By the way, in current code a single two-finger horizontal gesture will never be both a scroll and a swipe.  That's why, if you swipe to scroll the page to one side and continue your swipe, you'll never swipe to another page.  This was deliberate, and was an attempt to make it more difficult to accidentally swipe to another page.

It's the OS that determines what a "single two-finger horizontal gesture" is (in the information it passes to the trackingHandler passed to [NSEvent trackSwipeEventWithOptions:...]).

I suspect this behavior may make the timer you found unnecessary.

The timer in my test build will behave differently.  It will do *exactly* the following:

> The backoff time would only happen if the previous swipe did
> something ELSE. (like scrolling in a text box or horizontally
> scrolling the window itself)
The current Firefox 8.0b4 beta release notes say that this bug has been fixed:

http://www.mozilla.org/en-US/firefox/8.0/releasenotes/buglist.html

Isn't that a mistake? From what I understand of this discussion, it has been withdrawn from the current beta, and from what I see, it is only fixed in the 9.0a2 aurora.
Currently the patch is still on the beta tree (and in FF 8.0b4), and this bug is still fixed.
I don't understand how to make it work, then. I have tried setting "Swipe between pages" to either "Scroll left or right with two fingers" or "Swipe with two or three fingers". Since neither worked, I suspected that some extension might have interfered, so I quit the beta, removed both ~/Library/Application\ Support/Firefox and ~/Library/Preferences/org.mozilla*, and started it again. There must be something I have overlooked (I am known to miss the obvious), because it still would not work.

It worked fine in 9.0a2, though.
j_mach_wust:

Turns out you're right and I'm wrong.  My fix for this bug *isn't* in
FF 8 b4 (or b3 or b1), or on the beta branch.  It *is* on the Aurora
(alpha) branch -- what will become FF 9.

I don't know how this happened -- I think we all just got collectively
confused about which version of FF this bug was going to be fixed in.

Thanks for pointing out the problem!

So either our release notes need to be changed, or this patch needs to
be landed right away on the beta branch.  I vote for just changing the
release notes.

If we just change the release notes, we'll also need to change the
"target milestone" from mozilla8 to mozilla9.
Target Milestone: mozilla8 → mozilla9
When my patch for this bug got backed out of the aurora and beta
branches (in comment #129 and comment #130), it	(in effect) got backed
out from FF7 *and FF8*.  I didn't realize this at the time, or I'd
have changed the target milestone to "mozilla9".

Then the target milestone got changed a few more times (though it
ended up set to "mozilla8").  And (presumably) seeing that, people set
"status-firefox8" to "fixed".

Since comment #129 and comment #130 my patch (which remained on
mozilla-central) has been making its way into release versions "by
normal channels" -- by being merged from mozilla-central to aurora,
and then from aurora to beta.  But the merge to beta hasn't happened
yet, and wasn't intended to.

Yes, in	principle our "new" (as	of FF 6) release system	is very
simple.  But if even the experts can get so confused, I think there's
something wrong with how we keep track of target release versions :-)
> The current Firefox 8.0b4 beta release notes say that this bug has been fixed:
>
> http://www.mozilla.org/en-US/firefox/8.0/releasenotes/buglist.html

clegnitto has told me he'll fix this.
Depends on: 698761
(Following up comment #175)

It took	me longer than I'd hoped (I was distracted by other bugs), but
I've now got a patch that implements something very close to
Chromium's "traveling arrow".  See bug 698761.

There's	also a tryserver build.	 Please	try it out and comment at bug
698761.
(In reply to Steven Michaud from comment #185)
> When my patch for this bug got backed out of the aurora and beta
> branches (in comment #129 and comment #130), it	(in effect) got backed
> out from FF7 *and FF8*.  I didn't realize this at the time, or I'd
> have changed the target milestone to "mozilla9".
> 
> Then the target milestone got changed a few more times (though it
> ended up set to "mozilla8").  And (presumably) seeing that, people set
> "status-firefox8" to "fixed".
> 
> Since comment #129 and comment #130 my patch (which remained on
> mozilla-central) has been making its way into release versions "by
> normal channels" -- by being merged from mozilla-central to aurora,
> and then from aurora to beta.  But the merge to beta hasn't happened
> yet, and wasn't intended to.
> 
> Yes, in	principle our "new" (as	of FF 6) release system	is very
> simple.  But if even the experts can get so confused, I think there's
> something wrong with how we keep track of target release versions :-)

To clarify, does this mean it's in Fx9 and on, but still backed out for Fx7 and Fx8?  

According to the bug, it says 'status-firefox9: fixed'.
> To clarify, does this mean it's in Fx9 and on, but still backed out
> for Fx7 and Fx8?

Yes.

This bug's patch is in current Aurora nightlies, and will (in the
normal course of events) appear in FF 9.

But it's not on	the beta branch	-- which means in won't appear in FF
8.

And of course it's not in FF 7.
Verified on :
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20100101 Firefox/9.0 beta 3

I have run the testcase attached and I can scroll horizontally with one finger swipe, both scroll bars. When the first one reaches the end, the second one begins scrolling (left or right).

Two-fingers swipe goes back one page or forward.

It's enough to set this bug as verified?

Thanks
(In reply to Vlad [QA] from comment #190) 
> It's enough to set this bug as verified?

I believe so.
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [qa+] → [qa!]
Keywords: verified-aurora
Is it possible that this only works with 64 bits? I had my Nightly accidentally set to 32 bits only and spent over a day looking for the reason why swiping didn't work. Unchecking "Open in 32 Bit Mode" resolved the issue.
(In reply to Alexander from comment #192)
> Is it possible that this only works with 64 bits? I had my Nightly
> accidentally set to 32 bits only and spent over a day looking for the reason
> why swiping didn't work. Unchecking "Open in 32 Bit Mode" resolved the issue.

Confirmed.  Swiping doesn't seem to work for me either in 32bit mode, while in 64bit mode it works properly.
I only tested it on OS X 10.7.2.
Could you guys open a bug and mark it a blocking this one?
Depends on: 714605
> Is it possible that this only works with 64 bits?

Yes, and it's intentional.  See comment #67.

We won't be able to support the two-finger horizontal swipe on Lion in 32-bit mode until we drop support for OS X Leopard (10.5).
(Following up comment #195)

I suppose we should have relnoted this.
Keywords: relnote
Works fine for me in 10.7.2 using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0) Gecko/20100101 Firefox/10.0 in 64bit mode.

A nice to have feature would be some animations around going forward and back, similiar to the way Safari and Chrome show it.
Glenn, if you can't be bothered to read anything but the last three or four comments in a bug, please don't bother commenting.
(In reply to Chris Lawson from comment #198)
> Glenn, if you can't be bothered to read anything but the last three or four
> comments in a bug, please don't bother commenting.

Yes Chris because your comment is very useful indeed!
Thanks for confirming that the feature works, Glenn. The swipe animation is being dealt with in bug 678392, so you can follow along there.
No problem Josh. Thanks for that...I found the bug up above...missed it the first time round :)
I have it not working with firefox 10 and 9 under mac os x Lion. This is what happens and it really is strange: 
1) I download the .dmg file from the internet
2) I open the .dmg file and run ffox from it
3) The 2 finger swipe back works!
4) I install it under the /Application folder
5) The gesture is not working anymore
6) Creating a subfolder and placing ffox inside makes it work again

Have a look at here https://support.mozilla.org/it/questions/906145#answer-290292 other users are reporting this
Depends on: 718027
(In reply to Colin Barrett [:cbarrett] from comment #119)
> (In reply to Steven Michaud from comment #112)
> > And no, that's not going to happen soon.
> 
> Why? It seems obvious that the animation is critical to the UX of the
> feature, not only to indicate that something is occurring during the swipe
> but also to allow the user to cancel the gesture

Some friendly feedback from a frustrated user ...

Colin was right: it was nuts to release this feature without the animation.

I often find myself accidentally navigating forwards and backwards while scrolling up and down.  It's all the more frustrating because it's not immediately obvious which has occurred.

I understand that you've got bug 678392 to track the animation work, but please try to resist the urge to release half-baked features in the future, especially when there's no real urgency.

The absence of this feature was much less annoying than the broken implementation, which I'm sure has hit thousands of people who never had any desire to use the horizontal swipe.

Is there a Firefox preference I can use to disable the feature for now?
(In reply to Mark Goodhand from comment #203)
> Is there a Firefox preference I can use to disable the feature for now?

I just had a quick look in about:config and found what I was looking for:

browser.gesture.swipe.left
browser.gesture.swipe.right

Blanking those out seems to have done the trick.
Depends on: history-swipe
Depends on: 1267445
Depends on: 1382560
No longer depends on: 1382560
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: