Closed Bug 1201206 (theme-appcompat) Opened 4 years ago Closed 4 years ago

Application styles should inherit from Theme.AppCompat

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 3 open bugs)

Details

Attachments

(5 files, 1 obsolete file)

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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mhaigh)
And more importantly, do you agree? (before I go and potentially destroy the world by inheriting from a different base style)
Agree that the styles need to inherit from Theme.AppCompat, that is.
Assignee: nobody → michael.l.comella
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.
Flags: needinfo?(s.kaspari)
(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.
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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
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
Bug 1201206 - Inherit from Theme.AppCompat style at all API levels. r=mhaigh
Attachment #8661537 - Flags: review?(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
Attachment #8661539 - Flags: review?(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!
Attachment #8661540 - Flags: review?(mhaigh)
(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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mhaigh)
Attached image Settings, Honeycomb - KitKat (obsolete) —
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?
Attachment #8661541 - Flags: feedback?(alam)
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
Attachment #8661537 - Flags: review?(mhaigh) → review+
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
Attachment #8661538 - Flags: review?(mhaigh) → review+
Attachment #8661539 - Flags: review?(mhaigh) → review+
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 on attachment 8661540 [details]
MozReview Request: Bug 1201206 - Correct menu button background on 2.3. r=mhaigh

https://reviewboard.mozilla.org/r/19399/#review17941
Attachment #8661540 - Flags: review?(mhaigh) → review+
(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 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 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.
Attachment #8661538 - Attachment description: MozReview Request: Bug 1201206 - Restore v21+ action bar style. r=mhaigh → MozReview Request: Bug 1201206 - Inherit from version specific styles for prefs. r=mhaigh
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
Attachment #8661539 - Attachment description: MozReview Request: Bug 1201206 - Correct v11+ preferences activity text color. r=mhaigh → MozReview Request: Bug 1201206 - Inherit from appCompat on v21 prefs and restore action bar style. r=mhaigh
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!
Attachment #8661538 - Flags: review+ → review?(mhaigh)
Attachment #8661539 - Flags: review+ → review?(mhaigh)
Attachment #8661541 - Flags: feedback?(alam) → feedback-
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
Attachment #8661538 - Flags: review?(mhaigh) → review+
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
Attachment #8661539 - Flags: review?(mhaigh) → review+
Comment on attachment 8661541 [details]
Settings, Honeycomb - KitKat

I meant to obsolete this with the latest changesets.
Attachment #8661541 - Attachment is obsolete: true
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.
Flags: needinfo?(michael.l.comella) → needinfo?(nalexander)
I assume we do not add the AppCompat dependency to the standalone GeckoView build yet?
(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.
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(blassey.bugs)
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?
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
(Moved NI to bug 1212973)
Flags: needinfo?(blassey.bugs)
Nick, know what's going on here?
Flags: needinfo?(michael.l.comella) → needinfo?(nalexander)
(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...
Flags: needinfo?(nalexander)
Forced a clobber in bug 1212347 comment 8 and pushed comment 40. /me crosses fingers
Depends on: 1213480
Alias: theme-appcompat
Depends on: 1215079
Blocks: 1214956
No longer depends on: 1214956
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
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Meant to reopen bug 1215079.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.