Closed
Bug 678891
Opened 13 years ago
Closed 13 years ago
[10.7] Bug 668953's current two-finger swipe has ~700ms delay but no animation to hide it
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla9
People
(Reporter: djspiewak, Assigned: smichaud)
References
(Blocks 1 open bug)
Details
(Keywords: verified-aurora, verified-beta, Whiteboard: [inbound][qa!])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Summary: [10.7] Two-finger swipe for back/forward has ~700ms delay → [10.7] Two-finger swipe for back/forward has ~700ms delay but no animation to hide it (side effect of current patch for bug 668953)
Assignee | ||
Updated•13 years ago
|
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
Version: 7 Branch → unspecified
Assignee | ||
Updated•13 years ago
|
Summary: [10.7] Two-finger swipe for back/forward has ~700ms delay but no animation to hide it (side effect of current patch for bug 668953) → [10.7] Bug 668953's current two-finger swipe has ~700ms delay but no animation to hide it
Reporter | ||
Comment 2•13 years ago
|
||
(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.
Assignee | ||
Comment 3•13 years ago
|
||
What do you think, Asa? :-)
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
Provisionally taking this bug ... though I don't really have time for it.
Assignee: nobody → smichaud
Reporter | ||
Comment 6•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 7•13 years ago
|
||
Busy cursor, maybe?
Reporter | ||
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
Yeah, suppose you're right.
Could we make the progress bar show up immediately, on 0%?
Reporter | ||
Comment 10•13 years ago
|
||
(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•13 years ago
|
||
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.
Reporter | ||
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
> 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•13 years ago
|
||
Could we change the previous/forward button to an active state?
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
> 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•13 years ago
|
||
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?
Assignee | ||
Comment 19•13 years ago
|
||
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 :-)
Assignee | ||
Comment 20•13 years ago
|
||
> on NSEventPhaseEnded or NSEventPhaseCancelled
Actually it'd only make sense to do this on NSEventPhaseEnded.
Comment 21•13 years ago
|
||
(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•13 years ago
|
||
>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?
Assignee | ||
Comment 23•13 years ago
|
||
> 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.
Assignee | ||
Comment 24•13 years ago
|
||
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.
Attachment #553879 -
Flags: review?(mstange)
Comment 25•13 years ago
|
||
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.
Assignee | ||
Comment 26•13 years ago
|
||
> 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.
Assignee | ||
Comment 27•13 years ago
|
||
Attachment #553879 -
Attachment is obsolete: true
Attachment #553879 -
Flags: review?(mstange)
Attachment #553901 -
Flags: review?(mstange)
Comment 28•13 years ago
|
||
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.
Assignee | ||
Comment 29•13 years ago
|
||
How's this?
Attachment #553901 -
Attachment is obsolete: true
Attachment #553901 -
Flags: review?(mstange)
Attachment #553926 -
Flags: review?(mstange)
Comment 30•13 years ago
|
||
Comment on attachment 553926 [details] [diff] [review]
Quick fix rev2 (fix another problem)
This is good :)
Attachment #553926 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 31•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Comment 32•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Assignee | ||
Comment 33•13 years ago
|
||
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!
Assignee | ||
Comment 34•13 years ago
|
||
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•13 years ago
|
||
works perfectly for me, why don't you put this fix in 8 or 9 version ?
Assignee | ||
Comment 36•13 years ago
|
||
> 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.
Updated•13 years ago
|
Blocks: lion-compatibility
Updated•13 years ago
|
status-firefox6:
--- → affected
status-firefox7:
--- → affected
status-firefox8:
--- → affected
status-firefox9:
--- → fixed
Comment 37•13 years ago
|
||
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•13 years ago
|
||
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.
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 39•13 years ago
|
||
Alex, you've reopened the wrong bug. The delay that this bug is about has long since been fixed.
Assignee | ||
Updated•13 years ago
|
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 40•13 years ago
|
||
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?
Assignee | ||
Comment 41•13 years ago
|
||
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.
Assignee | ||
Comment 42•13 years ago
|
||
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.
Assignee | ||
Comment 43•13 years ago
|
||
> 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•13 years ago
|
||
> 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.
Assignee | ||
Comment 45•13 years ago
|
||
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•13 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora,
verified-beta
Whiteboard: [inbound][qa+] → [inbound][qa!]
Comment 47•13 years ago
|
||
(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. :)
Assignee | ||
Comment 48•13 years ago
|
||
(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).
You need to log in
before you can comment on or make changes to this bug.
Description
•