833800 - The pinned site indicator is not visible on dark thumbnails

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Attachment: screenshot

Firefox for Android
Version: 21.0a1 (2013-01-23)
Device: Galaxy R
OS: Android 2.3.4

Steps to reproduce:
1. Go to popuptest.com/popuptest1.html
2. Go to about:home and pin the site opened at step 1

Expected result:
The pin indicator is visible

Actual result: 
The pin indicator is not visible due to the dark top side of the thumbnail (see attached screenshot)

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Ian should this icon have a glow around it? Do you have some sort of other idea?

Comment 2

[Sriram Ramasubramanian [:sriram]]

May be we can crop the thumbnail on top right.
Or add a small shadow to entire thumbnail.

Comment 3

[Ian Barlow (:ibarlow)]

The original intent was to use a combination of the right grey and the right transparency so that it would show up everywhere. I would suggest adjusting the corner colour here to #666666 and giving it 70% opacity. 

That'll look something like this, visible on white and also on black: http://cl.ly/image/063J123W433C

Comment 4

[Aaron Train [:aaronmt]]

Saw a complaint about this on Google Play for FxBeta yesterday

Comment 5

[Sriram Ramasubramanian [:sriram]]

Shall we make it orange? It'll be clear. Using opacity would interfere with the thumbnail image anyways.

Comment 6

[Wesley Johnston (:wesj)]

I think that would just break on orange thumbnails? We need to add a light highlight to it on that one edge.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Screenshot (newer)

Just wanted to see what it looks like in modern times

Comment 9

[Wesley Johnston (:wesj)]

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

Review of attachment 804539 [details] [diff] [review]:
-----------------------------------------------------------------

Moving to an image here.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 11

[Wesley Johnston (:wesj)]

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

Review of attachment 806110 [details] [diff] [review]:
-----------------------------------------------------------------

Nice deleting.

::: mobile/android/base/resources/layout/top_bookmark_item_view.xml
@@ +19,2 @@
>              android:duplicateParentState="true"
> +            android:drawablePadding="4dip"

Put this in dimen and maybe a style?

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 806110 [details] [diff] [review]
> Patch
> 
> Review of attachment 806110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice deleting.
> 
> ::: mobile/android/base/resources/layout/top_bookmark_item_view.xml
> @@ +19,2 @@
> >              android:duplicateParentState="true"
> > +            android:drawablePadding="4dip"
> 
> Put this in dimen and maybe a style?

This is a one-time use value. Putting it in a style file would involve unnecessary lookups. I feel like having it here.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Fading in text-view

The earlier calculations didn't account for compound drawables. This patch accounts for it. (Note: This cases is only for pin icon).

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 806123 [details] [diff] [review]
Patch: Fading in text-view

Review of attachment 806123 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.

::: mobile/android/base/home/FadedTextView.java
@@ +56,5 @@
>          // Layout doesn't return a proper width for getWidth().
>          // Instead check the width of the first line, as we've restricted to just one line.
>          if (getLayout().getLineWidth(0) > width) {
> +            final Drawable leftDrawable = getCompoundDrawables()[0];
> +            int drawableWidth = 0;

nit: I'd reverse the order of lines above:
int drawableWidth...
final Drawable...

@@ +66,1 @@
>              int color = getCurrentTextColor();

nit: final

Comment 15

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/2839a4856902
http://hg.mozilla.org/projects/fig/rev/baf63262542e

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: LinearLayout

Ideally LinearLayout is good. But it doesn't have the third constructor in pre-ICS version. But we dump few things in style which will be used by Android. So, reverting back to RelativeLayout -- "all is well" world.

Comment 17

[:Margaret Leibovic]

Comment on attachment 806624 [details] [diff] [review]
Patch: LinearLayout

Looks good, it looks like this just reverts to what we were doing before in TopBookmarkItemView.

Out of curiosity, why was this change to a LinearLayout made in your other patch?

Comment 18

[Sriram Ramasubramanian [:sriram]]

We had a pin icon sticking on top left. In that case it's better to use a RelativeLayout (saves one ViewGroup in the middle). But then the pin is now a part of the title. So the image and the text can be placed in a vertical LinearLayout. However it didn't help due to Android code.

Comment 19

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/353f00637313

Comment 20

[:Margaret Leibovic]

Setting to track 26, since this is part of the follow-up work to bug 917394.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2839a4856902
https://hg.mozilla.org/mozilla-central/rev/baf63262542e
https://hg.mozilla.org/mozilla-central/rev/353f00637313

Comment 22

[Sriram Ramasubramanian [:sriram]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old design.
User impact if declined: The pin on top thumbnails is not seen.
Testing completed (on m-c, etc.): Landed safely in m-c on 09/20.
Risk to taking this patch (and alternatives if risky): No risk.
String or IDL/UUID changes made by this patch: None.

Comment 23

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 806123 [details] [diff] [review]
Patch: Fading in text-view

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old design.
User impact if declined: The pin on top thumbnails is not seen.
Testing completed (on m-c, etc.): Landed safely in m-c on 09/20.
Risk to taking this patch (and alternatives if risky): No risk.
String or IDL/UUID changes made by this patch: None.

Comment 24

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 806624 [details] [diff] [review]
Patch: LinearLayout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old design.
User impact if declined: The pin on top thumbnails is not seen.
Testing completed (on m-c, etc.): Landed safely in m-c on 09/20.
Risk to taking this patch (and alternatives if risky): No risk.
String or IDL/UUID changes made by this patch: None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Depends on bug 917396, which doesn't have approval yet.

Comment 26

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/067c524d378f
https://hg.mozilla.org/releases/mozilla-aurora/rev/cdd031129be5
https://hg.mozilla.org/releases/mozilla-aurora/rev/c44623ea46ea

Comment 27

[Paul Feher]

Verified fixed on:
Firefox 26 Beta 1 (2013-10-26)
Device: LG Optimus 4X
OS: Android 4.1.2