715272 - [ICS] On about:home, add padding to thumbnail selection highlight

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

There should be a wider selection highlight around the thumbnail and text in Top Sites. Gingerbread phones show this correctly, but the Galaxy Nexus only shows a tiny border right now.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Ian, Could you please share spec for this?

Comment 2

[Ian Barlow (:ibarlow)]

Sriram, the correct selection border around the thumbnail should be 

Top 5dip
Right 5dip
Bottom 20dip (to include title)
Left 5dip

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

I changed it to RelativeLayout to reduce the number of draws. I reused resources from tabs-tray. If the shadow is fine in this, we can remove abouthome_topsite_shadow resources.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Padding

Is the padding in this fine? I am still not happy with the font-size of the thumbnail. It's currently 9dip (which is less than "sp"). And the recommended small size is 12sp I guess.

Comment 5

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

Comment on attachment 593648 [details] [diff] [review]
WIP

Did you do some testing on different devices and portrait/landscape rotations? I just want to make sure the relative layout is robust enough to handle different layout changes.

Comment 6

[Ian Barlow (:ibarlow)]

Hi Sriram, couple things:

1. You need some padding on the top of your selection, it's getting cut off right now

2. I agree that font is too small, it seems like this might be related to bug 715259, where type and graphics aren't being scaled up appropriately based on their screen density. Ping me in IRC today if you'd like to chat about it some more.

Comment 8

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

Comment on attachment 593648 [details] [diff] [review]
WIP

This r+ is given as long as you tested this patch on multiple devices and portrait/landscape.

Also, if this patch was bitrotted a lot, post a new patch for a quick review.

Comment 9

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

Comment on attachment 593648 [details] [diff] [review]
WIP

I just saw Ian's feedback on the screenshot. Please address that as well and post a new patch and screenshot.

Comment 10

[Ian Barlow (:ibarlow)]

Soft blocking nom?

I think we're almost done here, so let's tie this off.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch has the required changes (and is "bit-trolled" ;) )
It's pretty much the same as in tabs_row.xml.

The only thing that is still confusing me that there is a 1px padding around the edges of thumbnails. I couldn't figure out why the thumbnail is different between tabs and in db. By "grep" for "136" failed me. :(

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Padding

Screenshot of the padding after the patch is applied.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Padding

Comment 14

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

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

What affect does this have on non-ICS phones. Test it out and report back please.

Comment 15

[Sriram Ramasubramanian [:sriram]]

As the changes are common for both ICS and non-ICS versions, the padding fix applies there too. This works fine on Galaxy SII running Gingerbread.

Comment 16

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

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

Thank you for checking. Clear to land.

Comment 17

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d585de9a3103

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d585de9a3103

Comment 19

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-05-22)
Device: Galaxy Nexus
OS: Android 4.0.2