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)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox6 --- affected
firefox7 --- affected
firefox8 --- affected
firefox9 --- fixed

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)

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.
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?
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)
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
Version: 7 Branch → unspecified
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
(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.
What do you think, Asa? :-)
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.
Provisionally taking this bug ... though I don't really have time for it.
Assignee: nobody → smichaud
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Busy cursor, maybe?
(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).
Yeah, suppose you're right. Could we make the progress bar show up immediately, on 0%?
(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...
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.
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 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.
Could we change the previous/forward button to an active state?
(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?
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.
> 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?
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?
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 :-)
> on NSEventPhaseEnded or NSEventPhaseCancelled Actually it'd only make sense to do this on NSEventPhaseEnded.
(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 :-)
>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?
> 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.
Attached patch Quick fix -- reduce delay (obsolete) (deleted) — Splinter Review
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 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.
> 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.
Attached patch Quick fix rev1 (fix problem) (obsolete) (deleted) — Splinter Review
Attachment #553879 - Attachment is obsolete: true
Attachment #553879 - Flags: review?(mstange)
Attachment #553901 - Flags: review?(mstange)
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.
How's this?
Attachment #553901 - Attachment is obsolete: true
Attachment #553901 - Flags: review?(mstange)
Attachment #553926 - Flags: review?(mstange)
Comment on attachment 553926 [details] [diff] [review] Quick fix rev2 (fix another problem) This is good :)
Attachment #553926 - Flags: review?(mstange) → review+
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
Whiteboard: [inbound]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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!
works perfectly for me, why don't you put this fix in 8 or 9 version ?
> 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.
Whiteboard: [inbound] → [inbound][qa+]
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).
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Alex, you've reopened the wrong bug. The delay that this bug is about has long since been fixed.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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?
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.
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.
> 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.
> 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.
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.
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
Whiteboard: [inbound][qa+] → [inbound][qa!]
(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. :)
(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.

Attachment

General

Created:
Updated:
Size: