1220315 - Set a homepage preference has the wrong theme on Lollipop+

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: device-2015-10-30-151822.png

accentColor is not used.

Unclear if this is related to theme-appcompat changes.

Comment 1

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

so. many. regressions.

Comment 2

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

Set homepage is set behind a Nightly flag – blocking that bug and not prioritizing it for now.

Comment 3

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

GeckoPreferences uses the system theme so this is only a problem on Lollipop+ where accentColor is used.

Comment 4

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

It looks like we don't extend `android.support.v7.preference.DialogPreference` because the v7 prefs library is not included in our builds. However, DialogPreference is pretty explicit to say we should extend AppCompatActivity, which GeckoPreferences does not (bug 1205124). But then again, we don't do this for GeckoApp yet either (bug 1220309).

Let's try it out!

An alternative quick fix: set the style of the dialog separately using built-in styles, not AppCompat.

Comment 5

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

We uplifted this and may want to uplift this fix, in which case, perhaps the quick fix is more appropriate.

Comment 6

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

Motivation for my approach with the upcoming patch:

11:57 <sebastian> mcomella: Did you try just duplicating accentColor? Having it with and without prefix?
11:58 <mcomella> sebastian: Yeah – I started to look into where the style comes from... There's no theme set when creating the dialog, so it uses the default styles http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/preference/DialogPreference.java#281
11:58 <mcomella> sebastian: But there's this: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/preference/DialogPreference.java#281
11:58 <mcomella> sebastian: Which we never declare ever, afaict
11:58 <mcomella> sebastian: So I'm trying that out

Comment 7

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

Attachment: Screenshot post patch

Comment 8

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

Attachment: MozReview Request: Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8694946 [details]
MozReview Request: Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

https://reviewboard.mozilla.org/r/26941/#review24609

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/c17fc2bbaaff6784b836eb03caeee7d81356f5cd
Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

Comment 11

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

I also found that the Site Settings dialog's theme (which also uses an AlertDialog) is incorrect before this patch (i.e. has some default colors) and is fixed after this patch.

Comment 12

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

Comment on attachment 8694946 [details]
MozReview Request: Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

Approval Request Comment
[Feature/regressing bug #]: bug 1227322
[User impact if declined]: Users will see the homepage preference dialog with the wrong theme, which basically is the rest of preferences has our Firefox orange accent color but this dialog has a green accent color.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low – we started using a previously unused attribute, alertDialogTheme, and set it to get the dialog to appear as expected. Things look correct in this dialog (and the site settings dialog) but it's not guaranteed we didn't change the style of another alert dialog. We only set the attribute on API 21+ devices, so the potential negative effects are limited.

[String/UUID change made/needed]: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c17fc2bbaaff

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8694946 [details]
MozReview Request: Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian

This has been in Nightly for a few days, seems safe to uplift to Aurora44.

Comment 15

[Carsten Book [:Tomcat]]

has problems uplifting to aurora:

grafting 318523:c17fc2bbaaff "Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian"
merging mobile/android/base/resources/values-v21/themes.xml
warning: conflicts while merging mobile/android/base/resources/values-v21/themes.xml! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 16

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

Attachment: 44 (aurora) branch patch

I didn't have time to compile this but this should be the fix.

Comment 17

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

NI Tomcat to land before merge.

Comment 18

[Carsten Book [:Tomcat]]

seems there are still conflicts adding 1220315 to series file
renamed 1220315 -> file_1220315.txt
applying file_1220315.txt
patching file mobile/android/base/resources/values-v21/themes.xml
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values-v21/themes.xml.rej
patch failed, unable to continue (try -v)

Comment 19

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

Attachment: 44 (beta) branch patch

Looks like my exported patch had ^M EOL characters, causing the patch to not apply properly – I'm not sure why that is.

Comment 20

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

We're aiming for 44 here.

Comment 21

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

Comment on attachment 8698117 [details] [diff] [review]
44 (beta) branch patch

Missed merge so let's get this into 44.

Approval Request Comment – see comment 12

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: Nexus 5 (Android 5.1)
Builds: Firefox for Android 45.0a2 and 46.0a1 (2015-12-22)

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8698117 [details] [diff] [review]
44 (beta) branch patch

Fix was verified, Beta44+

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b7c348a34b3d

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b7c348a34b3d

Comment 26

[Carsten Book [:Tomcat]]

its on beta now :)

Comment 27

[Flaviu Cos]

Verified as fixed in Firefox 44  Beta 8;
Device: Asus ZenPad 8 (Android 5.0.2).