745225 - No highlighting when clicking a synced tab link on About:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tony Chung [:tchung]]

on About:home page, the links from "synced tabs" listed at the bottom, do not get highlighted when tapping on them to launch.   But, they are highlighted just fine in the section above, "Your tabs from last time"

Nom'ing for triage, im thinking soft with low-risk fix?

Repro:
1) 4-12-2012 nightly, Galaxy Nexus (Andr 4.0.2), and HTC Sensation (Andr 2.3.4)
2) setup sync to desktop, and sync some tabs
3) goto about:home, and scroll to bottom where Synced tabs section is
4) tap a link there.  verify no highlighting of link.

Expected:
- Highlight synced tabs link just like how we highlight other links on this page

Actual:
- synced tabs link dont highlight.

Comment 1

[Aaron Train [:aaronmt]]

CC'ing Sriram

Comment 2

[Sriram Ramasubramanian [:sriram]]

This doesnt happen for any row in about:home.

Comment 3

[Sriram Ramasubramanian [:sriram]]

All the rows in about:home used a 9 png background: https://hg.mozilla.org/mozilla-central/raw-file/c61e7c3a232a/mobile/android/base/resources/drawable/abouthome_separator.9.png

This 9 png has the "2dp" divider to be shown above the row, and a transparent region that doesnt affect the row. This is not a "state-level" drawable to show a highlight on the row. Also, this cannot be done with this resource.

Reasons:
* The system highlight color cannot be "added" with the divider on the fly. So, the divider and the highlight/normal drawables should be separated.
* If the divider is separated, it should have it's own view. This needs to be added with every row added (Either as a part of the row, or as a part of the parent).

This is a bigger change and affects the layout of entire about:home. Probably this can be taken up only after 1.0 cut-off so that it doesnt affect anything much.

(On a side note, I have a patch that tries to do this -- half way through -- and there is a real good performance improvement).

Dirty fix: State-level drawable with normal state as it is now, and highlight state from the system. The divider won't be visible on highlight. And there will be some sort of visual glitch due to the divider vanishing on press.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This cleanup the about:home content. The dividers are not with the background, but added separately. Hence each section will have its own dividers (add to each row / more text / title / subtitle).

This is a prerequisite to re-arrange sections based on need for tablets.

Comment 5

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

Comment on attachment 641323 [details] [diff] [review]
Patch

Do we need to add the new views to the view factory for performance?
I notice we put a lot of colors in the layout XML. Should those be added to color.xml ?

Comment 6

[Sriram Ramasubramanian [:sriram]]

I thought of doing that. We currently do that optimization for static inner classes. I'll do that as a followup.

Also, I am thinking of renaming all "org.mozilla.gecko.Something" to "Gecko.Something" in XML, its simple and short. Now that we are supplying the required class, we can rename the XML tag to anything of our likes.

Comment 7

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/11900afd18de

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch 2: Custom view optimizations

This patch optimizes static inner classes.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch 3: Rename

This patch renames our custom views to "Gecko.Something". This is more readable and ensures a good XML coding pattern.

1. Any custom view should be Gecko.Something.
2. "Something" should be added to GeckoViewsFactory.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11900afd18de

Comment 11

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 641323 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: No highlighting on about:home.
Testing completed (on m-c, etc.): Landed on m-c on 07/13
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 641323 [details] [diff] [review]
Patch

no risk patch, approved.

Comment 13

[Sriram Ramasubramanian [:sriram]]

This patch has been come to aurora after merge.

Comment 14

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ae50075c688

Comment 15

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

Comment on attachment 641636 [details] [diff] [review]
Patch 2: Custom view optimizations

File a new bug if we still want this patch

Comment 16

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

Comment on attachment 641647 [details] [diff] [review]
Patch 3: Rename

File a new bug if you still want this patch