Closed Bug 1220315 Opened 8 years ago Closed 7 years ago

Set a homepage preference has the wrong theme on Lollipop+


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox44 verified, firefox45 verified, firefox46 verified, b2g-v2.5 fixed, fennec44+)

Firefox 45
Tracking Status
firefox44 --- verified
firefox45 --- verified
firefox46 --- verified
b2g-v2.5 --- fixed
fennec 44+ ---


(Reporter: mcomella, Assigned: mcomella)




(4 files, 1 obsolete file)

accentColor is not used.

Unclear if this is related to theme-appcompat changes.
so. many. regressions.
Set homepage is set behind a Nightly flag – blocking that bug and not prioritizing it for now.
GeckoPreferences uses the system theme so this is only a problem on Lollipop+ where accentColor is used.
Summary: Set a homepage preference has the wrong theme → Set a homepage preference has the wrong theme on Lollipop+
It looks like we don't extend `` 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.
We uplifted this and may want to uplift this fix, in which case, perhaps the quick fix is more appropriate.
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
11:58 <mcomella> sebastian: But there's this:
11:58 <mcomella> sebastian: Which we never declare ever, afaict
11:58 <mcomella> sebastian: So I'm trying that out
Assignee: nobody → michael.l.comella
Comment on attachment 8694946 [details]
MozReview Request: Bug 1220315 - Use alertDialogTheme on v21+ to add accentColor. r=sebastian
Attachment #8694946 - Flags: review?(s.kaspari) → review+
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 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
Attachment #8694946 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Attachment #8694946 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Flags: needinfo?(michael.l.comella)
Attached patch 44 (aurora) branch patch (obsolete) — Splinter Review
I didn't have time to compile this but this should be the fix.
Flags: needinfo?(michael.l.comella)
NI Tomcat to land before merge.
Flags: needinfo?(cbook)
Attachment #8697661 - Attachment is patch: true
Flags: needinfo?(cbook)
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)
Flags: needinfo?(michael.l.comella)
Looks like my exported patch had ^M EOL characters, causing the patch to not apply properly – I'm not sure why that is.
Attachment #8697661 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella) → needinfo?(cbook)
We're aiming for 44 here.
tracking-fennec: --- → 44+
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
Attachment #8698117 - Flags: approval-mozilla-beta?
Attachment #8698117 - Attachment description: 44 (aurora) branch patch → 44 (beta) branch patch
Verified as fixed using:
Device: Nexus 5 (Android 5.1)
Builds: Firefox for Android 45.0a2 and 46.0a1 (2015-12-22)
Comment on attachment 8698117 [details] [diff] [review]
44 (beta) branch patch

Fix was verified, Beta44+
Attachment #8698117 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
its on beta now :)
Flags: needinfo?(cbook)
Verified as fixed in Firefox 44  Beta 8;
Device: Asus ZenPad 8 (Android 5.0.2).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.