Closed Bug 519577 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b5
Tracking Status
fennec 1.0+ ---

People

(Reporter: madhava, Assigned: mfinkle)

Details

(Whiteboard: [polish])

Attachments

(1 file)

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.
tracking-fennec: --- → ?
Whiteboard: [polish]
Flags: in-litmus?
s/collapse/collide  ?

I am finding that the feedback on touch is good at all times. We might close/wontfix bug 516575
Bug 516575 is a different thing -- it's about not interpreting pans as taps.  This one is about rows in lists in chrome.
Priority: -- → P1
tracking-fennec: ? → 1.0+
(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...
Attached patch patchSplinter Review
* 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)
Assignee: nobody → mark.finkle
Attachment #408601 - Flags: review?(gavin.sharp)
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...
Attachment #408601 - Flags: review?(gavin.sharp) → review+
(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
pushed:
https://hg.mozilla.org/mobile-browser/rev/221bbc0aa0de
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B5
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.