Closed Bug 1164123 Opened 5 years ago Closed 5 years ago

meta theme-color should match the gaia-header theme color when possible

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g-master fixed)

RESOLVED FIXED
FxOS-S1 (26Jun)
Tracking Status
b2g-master --- fixed

People

(Reporter: etienne, Assigned: etienne)

References

Details

Attachments

(4 files)

46 bytes, text/x-github-pull-request
gmarty
: review+
gaye
: review+
arcturus
: review+
gsvelto
: review+
dkuo
: review+
arthurcc
: review+
julienw
: review+
Details | Review
38 bytes, text/x-github-pull-request
hub
: review+
Details | Review
3.48 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
mikehenrty
: review+
Details | Review
which is in a var(--header-color).

I think the main issue is the theme groups (which are set by a class on the body) where we might need some JS.
Summary: theme-colors should match the gaia-header theme color when possible → meta theme-color should match the gaia-header theme color when possible
Priority: -- → P1
(In reply to Etienne Segonzac (:etienne) from comment #0)
> I think the main issue is the theme groups (which are set by a class on the
> body) where we might need some JS.

Hey Wilson, so basically we would like to put a 
    <meta name="theme-color" content="var(--header-color)">
at the top of apps, but --header-color depends on a body class (.theme-settings, .theme-communications, etc...).

Do you know if there's anything preventing us from moving this class to the <html> tag?
Flags: needinfo?(wilsonpage)
(In reply to Etienne Segonzac (:etienne) from comment #1)
> (In reply to Etienne Segonzac (:etienne) from comment #0)
> > I think the main issue is the theme groups (which are set by a class on the
> > body) where we might need some JS.
> 
> Hey Wilson, so basically we would like to put a 
>     <meta name="theme-color" content="var(--header-color)">
> at the top of apps, but --header-color depends on a body class
> (.theme-settings, .theme-communications, etc...).
> 
> Do you know if there's anything preventing us from moving this class to the
> <html> tag?

That should work fine :)
Flags: needinfo?(wilsonpage)
Will take a look this week.
Assignee: nobody → etienne
Attached file Gaia PR
Since the status bar lives in the system app and header colors can be different depending on the theme "group" I need to introduce the meta "theme-group".

Did the change for apps where it made the most sense, the result is pretty cool :)

Flagging you for the pièce de résistance Guillaume (the system part of the patch)!
Attachment #8616078 - Flags: review?(gsvelto)
Attachment #8616078 - Flags: review?(gmarty)
Attachment #8616078 - Flags: review?(gaye)
Attachment #8616078 - Flags: review?(francisco)
Attachment #8616078 - Flags: review?(felash)
Attachment #8616078 - Flags: review?(dkuo)
Attachment #8616078 - Flags: review?(arthur.chen)
Attached file Sudio PR
Studio needs to be a good citizen too :)
Attachment #8616079 - Flags: review?(hub)
Comment on attachment 8616078 [details] [review]
Gaia PR

+1 for the dialer bits
Attachment #8616078 - Flags: review?(gsvelto) → review+
Comment on attachment 8616079 [details] [review]
Sudio PR

lgtm
Attachment #8616079 - Flags: review?(hub) → review+
Comment on attachment 8616078 [details] [review]
Gaia PR

r=me for the SMS part
Attachment #8616078 - Flags: review?(felash) → review+
Attachment #8616078 - Flags: review?(francisco) → review+
Comment on attachment 8616078 [details] [review]
Gaia PR

Assuming these changes only apply on some specific apps, like Music, because I didn't see the other media apps such as Gallery or Video, the patch looks good to me.
Attachment #8616078 - Flags: review?(dkuo) → review+
Attachment #8616078 - Flags: review?(gaye) → review+
Attachment #8616078 - Flags: review?(arthur.chen) → review+
Comment on attachment 8616078 [details] [review]
Gaia PR

LGTM! Thanks.
Attachment #8616078 - Flags: review?(gmarty) → review+
(In reply to Dominic Kuo [:dkuo] from comment #10)
> Comment on attachment 8616078 [details] [review]
> Gaia PR
> 
> Assuming these changes only apply on some specific apps, like Music, because
> I didn't see the other media apps such as Gallery or Video, the patch looks
> good to me.

It does! Keeping the fullscreen apps out of the PR for now.

Thanks everyone!
Hey, since this patch touches the AppWindow the raptor oranges I'm seeing might be true failures.
Eli, any suggestion?
Flags: needinfo?(eperelman)
Flags: needinfo?(eperelman) → needinfo?(rwood)
(In reply to Etienne Segonzac (:etienne) from comment #13)
> Hey, since this patch touches the AppWindow the raptor oranges I'm seeing
> might be true failures.
> Eli, any suggestion?

Hi Etienne, after looking at the numbers and re-triggering the raptor jobs, it is safe to say this is just noise/variance. If it was an actual regression the tests would be failing consistently each time on each effected app. We will continue to work to reduce variance/modify the algorithm to prevent false alarms. Thanks for checking on them though, appreciated.
Flags: needinfo?(rwood)
(In reply to Robert Wood [:rwood] from comment #14)
> (In reply to Etienne Segonzac (:etienne) from comment #13)
> > Hey, since this patch touches the AppWindow the raptor oranges I'm seeing
> > might be true failures.
> > Eli, any suggestion?
> 
> Hi Etienne, after looking at the numbers and re-triggering the raptor jobs,
> it is safe to say this is just noise/variance. If it was an actual
> regression the tests would be failing consistently each time on each
> effected app. We will continue to work to reduce variance/modify the
> algorithm to prevent false alarms. Thanks for checking on them though,
> appreciated.

Thanks! Landing then.

https://github.com/mozilla-b2g/gaia/commit/830eb9552419fbf5ed743fb085c1cc82f0bcbdde
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1175160
Backed out the gaia part as it needs a gecko patch to work.
https://github.com/mozilla-b2g/gaia/pull/30626

(really sorry about that)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Studio is a project that keeps on giving :)
Attachment #8623757 - Flags: review?(fabrice)
Attachment #8623757 - Flags: review?(fabrice) → review+
checking-needed for the gecko part, that can safely land alone.
Keywords: checkin-needed
Attached file Gaia PR relanding
The only change from the previous version is an extensive system marionette test suite for the statusbar background, so only flagging Guillaume for re-review.

(The new tests will fail until the gecko patch lands, no worries, I'll make sure to retrigger a bunch before merging)
Attachment #8624212 - Flags: review?(gmarty)
Comment on attachment 8624212 [details] [review]
Gaia PR relanding

Stealing!
Attachment #8624212 - Flags: review?(gmarty) → review+
https://github.com/mozilla-b2g/gaia/commit/ae08ba8dd10031ac35ca5900a36d61fdc6731688
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
No longer blocks: 1176711
Depends on: 1176711
Keywords: leave-open
Target Milestone: --- → FxOS-S1 (26Jun)
You need to log in before you can comment on or make changes to this bug.