698761 - [10.7] Add support for Chrome-style swipe animation

Status: RESOLVED DUPLICATE of bug 678392

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug is spun off from bug 668953 (particularly from bug 668953
comment #172).  We need visual cues for two-finger horizontal swipes.
Otherwise it's too easy to perform them accidentally, which can lead
to loss of data in a form which has been filled in but not yet
submitted.

Markus Stange has been working on implementing Safari-style swipe
animation at bug 678392.  But the work is complex, and hasn't yet been
finished.

Chrome Canary implements a different kind of animation for two-finger
horizontal swipes -- an overlay that follows the swipe until it's
finished.  This is much easier to implement, and so should be much
easier to bring to a fully polished state.

For now at least, I'm not saying that this is what we should do.  But
I've already created an implementation that I think is close to
release quality.  It can serve as a backup if we're not able to finish
the work on bug 678392 in time.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: v1 patch

Here's a patch that (basically) just copies the code that implements
Chromium's "traveling arrow".  Though I did change the arrows to left
and right pointing triangles, because they more closely resemble the
symbols we already use for the Back and Forward buttons.

Here's a tryserver build made with my patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-53a8a65f42d2/try-macosx64/firefox-10.0a1.en-US.mac.dmg

Please try it out!  And	comment	here about any problems	you find.

I already know my "traveling triangles" are kind of ugly.  For one
thing they're different sizes.  So it's probably better to use
graphics instead of Unicode font characters -- ideally the same
graphics we use for the Back and Forward buttons.  I'd appreciate
someone's help locating those graphics, and advice on how to use them
(for example on how to lay them out, and to size the window that
contains them).

Comment 2

[Simo Kivimäki]

(In reply to Steven Michaud from comment #1)
> Here's a tryserver build made with my patch:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 53a8a65f42d2/try-macosx64/firefox-10.0a1.en-US.mac.dmg
> 
> Please try it out!  And	comment	here about any problems	you find.

Looks nice! But it crashes when swipe back and forth is done fast without waiting page to load:

https://crash-stats.mozilla.com/report/index/bp-23fd0f5c-a067-47fc-aac8-959ae2111101

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> Looks nice! But it crashes when swipe back and forth is done fast
> without waiting page to load:

Please give URLs of pages you crash on, when swiping back and forth
between them.

Do you also crash without my patch (on, say, today's mozilla-central
nightly)?

I'll work on translating the symbols in your Breakpad crash stack.

Comment 4

[Simo Kivimäki]

I could not get mozilla-central's nightly build to crash but your build crashes easily. And your build does not crash when cmd+left/right arrow navigation is used.

I tried couple of sites, at least pages with rich content does crash. E.g:

1. Open http://www.theonion.com/
2. Click “Glenn Beck Appears In Revealing Documentary About Brooke Alvarez's Childhood As Russian Cosmonaut”
3. Swipe back and immediately after that to forward.
4. Continue until Firefox crashes, usually few times is enough for me.

Comment 5

[beingalink]

I don't really like this solution even apart from the current design of the arrows. But it's definitely much better than the current behavior. So I hope that this solution will not be needed for too long and the real animation can be landed rather sooner than later. Of course I know that it's not trivial, so I'll try to keep my hopes modest. :)

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: bp-23fd0f5c-a067-47fc-aac8-959ae2111101 symbols

Here are the symbol translations for Simo's Breakpad crash stack.  I don't fully trust it -- parts of it are truly weird.  But the top part puts the blame pretty squarely on my patch, at least as the crashes' trigger.

But the true cause may turn out to be an Apple bug -- possibly a bug in Apple's block support code.

I used atos -o [modulename] -arch x86_64 [address] to do the translations.

I'll work on reproducing the crashes myself, and getting a gdb crash stack, which is likely to be much more reliable.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb stack trace of crash

I've managed to get a gdb stack trace of the crash, following Simo's STR from comment #4.

The top part is the same as the bp stack, but the rest of it makes a *lot* more sense.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb stack trace of crash

Oops.

Comment 9

[Chris Lee [:clee]]

Using the tryserver build, the arrows appear to be the opposite direction of my swipe.  I'm using a magic mouse on 10.7.2.  Is this intentional?  

This is a good stop gap solution to the full animation that is being worked on right now.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> Using the tryserver build, the arrows appear to be the opposite
> direction of my swipe.  I'm using a magic mouse on 10.7.2.  Is this
> intentional?

No ... or at least I don't think so.

Do the arrows move in the "normal" direction when you do a two-finger
swipe on the trackpad?

Does the same reversal of direction happen with Google Canary?

I really need to get a magic mouse to test with ...

Comment 11

[Chris Lee [:clee]]

Testing out the build, Firefox just crashed.  

Here's the link to the crash report -- https://crash-stats.mozilla.com/report/index/bp-2a008ccf-356e-41e5-b8bc-f8d0a2111101

On the trackpad, I see the same behavior with the arrows that I see on the magic mouse.  However, looking at the Canary build on Chrome, it's the same behavior.  

Thinking about this a bit more, the arrow animation is less intuitive vs. the full page animation shown in Safari.  I retract my previous statement in comment 9 and the animation looks correct (even though it's still the opposite direction of my swipe).

Comment 12

[Robert Strong (they/them - no direct email)]

iirc the arrow represents the direction of the page and not the direction your hand moves. On Windows at least, it is more intuitive with up / down swipes where the page is moving down towards the bottom of the page with the arrow pointing down with the swipe being upward.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v2 (fix crashes)

Here's a patch that fixes the crashes.  Turns out that Apple's tracking handler for -[NSEvent trackSwipeEventWithOptions:...] has unexpected behavior -- it can be called more than once with isComplete == true.

A tryserver build should be available in a few hours.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #12)

Does Windows have swipe tracking animation?  Is it only Windows 7?

I currently can't run Windows 7 (or Vista).  But at some point I'll
spend the time needed to install it into a VM.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #13:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ad8a4e2a4a67/try-macosx-debug/firefox-10.0a1.en-US.mac.dmg

Please try it out.  The crashes should be fixed.

Comment 16

[Chris Lee [:clee]]

Just tried this build and swipe is broken for me.  Since back swipe doesn't work, there is no animation.  

Am I missing something?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Oops, I pasted in the wrong link.  Here's the correct one:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ad8a4e2a4a67/try-macosx64/firefox-10.0a1.en-US.mac.dmg

Sorry for the confusion.

Comment 18

[Chris Lee [:clee]]

Works now with this build.  So far it seems to work well and the back/forward swipes and snappy and the animation helps with the overall browsing experience.  

I'll let you know if I run into any issues with this new build.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I just got a different crash with the new build:

bp-32660649-4549-4c37-bb7a-d4f9e2111102

The top two lines translate to:

___trackSwipeWithScrollEvent_block_invoke_1 (in AppKit) + 2965
__-[ChildView maybeTrackScrollEventAsSwipe:scrollOverflow:]_block_invoke_1 (in XUL) + 410

So my work isn't yet done :-(

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Oops, got the top two lines reversed.  They should be:

__-[ChildView maybeTrackScrollEventAsSwipe:scrollOverflow:]_block_invoke_1
  (in XUL) + 410
___trackSwipeWithScrollEvent_block_invoke_1 (in AppKit) + 2965

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb stack trace of 2nd crash

I'll be working on this.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> I'll be working on this.

The problem was very simple, and I've fixed it -- we need to null check mGeckoChild in -[NSEvent trackSwipeEventWithOptions:...]'s tracking handler.

The bug is in existing code (not my patch).  My patch just uncovered it.

But I can't yet start a tryserver build, because there's a terrible bug in current trunk code, and any build I did now would crash on startup (see bug 694896).  So I'll post my patch and do my tryserver build tomorrow (by which time the jemalloc bug should have been fixed).

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v3 (fix 2nd crash)

Here's a version of my patch that fixes the 2nd crash.

And here's a tryserver build made from it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c7d572f89623/try-macosx64/firefox-10.0a1.en-US.mac.dmg

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v3, updated to current code

A new tryserver build will follow in a few hours.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Here's the new tryserver build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b78b110ccefb/try-macosx64/firefox-11.0a1.en-US.mac.dmg

Comment 26

[thakis]

Hi Steven,

cool you're working on this. I'd recommend that you trigger the navigation on `phase == NSEventPhaseEnded` instead of on `isComplete` – the former is sent earlier once it's clear that the user will complete the gesture. Say the user throws a swipe across the window and stops after 50%, then the swipe will complete, and you'll get phase == NSEventPhaseEnded as soon as the user lets go, but the value will continue to animate to 1.0 and only then send isComplete.

Nico

Comment 28

[Robert Strong (they/them - no direct email)]

Stephen Pohl is almost finished with bug 678392 so I am going to dupe this to that bug.