1214940 - Ensure OverlayActivity's android: theme attributes are correct

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

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

We use android:colorPrimary & android:statusBarColor, but since we switched over to appCompat, these should not have android: prefixes. In the past, attributes with android: prefixes were ignored so double-check what is the correct approach here. Check all API levels.

Comment 1

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

But ShareDialog does not extend AppCompatActivity (I filed bug 1214956 for this), so I'm not sure of the implications.

Sebastian, do you know what happens if a non-AppCompatActivity uses the Theme.AppCompat styles?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #1)
> But ShareDialog does not extend AppCompatActivity (I filed bug 1214956 for
> this), so I'm not sure of the implications.

Dialogs are nasty. I had a bunch of problems with styling them in the past. I just saw that they now have a variant of AlertDialog (android.support.v7.app) and AppCompatDialog in AppCompat. This might help styling dialogs. I just see that ShareDialog is actually an Activity but maybe this is helpful with all the other bugs you filed about theme stuff.

> Sebastian, do you know what happens if a non-AppCompatActivity uses the
> Theme.AppCompat styles?

I've read that Theme.AppCompat should relay things like primaryColor to the prefixed version. So on Android 5+ a bunch of things should still use the colors. But briefly looking at the code of AppCompatActivity[1] I see that they "install" a custom LayoutInflater. I did not go deeper but I assume they are manually fixing or replacing some views there?

[1] https://github.com/android/platform_frameworks_support/blob/master/v7/appcompat/src/android/support/v7/app/AppCompatActivity.java

Comment 3

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

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> But briefly looking at the code of AppCompatActivity[1] I see that
> they "install" a custom LayoutInflater. I did not go deeper but I assume
> they are manually fixing or replacing some views there?

I read a post once that they automatically replace the standard views (e.g. TextView) with their AppCompat versions (e.g. AppCompatTextView) so you can use the non-prefixed versions.

As for this bug, I've been finding sometimes the prefixed versions of attributes work and sometimes they don't, depending on app level. Rather than spend time auditing the attributes here and finding out some shouldn't have prefixes and some should, I'm going to go with the, "If it works, then don't fix it" approach – we can clean these up further when we unify the UI to one-ish style.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #3)
> I'm going to go with the, "If it works, then
> don't fix it" approach – we can clean these up further when we unify the UI
> to one-ish style.

Yeah, I agree. We could even duplicate styles (with and without prefixes) if this fixes the issues until we have really unified the themes and moved to AppCompat components everywhere.