1325931 - Activity stream - Title is not centred in the Top sites tile when website is pinned

Status: VERIFIED FIXED

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

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: Pin icon and title.png

Environment: 
Device: Prestigio Grace 5x (Android 4.4.2);
Build: latest Nightly 53.0a1 (2016-12-26);

Steps to reproduce:
1. Launch Firefox;
2. Tap on the 3 dots menu of a website from Top sites area;
3. Tap on “Pin site” option.

Expected result:
Website is pinned and the title remains centred inside the tile.

Actual result:
Website is pinned, but the title is moved a bit to the right side of the tile. 

Notes:
Please see the screenshot attached.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Bryan, do we have mocks for this somewhere?

Comment 2

[Andrzej Hunt :ahunt]

I've been playing with this a little, and it's slightly tricky (Android doesn't seem to support the concept of centering text relative to the parent View, it insists on centering within the available area - which gets reduced when we add a drawable - even if it's in a separate ImageView). One option is to have hardcoded padding on both sides (where we can then draw the pin), but that might look odd too. Or we can do dynamic calculations, but those will be tricky to perform correctly.

Or we can make the pin float (i.e. not make it aligned to the left of the card), which make it easier to center things.

Or we could align the text to the left (which is what conventional topsites does)

Comment 3

[Andrzej Hunt :ahunt]

Heh, old topsites on desktop does _exactly_ the same thing (which makes me wonder if this is MVP-worthy).

I've managed to create an incredibly hacky fix, I'll leave this as P2 until I decide if it's really the right approach. (I'll probably move the code involved into ViewUtil, mainly for cleanliness, and also in case we ever need it elsewhere.)

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1325931 - Pre: add drawable padding to pin

This is one huge hack.

Review commit: https://reviewboard.mozilla.org/r/111288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111288/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1325931 - Implement ViewUtil.setCenteredText() for TextViews with compound drawables

Compound drawables shift the point where text is "centered". This hack dynamically adds
equivalent padding on the opposite side from a compound drawable, to force the text
to be centered again. (We can't set this padding under all circumstances, it's unneeded
when the text is longer than the available space, i.e. when we wrap text we might
as well use the full width without fake padding.)

Review commit: https://reviewboard.mozilla.org/r/111352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111352/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1325931 - Use compound drawable text centering hack for AS Topsites

Review commit: https://reviewboard.mozilla.org/r/111354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111354/

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8835613 [details]
Bug 1325931 - Pre: add drawable padding to pin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111288/diff/1-2/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: device-2017-02-09-143334.png

A screenshot from when I was testing with a drawable on the right (my patches keep it on the left, I was just verifying that my code covers both sides as needed).

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8835613 [details]
Bug 1325931 - Pre: add drawable padding to pin

https://reviewboard.mozilla.org/r/111288/#review113000

::: mobile/android/base/resources/layout/activity_stream_topsites_card.xml:33
(Diff revision 2)
>              android:layout_below="@id/favicon"
>              android:background="@color/activity_stream_divider" />
>  
>          <TextView
>              android:id="@+id/title"
> +            android:drawablePadding="2dp"

I'm not sure what the "right" value would be, but currently we have no padding which can look bad.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8835613 [details]
Bug 1325931 - Pre: add drawable padding to pin

https://reviewboard.mozilla.org/r/111288/#review113812

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8835738 [details]
Bug 1325931 - Implement ViewUtil.setCenteredText() for TextViews with compound drawables

https://reviewboard.mozilla.org/r/111352/#review113814

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8835739 [details]
Bug 1325931 - Use compound drawable text centering hack for AS Topsites

https://reviewboard.mozilla.org/r/111354/#review113816

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5d12ac7e213
Pre: add drawable padding to pin r=sebastian
https://hg.mozilla.org/integration/autoland/rev/596c114a2524
Implement ViewUtil.setCenteredText() for TextViews with compound drawables r=sebastian
https://hg.mozilla.org/integration/autoland/rev/7697b05969f8
Use compound drawable text centering hack for AS Topsites r=sebastian

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5d12ac7e213
https://hg.mozilla.org/mozilla-central/rev/596c114a2524
https://hg.mozilla.org/mozilla-central/rev/7697b05969f8

Comment 15

[Carmen Fat [:cfat] - Ecosystem QA]

Tested using the latest Nightly build 54.0a1 (2017-02-16) with the following devices:
- Oneplus Two (Android 6.0.1);
- Samsung Galaxy Note 4 (Android 5.0.1);
- Huawei MediaPad M2 (Android 5.1.1).

After pinning a website, it's title stays centred into the tile as in the next Screenshot: http://imgur.com/a/sKW1A

I'm marking this as Verified.