1100439 - Tinted status bar not working in Android 5.0

Status: RESOLVED DUPLICATE of bug 1137240

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Screen Shot 2014-11-17 at 12.36.08 PM.png

I just realized that I don't think the tinted status bar is working as expected in Android 5.0, see screenshot.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Patch v1

I thought this was a nice starter bug for me to take. Attach is my first approach. I've added a theme override for v21 which sets the status bar to the correct color. And also a change to BrowserApp.setupSystemUITinting() to nodo special tinting if we are running on Lollipop or higher.

First patch so please let me know if I did something wrong in the process.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Would it make sense to create a PixelTest test for this?

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8526070 [details] [diff] [review]
Patch v1

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

Lucas worked on the status bar tinting, so he'd be a better reviewer.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8526070 [details] [diff] [review]
Patch v1

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

Looks good. Could you post a screenshot for antlam's review? Any side-effects from inheriting from Lollipop's theme?

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I will do some side by side comparison to see if there are side effect. Are there any places where you think this can possibly go wrong?

Do you want a unit test for this? I thought we could make a test that makes a screenshot and then checks the pixel color value in the status bar. Then we can see if the color is good on all versions of Android that we run our tests on.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Stefan Arentz [:st3fan] from comment #5)
> I will do some side by side comparison to see if there are side effect. Are
> there any places where you think this can possibly go wrong?

Nothing specific. Side-by-side should be fine.

> Do you want a unit test for this? I thought we could make a test that makes
> a screenshot and then checks the pixel color value in the status bar. Then
> we can see if the color is good on all versions of Android that we run our
> tests on.

You can but I don't feel strongly about it. Up to you.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This needs more work. The status bar looks good but things like the action bar and the popup menu (i dont know what the android terms are) have messed up styles.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Patch v2

I think this is a better approach. It turns out that it is really
difficult to inherit the correct style without undoing other styles
so this new patch simply does it in code.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: patch

Added a test. Cleaned up the code.

Comment 10

[Wesley Johnston (:wesj)]

I think I agree with Lucas. I like the first patch with the name="GeckoBase" set instead. Can we do that? We may have to chase down some regressions (I see that date-picker dialogs look a bit... odd, but neat and still shippable even in their current form). Everything else seemed pretty good to me.

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Wesley Johnston (:wesj) from comment #10)
> I think I agree with Lucas. I like the first patch with the name="GeckoBase"
> set instead. Can we do that?

I'm not sure. I tried to do this change completely in a style.xml but I was not able to inherit the Material theme and just set status bar color. Other style modifications that we do are alway disappearing no matter what I tried.

So it is completely possible that I don't fully understand how the style/theme inheritance works of course.

Comment 12

[Wesley Johnston (:wesj)]

Yeah. This works for me:

    <style name="GeckoBase" parent="@android:Theme.Material.Light.NoActionBar">
        <item name="android:statusBarColor">@color/background_tabs</item>
        <item name="android:windowNoTitle">true</item>
        <item name="android:windowContentOverlay">@null</item>
        <item name="arrowPopupWidth">match_parent</item>
    </style>

There's some issues with quick-share in web-context menus, but we can talk to UX about fixing that in a different bug. Other than that, I haven't seen much.

Comment 13

[Wesley Johnston (:wesj)]

Oh, there's a new time picker that looks a bit strange. Also a separate bug though.

Comment 14

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

Tracking 35 bug. Next steps?

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I tried the approach that :wesj suggests in comment #12. It works, but other UI elements lose their configured style. I think this is because other styles that have GeckoBase as their parent are invalidated in some way.

I am not sure if this change is actually possible to do with just themes.xml changes. This is why I decided to do it as a small code-only change.

Who knows best how the themes.xml inheritance works?

Comment 16

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

For the record, the SO solution:

https://stackoverflow.com/questions/26474125/android-4-4-translucent-status-and-navigation-bars-style-on-android-5-0

Comment 17

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

Now that we inherit from material, I tried applying Stefan's patch and just returning on API 21+ with the theme change (i.e. android:statusBarColor). However, I couldn't get this to work. Doing it all in code, like the 2nd patch, appears to work correctly.

Note that this fixes some fullscreen issues on Android L too (but introduces a graphical glitch over the status bar when transitioning into fullscreen mode - I'm assuming it's because we're not yet drawing content underneath).

Comment 18

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

This will be fixed by bug 1137240.