Closed Bug 1077755 Opened 5 years ago Closed 5 years ago

Implement new tablet menu bar pressed/focused private browsing button color

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files, 4 obsolete files)

Followup because we need to make these items `Themed*` and it's a pain.
via bug 1070087:

(In reply to Anthony Lam (:antlam) from comment #30)
> Comment on attachment 8499908 [details]
> Private browsing, tapped button, smaller visual
> 
> Can't find where I left this value but if it's not too much trouble could we
> do #222222 for V1 visual feedback on-press? For focused (with non-touch
> devices) we could use #363B40 for now (same as the background).
Good idea! I also found where I left that value, just a couple comments above in that same bug :P
Bug 1077195 will determine the final colors for private browsing.
Depends on: 1077195
I hate to complicate the generated code any more but oh well.
Attachment #8502899 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8502899 [details] [diff] [review]
Change pressed/focused private browsing colors of new tablet menu items

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

Looks good. I'm just not convinced that STYLE_CONSTRUCTOR is necessary.

::: mobile/android/base/widget/ThemedView.java.frag
@@ +30,5 @@
>      private boolean mIsDark;
>      private boolean mAutoUpdateTheme = true;
>  
>      public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs) {
> +#ifndef STYLE_CONSTRUCTOR

I see no reason to make this constructor optional. Does it cause any build errors if you simply added it for all ThemedViews?
Attachment #8502899 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> I see no reason to make this constructor optional. Does it cause any build
> errors if you simply added it for all ThemedViews?

While I haven't tried it, LinearLayout's three param constructor requires API 11 [1] and one of the other Views (I don't remember off-hand) does not work either.

I could simplify it by always having the three argument constructor, but calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others – I'll do that, unless you disagree.

[1]: https://developer.android.com/reference/android/widget/LinearLayout.html#LinearLayout(android.content.Context, android.util.AttributeSet, int)
(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I see no reason to make this constructor optional. Does it cause any build
> > errors if you simply added it for all ThemedViews?
> 
> While I haven't tried it, LinearLayout's three param constructor requires
> API 11 [1] and one of the other Views (I don't remember off-hand) does not
> work either.
> 
> I could simplify it by always having the three argument constructor, but
> calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others
> – I'll do that, unless you disagree.

Works for me.
Attachment #8502899 - Attachment is obsolete: true
Backed out in comment 11 for Fx-Team bustage (https://tbpl.mozilla.org/?tree=Fx-Team&rev=fa1f219e3eba).

bug 1075531 was also backed out, but I don't think that caused the issue.
Duplicate of this bug: 1081267
It seems the issue is that we weren't calling super(arg1, arg2) for the two argument constructors, which defines a default style for each class such as "com.android.internal.R.attr.imageButtonStyle".
Attachment #8504135 - Flags: review?(lucasr.at.mozilla)
Note that if we wanted to be extra safe, we could avoid making changes in unnecessary places (i.e. everywhere but ThemedImageButton).
Comment on attachment 8504135 [details] [diff] [review]
Part 2: Remove style constructor from unnecessary generated views

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

::: mobile/android/base/widget/ThemedView.java.frag
@@ +41,5 @@
>          a.recycle();
>      }
>  
> +#ifdef STYLE_CONSTRUCTOR
> +    public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs, int defStyle) {

The code duplication in these constructors is a bit unfortunate. No good way to avoid it? I don't feel so strongly about this because this is just a template anyway.
Attachment #8504135 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #17)
> The code duplication in these constructors is a bit unfortunate. No good way
> to avoid it?

One way is to look up the default styles by hand and include them in each template file. However, this duplicates the original source code and code face problems with new versions of Android.

We could also remove the `final` modifiers on some of these variables and call out to a shared method, at the cost of that type safety.

I'd be down to implement the second if you prefer.
(In reply to Michael Comella (:mcomella) from comment #18)
> (In reply to Lucas Rocha (:lucasr) from comment #17)
> > The code duplication in these constructors is a bit unfortunate. No good way
> > to avoid it?
> 
> One way is to look up the default styles by hand and include them in each
> template file. However, this duplicates the original source code and code
> face problems with new versions of Android.
> 
> We could also remove the `final` modifiers on some of these variables and
> call out to a shared method, at the cost of that type safety.
> 
> I'd be down to implement the second if you prefer.

Yeah, that sounds like a good compromise (removing the final modifiers) given that this is generated code that is unlikely to change often.
Called into initialize method, as recommended.
Attachment #8504135 - Attachment is obsolete: true
Comment on attachment 8502900 [details]
Private browsing, tapped button

Looks very subtle but I'm ok with this as a first iteration (kinda like how it goes with the whole dark theme)... 

We can refine as we go :) looks good!
Attachment #8502900 - Flags: feedback?(alam) → feedback+
Attachment #8503332 - Attachment is obsolete: true
Attachment #8505073 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/43ae53dc6394
https://hg.mozilla.org/mozilla-central/rev/73da2cbb36a5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.