Font in share overlay is unexpectedly bolditalic on 4.4

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
Last year

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks 1 bug)

unspecified
Firefox 39
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Posted image Screenshot
Apparently Android L sets sane defaults and previous versions do not.

Because of the Android inconsistency, there's some value in having a default TextAppearance for our app.
/r/6171 - Bug 1148041 - Have the ShareOverlay text styles inherit from the default TextAppearance. r=liuche

Pull down this commit:

hg pull review -r 555666d29be13d76bda4ba9f4e22f318d38ef9a2
Attachment #8583993 - Flags: review?(liuche)
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

https://reviewboard.mozilla.org/r/6169/#review5169
Attachment #8583993 - Flags: review?(liuche) → review+
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

Approval Request Comment
[Feature/regressing bug #]: bug 1130203
[User impact if declined]:
  Users on pre-Android L devices will get share dialogs with strange inconsistent font styles.

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
  We change the text styles used in the share overlay to inherit from our default text styles. This may include some attributes that style our text in unexpected ways in edge cases (e.g. text is highlighted, but text can't be highlighted normally in these views), but these edge cases are unexpected.

[String/UUID change made/needed]: None
Attachment #8583993 - Flags: approval-mozilla-aurora?
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

This breaks 2.3.

Investigating...
Attachment #8583993 - Flags: approval-mozilla-aurora?
(In reply to Michael Comella (:mcomella) from comment #6)
> Investigating...

Stack:

  Caused by: android.content.res.Resources$NotFoundException: Resource is not a ColorStateList (color or path): TypedValue{t=0x2/d=0x101009b a=2}

With liuche's help, we determined the problem line [1]:

  <item name="android:textColorHint">?android:attr/textColorHint</item>

Following the textColorHint trail, there doesn't appear to be any problems. In Gecko theme [2]:

  <item name="android:textColorHint">@color/text_color_hint</item>

And values/colors.xml [3]:

  <color name="text_color_hint">#666666</color>

I'm not sure what's going on. Hack fix: perhaps overriding the color will fix the issue here.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml?rev=4000ac847fc7#295
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/themes.xml?rev=75204e00b8cd#47
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/colors.xml?rev=6757510c20c0#88
(In reply to Michael Comella (:mcomella) from comment #7)
>   <item name="android:textColorHint">?android:attr/textColorHint</item>

To be clear, we determined it was the problem line by changing the color and not experiencing a crash.
https://hg.mozilla.org/mozilla-central/rev/37c1fdfd86ba
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
It seems the issue may have been that the default attributes we were referecing are private [1]. This isn't an issue in the browser because we override the default attributes.

The solution is to inherit from the Gecko theme in the ShareOverlayActivity. Working on it...

[1]: http://stackoverflow.com/a/10869421
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave-open]
Comment on attachment 8584194 [details] [diff] [review]
Inherit from Gecko theme in share overlay

Review of attachment 8584194 [details] [diff] [review]:
-----------------------------------------------------------------

That makes sense - ShareOverlay shows up within FxA, so the theming should be consistent.
Attachment #8584194 - Flags: review?(liuche) → review+
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

Approval Request Comment
(see comment 5)
Attachment #8583993 - Flags: approval-mozilla-aurora?
Comment on attachment 8584194 [details] [diff] [review]
Inherit from Gecko theme in share overlay

Please uplift the reviewboard review, then this patch (which fixes that patch).

Approval Request Comment
[Feature/regressing bug #]: This one
[User impact if declined]:
  If the other patch is uplifted, crashes on 2.3.

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
  We inherit from the base theme in the rest of the app (which we weren't doing before). This may override some Android defaults and we may get some strangely styled UI. However, I see nothing wrong with all of the interactions that I've made with the UI so I doubt anything will happen.

[String/UUID change made/needed]: None
Attachment #8584194 - Flags: approval-mozilla-aurora?
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/ed5addaca0c1
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment #8584194 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8583993 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

This missed the uplift to Aurora. It'll need to go through the Beta approval process.
Attachment #8583993 - Flags: approval-mozilla-aurora+
Attachment #8584194 - Flags: approval-mozilla-aurora+
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

Approval Request Comment 5
Attachment #8583993 - Flags: approval-mozilla-beta?
Attachment #8583993 - Flags: approval-mozilla-aurora?
Comment on attachment 8584194 [details] [diff] [review]
Inherit from Gecko theme in share overlay

Approval Request Comment 15
Attachment #8584194 - Flags: approval-mozilla-beta?
Attachment #8584194 - Flags: approval-mozilla-aurora?
Attachment #8584194 - Flags: approval-mozilla-aurora?
Attachment #8583993 - Flags: approval-mozilla-aurora?
Verified fixed on latest Nightly 40.0a1 and Aurora 39.0a1 on Nexus 5 with Android 5.0.2
Status: RESOLVED → VERIFIED
Comment on attachment 8583993 [details]
MozReview Request: bz://1148041/mcomella

Should be in 38 beta 2
Attachment #8583993 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8584194 [details] [diff] [review]
Inherit from Gecko theme in share overlay

Should be in 38 beta 2
Attachment #8584194 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I didn't make it for beta 2, probably beta 4
Verified fixed on Firefox 38 Beta 3 on Nexus 5 (Android 5.1)
No longer depends on: 1152337
Attachment #8583993 - Attachment is obsolete: true
Attachment #8619887 - Flags: review+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.