1201206 - (theme-appcompat) Application styles should inherit from Theme.AppCompat

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: DialogFragment referenced in comment 0

Or it's descendants (e.g. Theme.AppCompat.Light).

Why do I think this? As part of bug 1173147, I extended support.v4.app.DialogFragment and receive the following error when onCreateDialog is run:
  java.lang.IllegalStateException: You need to use a Theme.AppCompat theme (or descendant) with this activity.

This crashes at `return AlertDialog.Builder.create()`, which takes in an Activity (that I assume to be BrowserApp) in the previously called constructor. I attached my code for reference.

The error implies to me the activity should be using Theme.AppCompat. After from googling, the internet seems to agree that if you're using a support library activity (ActionBarActivity comes up a lot), you should use Theme.AppCompat.

I switched GeckoBase to inherit from @style/Theme.AppCompat.Light and it worked correctly. Note that on v21 (i.e. my test device), we currently inherit from @android:style/Theme.Material.Light.DarkActionBar".

Martyn, Sebastian – do you have any experience with this?

Comment 1

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

And more importantly, do you agree? (before I go and potentially destroy the world by inheriting from a different base style)

Comment 2

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

Agree that the styles need to inherit from Theme.AppCompat, that is.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

This pushed me to finally write to the mailing list about themes and our different UIs:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-September/001460.html

I like the idea to inherit from Theme.AppCompat very much! It will allow us to get rid of maintaining a variety of themes and use one theme for all Android versions.

However I expect more work will result from that, some random first thoughts:
* All themes will need to inherit from that. Effectively we will only have one theme after that. This is awesome but I assume we will need to fix a bunch of our styles that currently inherit from specific Android styles. But this could be the final goal and we can ship intermediate steps where we mix AppCompat and system styles.
* Look how our settings and the sync activities are affected by changes. They seem to react more to it than our browser activity that has most things overriden.
* For overriding properties in the appcompat theme we need to drop the "android" namespace, e.g. use "colorPrimary" instead of "android:colorPrimary". If you don't do that then these styles might not be applied on every Android version.
* More things I can't think of currently.

Again: I really like the idea and would love to see this as first steps to have a unified theme for Firefox.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> This pushed me to finally write to the mailing list about themes and our
> different UIs:
> https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-September/001460.
> html
> 
> I like the idea to inherit from Theme.AppCompat very much! It will allow us
> to get rid of maintaining a variety of themes and use one theme for all
> Android versions.
> 
> However I expect more work will result from that, some random first thoughts:
> * All themes will need to inherit from that. Effectively we will only have
> one theme after that. This is awesome but I assume we will need to fix a
> bunch of our styles that currently inherit from specific Android styles. But
> this could be the final goal and we can ship intermediate steps where we mix
> AppCompat and system styles.
> * Look how our settings and the sync activities are affected by changes.
> They seem to react more to it than our browser activity that has most things
> overriden.

Fennec preferences are a special snowflake, and I can't speak to them.

Be aware that most of the Old Sync activities will disappear entirely (Bug 957845, perhaps) and that most of the Firefox Account based activities will also go away (Bug 1161234).  The only survivor will be FxAccountStatusActivity, which is itself a special snowflake.  The motivations for keeping that a separate Activity, outside of the rest of Fennec's preferences, have largely disappeared, now that we have deprecated the android-sync repository.  In fact, as the owner of that code, I would be thrilled to normalize into the rest of the Fennec standards, so that it's less of a snowflake and something that others feel comfortable hacking on.

Comment 5

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

Nick, it's been difficult for me to test the old sync stuff so I'd like for the old sync activities/classes to die before we start this effort – do you know when this will be?

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> This pushed me to finally write to the mailing list about themes and our
> different UIs:

Yessssss!

> This is awesome but I assume we will need to fix a
> bunch of our styles that currently inherit from specific Android styles.

Good point.

> * For overriding properties in the appcompat theme we need to drop the
> "android" namespace, e.g. use "colorPrimary" instead of
> "android:colorPrimary". If you don't do that then these styles might not be
> applied on every Android version.

Sebastian, shouldn't the `app` namespace be used here? (e.g. http://stackoverflow.com/a/26692768) Is excluding a namespace the same as having none?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #5)
> Nick, it's been difficult for me to test the old sync stuff so I'd like for
> the old sync activities/classes to die before we start this effort – do you
> know when this will be?

The kill switch for the Mozilla Old Sync server cluster is September 30, 2015.  (One month.)  The code has been deprecated for a long time -- more than a year -- so I wouldn't even bother trying to not break it.

Comment 7

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

As Sebastian mentioned in comment 3, the changes in theme may be incremental but the end goal here, as per Sebastian's "unify the UI" email [1], is to ship a single theme (e.g. Material) to all Android versions.

[1]: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-September/001460.html

Comment 8

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

Attachment: MozReview Request: Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

Comment 9

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

Attachment: MozReview Request: Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

Bug 1201206 - Restore v21+ action bar style. r=mhaigh

Comment 10

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

Attachment: MozReview Request: Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

Bug 1201206 - Correct v11+ preferences activity text color. r=mhaigh

I was unable to correct the color of the black Material checkboxes. It is my
impression that AppCompat-users need to extend ActionBarActivity (which can use
PreferenceFragment) to make full use of the features, meaning GeckoPreferences
needs to extend ActionBarActivity (which is too much scope-creeping here).

For more info:
* The new checkboxes that are tint capable are introduced in:
http://android-developers.blogspot.com/2014/10/appcompat-v21-material-design-for-pre.html
* The docs indicate you can change this dynamically or via xml attr:
https://developer.android.com/reference/android/support/v7/widget/AppCompatCheckBox.html
* This post indicates all that is needed is colorAccent (AppCompatCheckBox
should be used automatically, according to other docs):
http://stackoverflow.com/a/31110775
* This post agrees with the above post: http://stackoverflow.com/a/26850668

Comment 11

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

Attachment: MozReview Request: Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

One fear is that different devices set different menu colors and text colors.
Since we're using the default text color and set an explicit menu color, the
text color may not look good on these devices. I was unable to find a way to
override the menu text color.

It seems the best way to find out if this is a problem is to land
it and test though!

Comment 12

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

(In reply to Michael Comella (:mcomella) from comment #5)
> Sebastian, shouldn't the `app` namespace be used here? (e.g.
> http://stackoverflow.com/a/26692768) Is excluding a namespace the same as
> having none?

I think in older versions of the support library, "app" was needed, but now none is needed.

Comment 13

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

Attachment: Settings, Honeycomb - KitKat

Anthony, a side effect of my recent change (which will allow us to eventually unify the UI and properly fix bug 1173147) is that the checkboxes on Honeycomb - KitKat now have the Material style but in the color black. It'd take a lot of work to change the color so I'd like to leave it as is. Is that okay?

Comment 14

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661537 [details]
MozReview Request: Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

https://reviewboard.mozilla.org/r/19393/#review17935

Comment 15

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661538 [details]
MozReview Request: Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

https://reviewboard.mozilla.org/r/19395/#review17937

Comment 16

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661539 [details]
MozReview Request: Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

https://reviewboard.mozilla.org/r/19397/#review17939

It's all a bit confusing, isn't it!?

Comment 17

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661540 [details]
MozReview Request: Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

https://reviewboard.mozilla.org/r/19399/#review17941

Comment 18

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

(In reply to Michael Comella (:mcomella) from comment #13)
> Anthony, a side effect of my recent change (which will allow us to
> eventually unify the UI and properly fix bug 1173147) is that the checkboxes
> on Honeycomb - KitKat now have the Material style but in the color black.
> It'd take a lot of work to change the color so I'd like to leave it as is.
> Is that okay?

We spoke on Vidyo – I'm going to try having the GeckoPreferences activity extend the original styles for now.

Comment 19

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

Comment on attachment 8661537 [details]
MozReview Request: Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

Comment 20

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

Comment on attachment 8661538 [details]
MozReview Request: Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

This excludes Material design in v21+, which will be overridden with AppCompat
in the following changeset.

Comment 21

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

Comment on attachment 8661539 [details]
MozReview Request: Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

Comment 22

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

Comment on attachment 8661540 [details]
MozReview Request: Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

One fear is that different devices set different menu colors and text colors.
Since we're using the default text color and set an explicit menu color, the
text color may not look good on these devices. I was unable to find a way to
override the menu text color.

It seems the best way to find out if this is a problem is to land
it and test though!

Comment 23

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661538 [details]
MozReview Request: Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

https://reviewboard.mozilla.org/r/19395/#review18965

Comment 24

[Martyn Haigh (:mhaigh)]

Comment on attachment 8661539 [details]
MozReview Request: Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

https://reviewboard.mozilla.org/r/19397/#review18967

Comment 25

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

Comment on attachment 8661541 [details]
Settings, Honeycomb - KitKat

I meant to obsolete this with the latest changesets.

Comment 26

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

https://hg.mozilla.org/integration/fx-team/rev/10aec9211e477521da35fa265777d8ecd732a18d
Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/1814d2ad6a95d274f961c1ef83c55933ca13cd87
Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/ba8934512019b0c17b1b6c977378588d7a98b973
Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/0da96f342a57ea9c4f2110740c05d606fe8efea6
Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/bcc6656955daf286360460a0c80f63d9179e23fe
Bug 1201206 - rebase - Remove android prefix from colorAccent. r=me

Comment 27

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

Backed out for android build bustage: https://hg.mozilla.org/integration/fx-team/rev/c09702241846


https://treeherder.mozilla.org/logviewer.html#?job_id=4949831&repo=fx-team

Comment 28

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

My brief skimming showed Geckoview version mismatching – Nick said he would help out. Last I heard:
  13:02 <nalexander> mcomella: ah, that'll be the issue -- I didn't update an ANDROID_SDK_ROOT, I think.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

I assume we do not add the AppCompat dependency to the standalone GeckoView build yet?

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #29)
> I assume we do not add the AppCompat dependency to the standalone GeckoView
> build yet?

Correct.  Specifically, we build embedding/android/geckoview_example using Ant, which has some support for dependent library projects: http://stackoverflow.com/a/18555084.  Frankly, I'd rather kill geckoview_example entirely than add this support.  I suggest we land this and set MOZ_DISABLE_GECKOVIEW=1 globally (see Bug 1174757, and others), before just removing the example.

If we strongly believe that we must keep geckoview_example building, then somebody else can hack the build system into doing this work: you should be able to use the $OBJDIR/exploded-aar directory for building the appcompat-v7 library using the Android tool, just like the StackOverflow answer suggests.

Flagging mfinkle and blassey for concurring opinions as to whether we can just kill this.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

1. I don't feel the GeckoView example blocks landing this change
2. What's a better way forward for the example? Using gradle instead of Ant?

Comment 32

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

(In reply to Mark Finkle (:mfinkle) from comment #31)
> 1. I don't feel the GeckoView example blocks landing this change

Filed bug 1212347.

> 2. What's a better way forward for the example? Using gradle instead of Ant?

NI nalexander.

Comment 33

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #31)
> 1. I don't feel the GeckoView example blocks landing this change
> 2. What's a better way forward for the example? Using gradle instead of Ant?

We need to evaluate what the GV example provides.  At one time it provided a check that the produced GeckoView could be compiled into an outside project; and it provided some (meager) documentation about the process.

Today, b2gdroid is building in automation, so it fills the automated compilation role.  And today, ~0 Ant based projects exist, so the documentation aspect is moot.  We have better artifacts (the .AAR files!) and different (still meager) documentation on how to integrate (https://github.com/ncalexan/geckobrowser-gradle), so neither of these are valuable.

In theory, GV example was to be used to test the interfaces; in reality that hasn't happened.  Were we to do that, I think Gradle will be the way forward:

1) split GeckoView and Fennec into separate Gradle projects;
2) mutate Robocop into Fennec instrumentation tests;
3) fold what was GV example into GeckoView instrumentation tests;
4) finally run both sets of instrumentation tests in automation.

Comment 34

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49318e2d179a

Comment 35

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

(Moved NI to bug 1212973)

Comment 36

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

https://hg.mozilla.org/integration/fx-team/rev/ebc006342c9e0654a2900e952299341251f01ad7
Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/bfc7e7f997eb2a4f5bbfea4e817aa4e738900d5b
Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/2d7962287928829803e68fe5cd1aeabd69003837
Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/c01f712e3d98c74a03f1dcf9c5133c0c8982d32d
Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

Comment 37

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

Backed out for Android x86 build bustage:
https://hg.mozilla.org/integration/fx-team/rev/1ffccf0304f3
https://treeherder.mozilla.org/logviewer.html#?job_id=5043326&repo=fx-team

Comment 38

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

Nick, know what's going on here?

Comment 39

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #38)
> Nick, know what's going on here?

No, it makes no sense.  Perhaps you got bitten by a clobber needed?  I doubt it though, since that x86 build is trying to build GV example.  But then I don't understand why your try push built...

Comment 40

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

https://hg.mozilla.org/integration/fx-team/rev/a34fe2bdd3abb1a34a1f35bafb1bc1ba5f5959b1
Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/e0077ffd54c562bc80e1883a9964a5b672a05b11
Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/de6a56d4bd23e266307e5054724934befa49ae47
Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh

https://hg.mozilla.org/integration/fx-team/rev/ed8188590f14b1aae2e4f44c8196994f375a99f4
Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

Comment 41

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

Forced a clobber in bug 1212347 comment 8 and pushed comment 40. /me crosses fingers

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a34fe2bdd3ab
https://hg.mozilla.org/mozilla-central/rev/e0077ffd54c5
https://hg.mozilla.org/mozilla-central/rev/de6a56d4bd23
https://hg.mozilla.org/mozilla-central/rev/ed8188590f14

Comment 43

[Dominique Vincent [:domivinc]]

Michael, on my device (HTC Android 2.3), using Nightly, I still see the black text with a black background in the Firefox vertical menu. It seems that the following changes [1] didn't fix the issue in all the 2.3 cases.

I tried to change my Android HTC theme, but it doesn't change the problem.

I can see the same issue in Aurora 44.0a2.

[1] https://hg.mozilla.org/integration/fx-team/rev/c01f712e3d98c74a03f1dcf9c5133c0c8982d32d

Comment 44

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

(In reply to Dominique Vincent [:domivinc] from comment #43)
> Michael, on my device (HTC Android 2.3), using Nightly, I still see the
> black text with a black background in the Firefox vertical menu. It seems
> that the following changes [1] didn't fix the issue in all the 2.3 cases.

This is unfortunate.

I think the only solution here is bug 1220309 which necessitates an older support library version (which can come with its own set of problems) or bug 1209967.

Comment 45

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

Meant to reopen bug 1215079.