Closed Bug 1164123 Opened 10 years ago Closed 9 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(b2g-master fixed)

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

People

(Reporter: etienne, Assigned: etienne)

References

Details

Attachments

(4 files)

Gaia PR
46 bytes, text/x-github-pull-request
gmarty
: review+
gaye
: review+
arcturus
: review+
gsvelto
: review+
dkuo
: review+
arthurcc
: review+
julienw
: review+
Details | Review
Sudio PR
38 bytes, text/x-github-pull-request
hub
: review+
Details | Review
bug-1164123.patch
3.48 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
Gaia PR relanding
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
Blocks: spark-theme-editor
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: 9 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: 9 years ago → 9 years ago
Resolution: --- → FIXED
No longer blocks: 1176711
Depends on: 1176711
status-b2g-v2.5: --- → fixed
status-b2g-master: --- → fixed
Keywords: leave-open
Target Milestone: --- → FxOS-S1 (26Jun)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: