1177611 - Adjust private browsing tab tray color

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

When the private browsing tab tray is selected, we should highlight thumbnails in private_browsing_purple and the selected tab should use a private browsing purple indicator.

Follow-up to bug 864958.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This is a bit messy. The selected tab indicator is set as the background drawable on a TabWidget, with the foreground being the icon (e.g. private tabs mask). android:tint only affects the foreground so we can't use that. Tint drawables are API 21+ so we can't use that.

Then, I can set a color filter on the background drawable but the background drawable includes the pressed state (a solid color) and the color filter tints this in addition to the selected drawable.

Potential solutions:
  1) Set the color filter only when the item is not pressed or focused. (perf implications?)
  2) Have a new asset with purple indicators
  3) Rewrite the widget (though the current setup does what Android does)

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Tinted pressed state

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, re comment 2, this is the outcome pressed state (the tint is set as such, rather than solid, because of the alpha in the pressed state color). What do you think? I think it's either this, or include new assets (given the amount of time I should invest in this atm).

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8626709 [details]
Tinted pressed state

I think we should leave it as is then if getting the underline indicator to be Private Browsing purple is not feasible. I'm OK if we don't do anything with this pressed state especially.

Are we goign to use this bug to track the "active-state box" (that selects the active tab) changing to our PB purple?

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pb_tray3.png

Uploading design for context :)

Just to make sure, are we talking about the same things here Mike?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry, I meant to say as a side effect of the selected state being private browsing purple, the pressed state is tinted, like the screenshot. Is this okay? Otherwise, we can do the options in comment 1.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, I've got this working correctly with some really hacky code - I'll post a patch once it's worthy.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Friendly ping on the status here Mike? Getting the purple indicators will really round off the V1 UI updates.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #8)
> Friendly ping on the status here Mike? Getting the purple indicators will
> really round off the V1 UI updates.

Focusing on some sec bugs but I'll get to this right after. From what I've seen, the changes are not simple so I may be uncomfortable uplifting them - I've been considering backing out the other private browsing changes so this all lands in 42 with tracking protection (and reduces my burden a bit) - thoughts?

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

AFAICT we're releasing these UI updates to PB in 42

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I backed the private browsing changes out of 41 (see bug 864958 comment 51 which ties it all together).

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, what should the color of the tabs thumbnail background be when in private mode and it's pressed?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Bug 1177611 - Change private browsing tablet thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Bug 1177611 - Update ThemedView template to import the correct LWT. r=mhaigh

I believe I just used an IDE to update the files last time, meaning the
template never got updated.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Add ThemedFrameLayout. r=mhaigh

Bug 1177611 - Add ThemedFrameLayout. r=mhaigh

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Bug 1177611 - Change private browsing tablet thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638850 [details]
MozReview Request: Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Bug 1177611 - Update ThemedView template to import the correct LWT. r=mhaigh

I believe I just used an IDE to update the files last time, meaning the
template never got updated.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638851 [details]
MozReview Request: Bug 1177611 - Add ThemedFrameLayout. r=mhaigh

Bug 1177611 - Add ThemedFrameLayout. r=mhaigh

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still TODO:
  * Phone thumbnail color
  * Tab selector color

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #12)
> Anthony, what should the color of the tabs thumbnail background be when in
> private mode and it's pressed?

I think we can keep it the same for now and see how that looks.

Comment 21

[Martyn Haigh (:mhaigh)]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

https://reviewboard.mozilla.org/r/14137/#review12771

::: mobile/android/base/resources/drawable/tab_thumbnail.xml:56
(Diff revision 2)
>              <corners android:radius="3dp"/>            

It'd be nice to clean up a bit whilst we're here :)

::: mobile/android/base/resources/layout-v11/new_tablet_tabs_item_cell.xml:52
(Diff revision 2)
> -    <RelativeLayout android:layout_width="wrap_content"
> +    <!-- We set state_private on this View dynamically. -->

-> We set state_private on this View dynamically in TabsGridLayout

Comment 22

[Martyn Haigh (:mhaigh)]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

https://reviewboard.mozilla.org/r/14137/#review12773

Ship It!

Comment 23

[Martyn Haigh (:mhaigh)]

Comment on attachment 8638850 [details]
MozReview Request: Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

https://reviewboard.mozilla.org/r/14139/#review12775

Ship It!

Comment 24

[Martyn Haigh (:mhaigh)]

Comment on attachment 8638851 [details]
MozReview Request: Bug 1177611 - Add ThemedFrameLayout. r=mhaigh

https://reviewboard.mozilla.org/r/14141/#review12777

I'd like to see those formatting changes made before landing

::: mobile/android/base/widget/ThemedFrameLayout.java:19
(Diff revision 2)
> +                                     implements LightweightTheme.OnChangeListener {

Formatting - align extends and implements.

::: mobile/android/base/widget/ThemedFrameLayout.java:20
(Diff revision 2)
> +    private LightweightTheme mTheme;

Aren't we trying to get rid of the 'm' prefix?

::: mobile/android/base/widget/ThemedFrameLayout.java:33
(Diff revision 2)
> +    private boolean mAutoUpdateTheme;        // always false if there's no theme.

Personal preference here, but I'm a fan of comments above the lines they refer to.

::: mobile/android/base/widget/ThemedFrameLayout.java:62
(Diff revision 2)
> +        if (mAutoUpdateTheme)

I'm not a fan of the bracket-less single line if statements; I believe the common formatting for this codebase is to include them regardless of the number of lines

::: mobile/android/base/widget/ThemedFrameLayout.java:76
(Diff revision 2)
> +        final int[] drawableState = super.onCreateDrawableState(extraSpace + 1);

Why "+ 1"?

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/14137/#review12771

> It'd be nice to clean up a bit whilst we're here :)

I'm of the opinion that we shouldn't remove the WS unless we actually change the line because it obscures `hg blame` and makes it harder to figure out when those lines were added.

I'm going to leave it in, unless you're particularly passionate about it.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/14141/#review12777

To be explicit, I'm dropping all of these suggestions because this code is based off the template I keep mentioning, ThemedView.java.frag.

> Formatting - align extends and implements.

This is auto-generated - we align in the template but aligning in the generated output would be a lot of work.

> Aren't we trying to get rid of the 'm' prefix?

Convention in the template - I filed bug 1188198.

> Personal preference here, but I'm a fan of comments above the lines they refer to.

Template.

And agree to disagree. ;)

> I'm not a fan of the bracket-less single line if statements; I believe the common formatting for this codebase is to include them regardless of the number of lines

Agree, but it's the template - this could be a good source of good first/next bugs. :P

> Why "+ 1"?

We're adding a space in the drawable state array for the state we're just about to merge, iirc.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Tab thumbnail pressed color

(In reply to Anthony Lam (:antlam) from comment #20)
> > Anthony, what should the color of the tabs thumbnail background be when in
> > private mode and it's pressed?
> 
> I think we can keep it the same for now and see how that looks.

To be explicit, this is the pressed color - is this what you want to keep the same?

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Going to build on bug 1187538 to avoid the ThemedFrameLayout and just use the fact that all of these wrappers extend RelativeLayout.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638850 [details]
MozReview Request: Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Bug 1177611 - Make tab indicator purple in private tab. r=mhaigh

Comment 31

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #27)
> Created attachment 8639632 [details]
> Tab thumbnail pressed color
> 
> (In reply to Anthony Lam (:antlam) from comment #20)
> > > Anthony, what should the color of the tabs thumbnail background be when in
> > > private mode and it's pressed?
> > 
> > I think we can keep it the same for now and see how that looks.
> 
> To be explicit, this is the pressed color - is this what you want to keep
> the same?

Ah, no sorry. I thought you meant something else. 

Can we apply the same overlay to our new purple outline boxes?

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #31)
> Can we apply the same overlay to our new purple outline boxes?

What overlay? The pressed state is a color in this case, not an alpha value.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #32)
> What overlay? The pressed state is a color in this case, not an alpha value.

via IRC we noticed the pressed state of the non-private thumbnails is actually an alpha value (#BF) over the non-pressed color - we're going to try using the same alpha value on private tabs.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Copy tabs_panel_indicator -> tabs_strip_indicator. r=mhaigh

Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Martyn, the tab indicator color is not overridden in versions less than Lollipop - any ideas?

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #35)
> Martyn, the tab indicator color is not overridden in versions less than
> Lollipop - any ideas?

Nothing that I can find is overridden or changed by different resource configurations.

Comment 37

[Martyn Haigh (:mhaigh)]

Comment on attachment 8640229 [details]
MozReview Request: Bug 1177611 - Copy tabs_panel_indicator -> tabs_strip_indicator. r=mhaigh

https://reviewboard.mozilla.org/r/14299/#review13163

Ship It!

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638850 [details]
MozReview Request: Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8640229 [details]
MozReview Request: Bug 1177611 - Copy tabs_panel_indicator -> tabs_strip_indicator. r=mhaigh

Bug 1177611 - Copy tabs_panel_indicator -> tabs_strip_indicator. r=mhaigh

The next commits will modify tabs_panel_indicator so that unrelated views will
change.

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Set tabs panel indicator to purple when private. r=mhaigh

Bug 1177611 - Set tabs panel indicator to purple when private. r=mhaigh

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177611 - Move non-private browsing indicator to static drawables. r=mhaigh

Bug 1177611 - Move non-private browsing indicator to static drawables. r=mhaigh

This is not strictly necessary, but is a step towards removing the
tab_indicator_selected*.png assets (bug 1192048).

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Found a much easier approach. :)

Patches should be final here, Martyn.

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(42 w/ tracking protection)

Comment 45

[Martyn Haigh (:mhaigh)]

Comment on attachment 8644640 [details]
MozReview Request: Bug 1177611 - Set tabs panel indicator to purple when private. r=mhaigh

https://reviewboard.mozilla.org/r/15271/#review13927

Ship It!

::: mobile/android/base/resources/values/dimens.xml:14
(Diff revision 1)
> +    <!-- This value is (browser_toolbar_height - 6dp). Note that this

Why 6dp?  Maybe link a bug for future reference

::: mobile/android/base/resources/values-large-v11/dimens.xml:11
(Diff revision 1)
> +    <!-- This value is (browser_toolbar_height - 6dp). Note that this

ditto

::: mobile/android/base/tabs/TabsPanel.java:143
(Diff revision 1)
> -        mTabWidget.addTab(R.drawable.tabs_private, R.string.tabs_private);
> +        ((ThemedImageButton) mTabWidget.addTab(R.drawable.tabs_private, R.string.tabs_private)).setPrivateMode(true);

Currently this has two distinct functions of add tab and set tab to private - I'd prefer to split this in to two lines to increase readability.

::: mobile/android/base/widget/IconTabWidget.java:41
(Diff revision 1)
> -    public void addTab(int imageResId, int stringResId) {
> +    public View addTab(int imageResId, int stringResId) {

Lets make these arguments final whilst we're touching this line.

Comment 46

[Martyn Haigh (:mhaigh)]

Comment on attachment 8644641 [details]
MozReview Request: Bug 1177611 - Move non-private browsing indicator to static drawables. r=mhaigh

https://reviewboard.mozilla.org/r/15273/#review13933

Ship It!

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/15271/#review13927

> Why 6dp?  Maybe link a bug for future reference

6dp is the height of indicator - I added a comment.

Comment 48

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

url:        https://hg.mozilla.org/integration/fx-team/rev/807faeb41e1509d97f03c670da514ea5debac6dd
changeset:  807faeb41e1509d97f03c670da514ea5debac6dd
user:       Michael Comella <[e-mail]>
date:       Fri Jul 24 17:00:40 2015 -0700
description:
Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

url:        https://hg.mozilla.org/integration/fx-team/rev/5e6d9c0b3e4ec961eea492b6340cbcaa48d68257
changeset:  5e6d9c0b3e4ec961eea492b6340cbcaa48d68257
user:       Michael Comella <[e-mail]>
date:       Tue Jul 28 16:49:42 2015 -0700
description:
Bug 1177611 - Update tabs tray pressed pb color. r=mhaigh

url:        https://hg.mozilla.org/integration/fx-team/rev/bc6afa12b6dbb858a804f09a4b96986fe64da8e5
changeset:  bc6afa12b6dbb858a804f09a4b96986fe64da8e5
user:       Michael Comella <[e-mail]>
date:       Thu Aug 06 15:10:58 2015 -0700
description:
Bug 1177611 - Copy tabs_panel_indicator -> tabs_strip_indicator. r=mhaigh

The next commits will modify tabs_panel_indicator so that unrelated views will
change.

url:        https://hg.mozilla.org/integration/fx-team/rev/6d44161d2bdfd4b4036b69987729944c61eb66cd
changeset:  6d44161d2bdfd4b4036b69987729944c61eb66cd
user:       Michael Comella <[e-mail]>
date:       Thu Aug 06 15:23:28 2015 -0700
description:
Bug 1177611 - Set tabs panel indicator to purple when private. r=mhaigh

url:        https://hg.mozilla.org/integration/fx-team/rev/3536377b0dcb5f3d62d37440efdf8c188dd57e43
changeset:  3536377b0dcb5f3d62d37440efdf8c188dd57e43
user:       Michael Comella <[e-mail]>
date:       Thu Aug 06 15:31:00 2015 -0700
description:
Bug 1177611 - Move non-private browsing indicator to static drawables. r=mhaigh

This is not strictly necessary, but is a step towards removing the
tab_indicator_selected*.png assets (bug 1192048).

Comment 49

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

These are all "Ship it" on reviewboard.

Comment 50

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

This request applies to all changesets that were pushed.

Approval Request Comment
[Feature/regressing bug #]: Tracking protection initiative in 42
[User impact if declined]:
  Users will have an orange-themed tab tray, which is inconsistent with the purple theme we generally use for private browsing (i.e. polish).

[Describe test coverage new/current, TreeHerder]: Tested locally
[Risks and why]: These are small changes but there are a lot of them so it's possible I missed some edge case. In particular, with Android fragmentation, it's possible my changes don't work on some configuration of Android (I tested API 5+ and < 5 on tablet and phone). However, my changes are largely UI so the consequences, outside of NullPointerException and similar crashes, are low (inconsistent experience)

[String/UUID change made/needed]: None

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/807faeb41e15
https://hg.mozilla.org/mozilla-central/rev/5e6d9c0b3e4e
https://hg.mozilla.org/mozilla-central/rev/bc6afa12b6db
https://hg.mozilla.org/mozilla-central/rev/6d44161d2bdf
https://hg.mozilla.org/mozilla-central/rev/3536377b0dcb

Comment 52

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-08-17-11-43-22.png

Verified as fixed using:
Device: LG Nexus 4 (Android 5.0)
Build: Firefox for Android 43.0a1 (2015-08-17)

Comment 53

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8638849 [details]
MozReview Request: Bug 1177611 - Change private browsing thumbnail highlight to @color/private_browsing_purple. r=mhaigh

Tracking protection is an important feature of 42, taking this improvement fix.

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70da6494624c
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7520ca1b034
https://hg.mozilla.org/releases/mozilla-aurora/rev/15c6bb581166
https://hg.mozilla.org/releases/mozilla-aurora/rev/024d7a27df7e
https://hg.mozilla.org/releases/mozilla-aurora/rev/1afdf536cb3b