519577 - Use row-highlighting to indicate a successful tap on a row

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Madhava Enros [:madhava]]

In the awesomebar/bookmarks lists, there is no immediate feedback for having tapped on a row, except in one case -- tapping on the "See All Bookmarks" row.  That one has it because it can take time to bring up the bookmarks list, so it was extra obvious that some immediate feedback was needed.  The others could use it too -- having it would make the Fennec feel more responsive and a little less random.  It would be more obvious when you'd tapped vs. swiped, and also how dragging off the screen can cancel a tap.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I think this bug collapse with bug 516575.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

s/collapse/collide  ?

I am finding that the feedback on touch is good at all times. We might close/wontfix bug 516575

Comment 3

[Madhava Enros [:madhava]]

Bug 516575 is a different thing -- it's about not interpreting pans as taps.  This one is about rows in lists in chrome.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> In the awesomebar/bookmarks lists, there is no immediate feedback for having
> tapped on a row, except in one case -- tapping on the "See All Bookmarks" row.

That doesn't match what the code does - we explicitly select both normal bookmark items and the "All Bookmarks" item before closing the popup. Maybe there's a delay caused by the inputmanager code or something, but what you describe matches what the code _should_ already be doing.

(In reply to comment #3)
> Bug 516575 is a different thing -- it's about not interpreting pans as taps. 
> This one is about rows in lists in chrome.

That wasn't my intent when I filed it. I filed that because there's no point in "selecting" the item if the popup is being closed immediately - you wouldn't have a chance to see the selection. The only reason we started doing it was because the closing took too long. If we want to intentionally delay the closing, then I guess we should morph it...

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

* Use new blue as the selection color for awesomebar
* Use new blue as the selection color when tapping a richlistitem (add-ons, downloads) and bookmarks and awesomebar result
* Remove the need to _select_ an awesomebar item when tapping it. Instead, we just style it on tap.

Note: since we style on tap, the awesombar can have two rows highlighted: one you picked with the arrow keys and one you touched. This happens because tap highlighting is done on mousedown and arrow highlight is done on click (up and down)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 408601 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>       <method name="_styleItem">

>+          if (!aItem)
>+            return;

This is unnecessary, right? Let's omit it for now.

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

>+autocompleteresult:active,
>+placeitem:active {
>+  background-color: #8db8d8;
>+}
>+
> .autocompleteresult-selected {
>-  color: white;
>-  background-color: grey !important;
>+  background-color: #8db8d8 !important;
> }

Could merge these (wince as well)...

> /* XXX should be a richlistitem description.normal */
> .prefdesc {
>   font-size: 1.8mm !important;
>   color: grey;
>-  background-color: white;
> }

omit this change too?

The two-selected-rows-while-active aspect of this isn't ideal, but I suppose probably still better than the status quo...

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)

> >+          if (!aItem)
> >+            return;
> 
> This is unnecessary, right? Let's omit it for now.

Done

> >+autocompleteresult:active,
> >+placeitem:active {
> >+  background-color: #8db8d8;
> >+}
> >+
> > .autocompleteresult-selected {
> >-  color: white;
> >-  background-color: grey !important;
> >+  background-color: #8db8d8 !important;
> > }
> 
> Could merge these (wince as well)...

Done

> > .prefdesc {
> >   font-size: 1.8mm !important;
> >   color: grey;
> >-  background-color: white;
> > }
> 
> omit this change too?

Done

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/221bbc0aa0de

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED (and HAWT) on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091028 Namoroka/3.6b2pre Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091028 Namoroka/3.7a1pre Fennec/1.0b5pre

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Add to litmus: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9827