Open Bug 769857 Opened 7 years ago Updated 4 months ago

Delay showing tap highlight instantly on tap (50ms delay)

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

REOPENED
Tracking Status
firefox16 --- affected
firefox17 --- affected

People

(Reporter: st3fan, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: polish, ux-userfeedback, Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

When panning, the link under the tap is briefly highlighted before the panning starts. The tap should not be interpreted at all.
We intentionally provide highlighting as feedback as soon as you press down, to improve perceived responsiveness and to let you know what will happen if you complete the tap (so you have a chance to move your finger if it wasn't what you wanted to click).
I think we have to do this to feel responsive to users. Marking this invalid for now.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Why would you suggest that you are going to open a link if you are not? To me that has the opposite effect: I want to pan but I see a link highlighted. Panic. No no no stop opening, cancel. i just want to pan!
(In reply to Stefan Arentz [:st3fan] from comment #3)
> Why would you suggest that you are going to open a link if you are not?

At the time the user touches the screen, we don't know whether we are going to follow the link.  We won't know that until at least a half-second later, because we have to wait to see whether the user single-taps, long-taps, double-taps, or pans.  If we wait that long before giving any feedback, the lack of feedback adds its own confusion.

Maybe we could make this a little less sensitive, by adding a small delay before the tap highlight (maybe 10ms?) -- enough so that it won't happen on a quick swipe gesture, but not enough to hurt perceived responsiveness.

Other mobile browsers also have tap highlights that vanish if the tap becomes a pan.  The stock browser on ICS works similar to Firefox, with a highlight that appears almost immediately.  Chrome for Android has a noticeable delay before the highlight appears, and so do Opera Mobile and the stock Gingerbread browser.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → mbrubeck
OS: Mac OS X → Android
Hardware: x86 → All
Version: Firefox 14 → Trunk
Yeah that makes sense Matt. I think it is indeed a sensitivity issue. I do see the same behaviour on iOS/Safari but I think the grace period there is maybe a little longer. Not super long though, it is likely in the 10 to 50ms range or so.
Adjusting the title to something more specific then
Summary: Pan gesture briefly highlights link under tap → Delay showing tap highlight instantly on tap (50ms delay)
Attached patch WIP (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Based on trial and error, 50ms indeed seems to be a good compromise.  It doesn't completely eliminate premature highlights, but it makes them rather less common without any significant effect on responsiveness.
Attachment #641265 - Attachment is obsolete: true
Attachment #641301 - Flags: review?(wjohnston)
Comment on attachment 641301 [details] [diff] [review]
patch

Review of attachment 641301 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Let's try it.

::: mobile/android/chrome/content/browser.js
@@ +3466,5 @@
>    _scrollableElement: null,
>  
>    _highlightElement: null,
>  
> +  _timeout: null,

Lets use _highlightTimer or _highlightTimeout

@@ +3473,5 @@
> +    this._cancelTapHighlight();
> +    this._timeout = setTimeout(function(self) {
> +      DOMUtils.setContentState(aElement, kStateActive);
> +      self._highlightElement = aElement;
> +    }, 50, this);

Move this 50ms constant somewhere
Attachment #641301 - Flags: review?(wjohnston) → review+
Pushed with review comments fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d11ab4b1a8c
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7d11ab4b1a8c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Depends on: 775372
Depends on: 778333
Backed out because of bug 778333 and possibly other regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef483246388

In bug 775372 I requested Aurora approval for the backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 16 → ---
Depends on: 779339
(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Backed out because of bug 778333 and possibly other regressions:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef483246388
> 
> In bug 775372 I requested Aurora approval for the backout.

https://hg.mozilla.org/mozilla-central/rev/4ef483246388
Is it possible to put this back in? I really liked this change. It made a big difference to me.
I haven't had time to investigate the regressions properly.  If someone else has time before I do (Wes?), feel free to steal this bug from me.
I wonder if we can set the tapHighlight item but delay setting active on it. I think that might fix these races.
This is bug is also showing up when you scroll a selectable list of things.

For example the list of tabs in Firefox itself or on the Mozilla Marketplace, when you scroll the list of apps.

The effect is the same, the list item is selected and then deselected a little while later when you are scrolling the list.

In other words, it does not just happen with links, but with any clickable element?
Attached patch PatchSplinter Review
So I THINK that most of the problems we had before were because we weren't setting _highlightElement immediately when you tapped. That led to races where certain events expect a highlightElement and if it doesn't exist they.... fail. So this is basically the same patch except it sets highlightElement regardless, and just delays setting :active on it for some timeout.

Also I kept getting into situations where we'd set the highlight on something that had no ownerDocument (not sure what....) and then throw leading us to never reset the highlightElement and breaking.... everything. So I added a check for the elt.ownerDocument.
Assignee: mbrubeck → wjohnston
Attachment #641301 - Attachment is obsolete: true
Attachment #655676 - Flags: review?(mbrubeck)
Comment on attachment 655676 [details] [diff] [review]
Patch

>+  _timeout: null,

In comment 9 you asked me to use "_highlightTimeout"

>+    this._timeout = setTimeout(function() {
>+      DOMUtils.setContentState(aElement, kStateActive);
>+    }, 50);

...and to make this a named constant.

I also think you might need to check the ownerDocument before calling setContentState, like we do in _cancelTapHighlight.  See the original patch from bug 775372, for example.

(In reply to Wesley Johnston (:wesj) from comment #19)
> Also I kept getting into situations where we'd set the highlight on
> something that had no ownerDocument (not sure what....) and then throw
> leading us to never reset the highlightElement and breaking.... everything.
> So I added a check for the elt.ownerDocument.

This might be similar to what happened in bug 775372, when the timeout fired after the original page has been unloaded.

This code looks good, but please test thoroughly to make sure bug 775372 and bug 778333 (or similar situations) don't recur, and that no uncaught exceptions are logged (in particular if a new page loads while the highlight is showing or about to show).
Attachment #655676 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/58b937c070d6

For those wondering, mbrubeck did not r+ his own patch even though the commit says he did. I just forgot to hg qref -U it.
https://hg.mozilla.org/mozilla-central/rev/58b937c070d6
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Channel uplift?
I saw a report today of this causing problems. Don't want to uplift yet.
Blocks: 787540
backed out ^
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 18 → ---
Not actively working on this. The races here are difficult to figure out, but if someone wants to give it a shot, feel free.
Assignee: wjohnston → nobody
Whiteboard: [mentor=wesj][lang=js]
Mentor: wjohnston
Whiteboard: [mentor=wesj][lang=js] → [lang=js]
Mentor: wjohnston2000
You need to log in before you can comment on or make changes to this bug.