Last Comment Bug 678891 - [10.7] Bug 668953's current two-finger swipe has ~700ms delay but no animation to hide it
: [10.7] Bug 668953's current two-finger swipe has ~700ms delay but no animatio...
Status: VERIFIED FIXED
[inbound][qa!]
: verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 6 votes (vote)
: mozilla9
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on:
Blocks: lion-compatibility
  Show dependency treegraph
 
Reported: 2011-08-14 16:33 PDT by Daniel Spiewak
Modified: 2011-11-29 08:48 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
fixed


Attachments
Quick fix -- reduce delay (1.79 KB, patch)
2011-08-17 13:07 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
Quick fix rev1 (fix problem) (1.77 KB, patch)
2011-08-17 14:15 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
Quick fix rev2 (fix another problem) (2.20 KB, patch)
2011-08-17 15:24 PDT, Steven Michaud [:smichaud] (Retired)
mstange: review+
Details | Diff | Splinter Review

Description Daniel Spiewak 2011-08-14 16:33:05 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0a2) Gecko/20110813 Firefox/7.0a2
Build ID: 20110813042013

Steps to reproduce:

Loaded a page, then loaded a subsequent page to populate the tab history.  Used the two-finger swipe to go back.


Actual results:

Firefox *did* go back, but the action was delayed about 770ms (according to multiple stopwatch timings) from the gesture to the actual browser response (as determined by the back/forward buttons changing state).  Keyboard navigation (via Cmd+[) had no observable delay.


Expected results:

Safari may be considered the reference implementation for this gesture.  Safari *does* delay the actual back action by exactly the same amount (~770ms).  However, Safari has an animation which is running between the moment of the gesture and the moment the browser UI responds to the back event.  This gives the user *immediate* feedback that the gesture was effective.  In the case of Firefox, it's simply 770ms of dead silence.

This is a rather serious UX issue with the back/forward gesture.  770ms is well above the 150-200ms threshold of user patience.  In fact, it's actually enough time that *really* impatient users (such as myself) have time to input the gesture a second time, working under the assumption that the first time didn't "take".

Ideally, Firefox would implement a back/forward animation similar to Safari's, supporting the touch metaphor implied by the back/forward gesture itself.  However, I would settle for *any* sort of feedback (spinner, anyone?) just to let the user know that something is happening.
Comment 1 Sam Illingworth 2011-08-14 16:41:07 PDT
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?
Comment 2 Daniel Spiewak 2011-08-14 17:06:16 PDT
(In reply to Sam Illingworth from comment #1)
> 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?

I agree.  Users have very low tolerance for UI response time.  This is true for anything interface-related, but *especially* touch gestures.  I would much rather have no back/forward swipe than go with what we currently have, which is a potential source of frustration.
Comment 3 Steven Michaud [:smichaud] (Retired) 2011-08-14 17:09:14 PDT
What do you think, Asa? :-)
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-08-14 17:11:49 PDT
And I'd appreciate suggestions for what kind UI effect I might use to signal that a swipe has *taken* (though it might still not succeed -- for example if it's interrupted).

This almost certainly can't be any kind of animation -- not it we want to implement it quickly.
Comment 5 Steven Michaud [:smichaud] (Retired) 2011-08-14 17:13:32 PDT
Provisionally taking this bug ... though I don't really have time for it.
Comment 6 Daniel Spiewak 2011-08-14 17:15:14 PDT
(In reply to Steven Michaud from comment #4)
> And I'd appreciate suggestions for what kind UI effect I might use to signal
> that a swipe has *taken* (though it might still not succeed -- for example
> if it's interrupted).
> 
> This almost certainly can't be any kind of animation -- not it we want to
> implement it quickly.

That's a tough one.  I have several ideas, but they generally involve animations or non-trivial transitions of some form or another.  I think the best thing to do might be to just trigger the progress spinner on the current tab.  If the action is interrupted, we cancel the spinner.  This is in line with how users associate the spinner (i.e. page is loading; please wait).  It's also a UI element which is already implemented and (hopefully) not too difficult to access.
Comment 7 Sam Illingworth 2011-08-14 17:27:16 PDT
Busy cursor, maybe?
Comment 8 Daniel Spiewak 2011-08-14 17:28:51 PDT
(In reply to Sam Illingworth from comment #7)
> Busy cursor, maybe?

*Definitely* not.  Busy cursors are modal indicators that completely take over a user's primary means of interacting with the browser.  They should only be used for absolutely catastrophic waits (such as a hung process).
Comment 9 Sam Illingworth 2011-08-14 17:31:29 PDT
Yeah, suppose you're right.

Could we make the progress bar show up immediately, on 0%?
Comment 10 Daniel Spiewak 2011-08-14 17:32:15 PDT
(In reply to Sam Illingworth from comment #9)
> Could we make the progress bar show up immediately, on 0%?

I didn't realize we even had a progress bar anymore...
Comment 11 Asa Dotzler [:asa] 2011-08-14 17:57:05 PDT
Steven, I'd like to chat with Limi about this and see what we can come up with. I'll try to follow-up Monday or Tuesday. I personally don't think this is worse than no swipe at all and so I'm not yet ready for making an improvement a condition for shipping. More in a day or two.
Comment 12 Daniel Spiewak 2011-08-14 19:30:45 PDT
I just discovered something even more serious here: if you change tabs during the delay, the back/forward event will be handled by the tab you changed to, rather than the original tab.  I actually do this sort of thing all the time, so the impact of this issue just jumped in my book.
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-08-14 19:47:07 PDT
> I just discovered something even more serious here: if you change
> tabs during the delay, the back/forward event will be handled by the
> tab you changed to, rather than the original tab.  I actually do
> this sort of thing all the time, so the impact of this issue just
> jumped in my book.

Open a new bug and make it block bug 668953 :-(

It's related to, but not the same as the problem I reported in bug
668953 comment #73 (and fixed in the final version (rev1) of my
patch).

For completeness, check to see if Safari has the same bug.
Comment 14 Anthony Ricaud (:rik) 2011-08-14 22:00:12 PDT
Could we change the previous/forward button to an active state?
Comment 15 Erunno 2011-08-15 06:54:58 PDT
(In reply to Daniel Spiewak from comment #12)
> I just discovered something even more serious here: if you change tabs
> during the delay, the back/forward event will be handled by the tab you
> changed to, rather than the original tab.  I actually do this sort of thing
> all the time, so the impact of this issue just jumped in my book.

I experience a similar problem of interaction X intended for tab A "bleeding" to tab B when switching from A to B while X is still in progress. For instance, if you swipe up or down and have "kinetic" scrolling enabled, the "momentum" of tab A will carry over to tab B if you switch from A to B while A hasn't finished scrolling yet.

Does a bug exist for this unexpected behavior?
Comment 16 Alex Faaborg [:faaborg] (Firefox UX) 2011-08-15 13:09:24 PDT
best: animation providing feedback

quick fix: blank the page, even though we have the next page in cache.  It will at least feel responsive.  We should also invoke the throbber, but doing only that isn't likely to be noticeable.
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-08-15 14:04:26 PDT
> quick fix: blank the page ...

Seems a bit startling.

Are you sure this is better than just living with the delay?

In other words, have you spent a while swiping between pages on Lion in builds that have my patch for bug 668953?
Comment 18 Markus Stange [:mstange] 2011-08-15 15:43:06 PDT
Steven, the current implementation waits for isComplete before sending the event. But isComplete really means "animation has finished". The sample code provided by Apple suggests that we change the content behind the scenes already when phase == NSEventPhaseEnded. Can you give that a try?
Comment 19 Steven Michaud [:smichaud] (Retired) 2011-08-15 16:02:35 PDT
Actually I've already thought of trying to dispatch the Gecko gesture event on NSEventPhaseEnded or NSEventPhaseCancelled.  One problem with doing that is that gestureAmount might not yet have been initialized.

But for now I'm desperately trying to fix bug 678607, and things haven't been going well.  Needless to say, if we can't fix those crashes, it will be pointless continuing to work on this bug (and related bugs).

Feel free to try this yourself, though.  And let us know your results :-)
Comment 20 Steven Michaud [:smichaud] (Retired) 2011-08-15 16:08:52 PDT
> on NSEventPhaseEnded or NSEventPhaseCancelled

Actually it'd only make sense to do this on NSEventPhaseEnded.
Comment 21 Markus Stange [:mstange] 2011-08-15 16:22:03 PDT
(In reply to Steven Michaud from comment #19)
> One problem with doing that
> is that gestureAmount might not yet have been initialized.

Good point. Have you seen any strange values during testing?
gestureAmount most likely won't be an integer value at that point, but since NSEventPhaseEnded states that *something* will happen, the sign of gestureAmount might be sufficient data.

> But for now I'm desperately trying to fix bug 678607, and things haven't
> been going well.  Needless to say, if we can't fix those crashes, it will be
> pointless continuing to work on this bug (and related bugs).

Right. Good luck!

> Feel free to try this yourself, though.

I will, as soon as I'm near my Lion machine again, and that won't happen this week :-)
Comment 22 Alex Faaborg [:faaborg] (Firefox UX) 2011-08-15 18:02:40 PDT
>Seems a bit startling.

agreed, but might be better than feeling really unresponsive?

The fact that the animation and our delay are the roughly the same amount of time seems oddly coincidental.  Are we sure there isn't another event that we could monitor so that we could start the navigation as soon as the user has completed enough of the gesture for it to activate?
Comment 23 Steven Michaud [:smichaud] (Retired) 2011-08-15 18:13:20 PDT
> Are we sure there isn't another event that we could monitor so that
> we could start the navigation as soon as the user has completed
> enough of the gesture for it to activate?

There's Markus' suggestion from comment #18 (and mine from comment
#19).

I'll follow it up if/when I can fix bug 678607.
Comment 24 Steven Michaud [:smichaud] (Retired) 2011-08-17 13:07:14 PDT
Created attachment 553879 [details] [diff] [review]
Quick fix -- reduce delay

Markus, your suggestion from comment #18 worked out very well.

Though gestureAmount's values when isComplete isn't TRUE aren't
documented, my tests show they're still usable.  And, best of all,
following your suggestion seems to greatly reduce the delay ... though
others will be better able to judge.

A tryserver build will follow in a few hours.
Comment 25 Markus Stange [:mstange] 2011-08-17 13:41:12 PDT
Comment on attachment 553879 [details] [diff] [review]
Quick fix -- reduce delay

Why the "|| isComplete"? Won't that make us send two events? One when the user lifts his fingers off the trackpad / mouse, and one when the invisible animation completes, I'd think.
Comment 26 Steven Michaud [:smichaud] (Retired) 2011-08-17 14:01:44 PDT
> Why the "|| isComplete"? Won't that make us send two events?

You're right.  Don't know what I was thinking :-(

New patch coming up.
Comment 27 Steven Michaud [:smichaud] (Retired) 2011-08-17 14:15:58 PDT
Created attachment 553901 [details] [diff] [review]
Quick fix rev1 (fix problem)
Comment 28 Markus Stange [:mstange] 2011-08-17 14:21:29 PDT
Comment on attachment 553901 [details] [diff] [review]
Quick fix rev1 (fix problem)

I'm sorry but I don't think this is correct either :)
mSwipeAnimationCancelled isn't set to nil if the user cancels the swipe gesture. Either you need to set it to nil for NSEventPhaseCanceled, too, or you could just add another if (isComplete) and move mSwipeAnimationCancelled = nil there. The latter is what the Apple sample code does.
Comment 29 Steven Michaud [:smichaud] (Retired) 2011-08-17 15:24:35 PDT
Created attachment 553926 [details] [diff] [review]
Quick fix rev2 (fix another problem)

How's this?
Comment 30 Markus Stange [:mstange] 2011-08-17 15:28:47 PDT
Comment on attachment 553926 [details] [diff] [review]
Quick fix rev2 (fix another problem)

This is good :)
Comment 31 Steven Michaud [:smichaud] (Retired) 2011-08-17 15:56:57 PDT
Comment on attachment 553926 [details] [diff] [review]
Quick fix rev2 (fix another problem)

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/35e465c9d470
Comment 32 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-18 03:57:30 PDT
http://hg.mozilla.org/mozilla-central/rev/35e465c9d470
Comment 33 Steven Michaud [:smichaud] (Retired) 2011-08-19 08:57:40 PDT
My patch for this bug is now in mozilla-central 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 it out, and let us know what you think!
Comment 34 Steven Michaud [:smichaud] (Retired) 2011-08-19 09:09:03 PDT
My previous comment gave the wrong link.  Here's the correct one:

ftp://ftp.mozilla.org/pub/firefox/nightly/2011-08-19-03-07-49-mozilla-central/firefox-9.0a1.en-US.mac.dmg
Comment 35 MagnitudePAPAPA 2011-08-19 15:24:23 PDT
works perfectly for me, why don't you put this fix in 8 or 9 version ?
Comment 36 Steven Michaud [:smichaud] (Retired) 2011-08-19 15:41:52 PDT
> works perfectly for me

Glad to	hear it.

> why don't you put this fix in 8 or 9 version ?

If things go well, this fix should appear in FF 9.

For the reasons why I don't think it (or other parts of my fix for bug
668953) should appear in earlier FF releases, see bug 668953 comment
#129.
Comment 37 Alex Limi (:limi) — Firefox UX Team 2011-11-22 20:19:22 PST
Without an animation that shows what's happening, this is way too easy to trigger accidentally. If we're going to ship this, it has to happen with the accompanying animation that indicates the back/forward movement (the bug number escapes me right now, but the one that shows the page sliding over the previous one).
Comment 38 Alex Limi (:limi) — Firefox UX Team 2011-11-22 20:22:00 PST
Ah, found it — bug 678392.

And I've been accidentally triggering the back action many many times over the past weeks, just didn't realize what was happening.
Comment 39 Steven Michaud [:smichaud] (Retired) 2011-11-22 20:41:30 PST
Alex, you've reopened the wrong bug.  The delay that this bug is about has long since been fixed.
Comment 40 Alex Limi (:limi) — Firefox UX Team 2011-11-22 20:43:14 PST
Bleh, that's what I get for reading too quickly. Sorry, everyone!

The problem still remains, though — we are doing a back gesture without any feedback as to what happened, do you know which bug I should reopen to make sure it doesn't ship without the visual feedback?
Comment 41 Steven Michaud [:smichaud] (Retired) 2011-11-22 21:03:51 PST
It's bug 668953.

But you need to argue your case, which you manifestly haven't yet done -- either in your current comments or at bug 668953 comment #158.

Yes, as I acknowledge in bug 668953 comment #172, it's a bit too easy to do a back swipe accidentally.  And though in most cases this doesn't really cause trouble, in unusual cases it can cause loss of data.

But how is that worse that not having any support at all for two finger horizontal swipes?  Especially considering that this (apparently) is the #1 support request for Lion.

You also need to bring Asa back into the discussion, and hear what he has to say.
Comment 42 Steven Michaud [:smichaud] (Retired) 2011-11-22 21:10:29 PST
You also need to familiarize yourself with the alternatives to my patch for bug 668953 at bug 678392 and bug 698761.  Both have tryserver builds you should test.
Comment 43 Steven Michaud [:smichaud] (Retired) 2011-11-22 21:44:40 PST
> But you need to argue your case, which you manifestly haven't yet
> done -- either in your current comments or at bug 668953 comment
> #158.

Oops, I mixed you up with Alex Faaborg (who wrote bug 668953 comment
#158).  Sorry :-(

But my main points still stand.  Nobody has yet made a convincing case
for backing out my patch for bug 668953.  And to hold an informed
discussion on bug 668953 you also need to look at its possible
alternatives -- the patches for bug 678392 and bug 698761.
Comment 44 Valentin Tutu 2011-11-23 00:39:15 PST
> But how is that worse that not having any support at all for two finger
> horizontal swipes?  Especially considering that this (apparently) is the #1
> support request for Lion.

Honestly, have you tried to use it for few a days with a touchpad? (no irony here)
I'm constantly triggering the go-to-previous-page action when I'm scrolling a large page (try any ars technica article with many comments). And not only that, but it takes a while to understand what happens.

I'm not trying to start a flame, it's just my opinion, as a regular Firefox user. I appreciate the great work that you've all done with Firefox.
Comment 45 Steven Michaud [:smichaud] (Retired) 2011-11-23 07:25:45 PST
Valentin:

Yes, I have tried the two-finger swipe implemented by bug 668953, and I use it quite often (though not every day).  And like I also said, I realize the current implementation isn't ideal.

But you haven't answered my question.  In any serious discussion of this issue, one needs to take account not only of how bad something is, but whether or not the alternatives are worse.
Comment 46 Vlad [QA] 2011-11-25 02:29:06 PST
Setting resolution to Verified Fixed on:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20100101 Firefox/9.0 beta 3
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111124 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111124 Firefox/11.0a1

I've used a magic mouse with two-finger swipe function active. The back/forward procedure is made instantly, it lags only when the site is loading but I think this is due to internet connection.
Comment 47 Alex Limi (:limi) — Firefox UX Team 2011-11-28 01:28:29 PST
(In reply to Steven Michaud from comment #43)
> Oops, I mixed you up with Alex Faaborg (who wrote bug 668953 comment
> #158).  Sorry :-(

It happens, no worries. :)

> But my main points still stand.  Nobody has yet made a convincing case
> for backing out my patch for bug 668953.  And to hold an informed
> discussion on bug 668953 you also need to look at its possible
> alternatives -- the patches for bug 678392 and bug 698761.

The problem basically comes down to "first, do no harm". Doing the back/forward accidentally — and it's very easy to do this unintentionally without the animation, I do it multiple times a day myself — causes confusion, and in some cases unwanted (blocking!) dialog boxes. It's better to ship without this than to ship with an implementation that has unwanted side effects. The action and the animation should land together. Especially since you already have an implementation of this, it can't hurt to delay it until it's ready to land together.

I will try the tryserver builds tomorrow, thanks for pointing me to them, and for realizing that I'm the other Alex. :)
Comment 48 Steven Michaud [:smichaud] (Retired) 2011-11-29 08:48:27 PST
(In reply to comment #47)

> The problem basically comes down to "first, do no harm".

OK.  This is certainly reasonable.  The problem is that, given the
high demand for support for the two-finger horizontal swipe, we also
do a certain amount of harm by continuing not to support it.  So we
need to weigh the "harms".

> and in some cases unwanted (blocking!) dialog boxes

This is another concrete example of the harm caused by implementing
the two-finger horizontal swipe without implementing animation to help
users track it.  We need more of these!  Please write it up with
detailed STR at bug 668953.

I'm not	(in principle) opposed to backing out my patch for bug 668953.
I *am* opposed to doing it thoughtlessly, without having properly
considered all the alternatives.

At least for now, I'm inclined to think	we should quickly land some
kind of animation support in time for the 9.0 release -- presumably my
patch for bug 698761 (since the	patch for bug 678392 is	so complex,
though even I think it's clearly better).  But even that raises a host
of issues (not least that neither patch's animation is smooth on busy
pages).

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