Closed Bug 698761 Opened 9 years ago Closed 8 years ago

[10.7] Add support for Chrome-style swipe animation

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 678392

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

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.
Attached patch v1 patch (obsolete) — Splinter Review
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/smichaud@pobox.com-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).
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Summary: Add support for Chrome-style swipe animation → [10.7] Add support for Chrome-style swipe animation
(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/smichaud@pobox.com-
> 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
> 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.
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.
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. :)
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.
Attached file Gdb stack trace of crash (obsolete) —
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.
Oops.
Attachment #571092 - Attachment is obsolete: true
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.
> 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 ...
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).
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.
Attached patch Patch v2 (fix crashes) (obsolete) — Splinter Review
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.
Attachment #571029 - Attachment is obsolete: true
(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.
Just tried this build and swipe is broken for me.  Since back swipe doesn't work, there is no animation.  

Am I missing something?
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.
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 :-(
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
I'll be working on this.
> 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).
Attached patch Patch v3 (fix 2nd crash) (obsolete) — Splinter Review
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/smichaud@pobox.com-c7d572f89623/try-macosx64/firefox-10.0a1.en-US.mac.dmg
Attachment #571376 - Attachment is obsolete: true
A new tryserver build will follow in a few hours.
Attachment #572067 - Attachment is obsolete: true
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
Duplicate of this bug: 812914
Stephen Pohl is almost finished with bug 678392 so I am going to dupe this to that bug.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 678392
You need to log in before you can comment on or make changes to this bug.