The default bug view has changed. See this FAQ.
Bug 1314091 (compact-themes)

Change the Dev Edition theme(s) to be alternative "compact themes" and let them ride the trains

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Theme
P3
normal
RESOLVED FIXED
5 months ago
11 days ago

People

(Reporter: canuckistani, Assigned: bgrins)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug)

Trunk
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox53+ fixed, relnote-firefox 53+)

Details

User Story

#MVP

* As a user with a low-resolution screen laptop or 2-in1 tablet ( eg 1366x768 ), I would appreciate a browser that used as little vertical space as possible
* I would like to be able to switch to a compact theme from about:addons as usual.
* I would like to be able to switch to a compact theme from Firefox’s customization mode because I’m on of the 5 people who knows where that is.
* I would like to choose a ‘dark’ compact theme to match all the other creative dark theme apps I use like photoshop and Logic Pro.
* I would like to choose a ‘light’ compact theme to match the light theme
* If my system has a fairly low[1] resolution screen, Firefox should suggest using a compact theme in order to save space.
* Firefox should explain clearly that compact themes do not currently support lightweight themes.


#Backlog


* As a lightweight themes enthusiast, I would love it if this new compact theme supported my lightweight favourite lightweight themes as well.
* As a lightweight theme author, I would be overjoyed to hear that I had to make no changes to my theme in order for it to work with compact themes.
* Depending on what operating system I am on, the dark and light themes should adopt as native a colour pallette as possible.
* As a developer, I want to be able to change the colors in the toolbox.
* As a developer, it would be great if common developer themes from other products like sublime text could be imported and used in the toolbox

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 5 obsolete attachments)

58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
152.12 KB, image/png
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details | Review
58 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
184.92 KB, image/gif
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details | Review
See user story
(Reporter)

Updated

5 months ago
User Story: (updated)

Comment 1

5 months ago
Was this discussed with the folks who are working on the lwt/"full theme" overhaul? The kind of "meshing" between different themes suggested here seems... complex, and devedition does more than just being "compact" / dark/light.
It was discussed but I didn't see a conclusion come through. The last message I have in the email thread is from October 12th.

Since those last messages, we are now quite a bit closer to implementing the DevEdition theme through the new prototype Theme API over in bug 1306671.

I would prefer that these "compact themes" get implemented using the new API as right now the DevEdition theme touches a lot of places and the list of changes are not cataloged in one easy to find place.
I logged this bug to get it in the system. bgrins has some ideas on how to implement this and he is returning from leave tomorrow, this reminded me to add this.
Component: General → Theme
(Reporter)

Updated

5 months ago
See Also: → bug 1306671
(Assignee)

Comment 4

5 months ago
(In reply to :Gijs Kruitbosch from comment #1)
> Was this discussed with the folks who are working on the lwt/"full theme"
> overhaul? The kind of "meshing" between different themes suggested here
> seems... complex, and devedition does more than just being "compact" /
> dark/light.

There could potentially be two steps here: the first is splitting the current DE theme into two different LW themes which are preinstalled on all channels, and the second is converting these LW themes into addons using the new theme API.  There'd be extra work created by doing it in two steps due to migration from one to the other, so whether it makes sense to do it in two steps or wait until the theme API is completed before rolling out 'compact themes' depends on product needs / timelines.

I have a WIP patch that does quite a bit of the first step and it might be helpful for testing, so I'll attach that.
(Assignee)

Comment 5

5 months ago
Created attachment 8807228 [details] [diff] [review]
compact-themes-WIP.patch

Just a WIP.  Here's a try push for binaries (mochitests will be failing, see commit message in the patch): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c191b59448fac880126cf4dbcffa25bdf0beac7.

Note: if we *also* wanted to keep the devtools toolbox colors in sync with the currently applied lwt, that would require some extra code:
1) applying the correct lwt if and only if one of the two is applied when the devtools theme changes
2) applying the correct devtools theme color when one of the two lwts is applied
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

4 months ago
Updated patches are up. This is now moving all devtools-specific logic into the existing devtools-browser module and renaming all of the devedition-related files to compacttheme.  We discussed that there would be no 'linkage' between devtools toolbox color and the compact theme color, we would instead link from the toolbox to about:addons#appearance.

Here's an ongoing try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d534cc23935ce2d7597cb08e9b6fcd3145e50d37.

And talos comparison link: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=09475d266f428774a300033a1fcc37db00edace2&newProject=try&newRevision=d534cc23935ce2d7597cb08e9b6fcd3145e50d37&framework=1&filter=damp&showOnlyImportant=0
Brian: I downloaded the build from that try run and it doesn't run, macOS complains that it is damaged.
(Reporter)

Updated

4 months ago
Flags: needinfo?(bgrinstead)

Comment 11

4 months ago
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> Brian: I downloaded the build from that try run and it doesn't run, macOS
> complains that it is damaged.

I suspect you need to turn off the gatekeeper checks for verified developers.
(Assignee)

Comment 12

4 months ago
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> Brian: I downloaded the build from that try run and it doesn't run, macOS
> complains that it is damaged.

Hm, I see the same thing.  Talos tests ran successfully so the build itself should be fine, but I guess the artifact downloading is somehow not working.  I'll see if a Windows build works

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> > Brian: I downloaded the build from that try run and it doesn't run, macOS
> > complains that it is damaged.
> 
> I suspect you need to turn off the gatekeeper checks for verified developers.

That's a good thought - although I'm not even getting to the warning about verified code AFAICT.  I'm seeing "Nightly is damaged and cannot be opened", whether I try to open it from the dmg or copy to Applications
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 13

4 months ago
FYI the build works on Windows
(Assignee)

Updated

4 months ago
Attachment #8807228 - Attachment is obsolete: true
(Assignee)

Comment 14

4 months ago
Created attachment 8816620 [details] [diff] [review]
compactthemes-folded.patch

Jeff, here's a folded patch file in case you want to do a build locally in the meantime
(Reporter)

Updated

4 months ago
Depends on: 1321911
I got the build working by running

sudo spctl --master-disable 

see http://apple.stackexchange.com/a/245029

The build works really well except for some funky preview behavior in customize mode, see bug 1321911
(Reporter)

Updated

3 months ago
Target Milestone: --- → Firefox 53
(Assignee)

Comment 16

3 months ago
Current todo list:

1) Handle migration for users currently on firefox-devedition@mozilla.org
2) Update the browser_devedition tests expecting the old behavior
3) Figure out syncing (or not) between devtools theme and this theme.  If not, somehow link to about:addons with the appearance tab selected
4) Fix the preview behavior in customize mode thing (bug 1321911)
[Tracking Requested - why for this release]: I'm changing Target Milestone to Tracking. We use Target Milestone to track when the changes actually land. It appears that Jeff wants this to land in 53, so we can use tracking for that.
status-firefox52: affected → ---
tracking-firefox53: --- → ?
Target Milestone: Firefox 53 → ---
(Assignee)

Updated

3 months ago
Depends on: 1323619
(Assignee)

Comment 18

3 months ago
Created attachment 8818766 [details]
all-the-themes.png

With the two compact themes and 'recommended' themes this popup is getting huge (and mouse events seem to get out of whack once it becomes about as tall as the browser window).  Any thoughts about removing one or two of the recommended themes to make it smaller?
Flags: needinfo?(jgriffiths)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8816620 - Attachment is obsolete: true
(Assignee)

Comment 22

3 months ago
Created attachment 8818947 [details]
theme-switching.gif
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

3 months ago
A nice side effect to this change is that we can set the `textcolor` and `accentcolor` properties on the two individual themes which makes them much more like normal lightweight themes, and we get "brighttitlebarforeground", ToolbarIconColor.inferFromText(), and "lwthemetextcolor"/_updateLWTBrightness (Bug 1308407) for free.  The latest patch set removes all of that from browser-devedition.
(Assignee)

Comment 30

3 months ago
Jeff, here's a new try push for binaries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you confirm that Bug 1321911 is resolved in it?
(Assignee)

Comment 31

3 months ago
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

Sorry, accidentally flagged review
Attachment #8818987 - Flags: review?(gijskruitbosch+bugs)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Created attachment 8818766 [details]
> all-the-themes.png
> 
> With the two compact themes and 'recommended' themes this popup is getting
> huge (and mouse events seem to get out of whack once it becomes about as
> tall as the browser window).  Any thoughts about removing one or two of the
> recommended themes to make it smaller?

Can you see if we have telemetry that shows how popular each one is?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 33

3 months ago
I don't see any obvious telemetry here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1421, nor do I see a match in histograms.json for anything matching 'theme' / 'selectedThemeID'.  Maybe Jeff knows if we have some kind of data on what themes people are using.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Created attachment 8818766 [details]
> all-the-themes.png
> 
> With the two compact themes and 'recommended' themes this popup is getting
> huge (and mouse events seem to get out of whack once it becomes about as
> tall as the browser window).  Any thoughts about removing one or two of the
> recommended themes to make it smaller?

I think it would be good to know 2 pieces of info:

1. doe people use customize mode to change themes?
2. I think we could cut it to a top 3, based on popularity on AMO. I'll talk to Kev about that. Logged bug 1323833
(Reporter)

Updated

3 months ago
Depends on: 1323833
Flags: needinfo?(jgriffiths)

Comment 35

3 months ago
Created attachment 8819038 [details]
compact-themes.png

(In reply to Brian Grinstead [:bgrins] from comment #30)
> Jeff, here's a new try push for binaries:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> confirm that Bug 1321911 is resolved in it?

The menu button separator has a style bug in the try build on macOS.
(Assignee)

Comment 36

3 months ago
(In reply to Sören Hentzschel from comment #35)
> Created attachment 8819038 [details]
> compact-themes.png
> 
> (In reply to Brian Grinstead [:bgrins] from comment #30)
> > Jeff, here's a new try push for binaries:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> > confirm that Bug 1321911 is resolved in it?
> 
> The menu button separator has a style bug in the try build on macOS.

Yes, I see that too - thanks for the heads up
(Assignee)

Comment 37

3 months ago
(In reply to Brian Grinstead [:bgrins] from comment #36)
> (In reply to Sören Hentzschel from comment #35)
> > Created attachment 8819038 [details]
> > compact-themes.png
> > 
> > (In reply to Brian Grinstead [:bgrins] from comment #30)
> > > Jeff, here's a new try push for binaries:
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> > > confirm that Bug 1321911 is resolved in it?
> > 
> > The menu button separator has a style bug in the try build on macOS.
> 
> Yes, I see that too - thanks for the heads up

Looks like this is already a bug with DevEdition theme in nightly.  I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1323844 for that
(Assignee)

Comment 38

3 months ago
Comment on attachment 8816497 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme

Gijs, I'd appreciate a round of feedback on the current patch series if you have a chance (no hurry at all since I'll be out for a couple of weeks).  I hope that the series is easy enough to follow and that the end result is simpler than the current DE theme implementation (there are special cases being removed in the first and fifth patches).

I still have some changes to make, like:
* migration for old profiles
* a few more test updates in browser_devedition.js
* figure out syncing between the toolbox theme color and the corresponding lightweight theme
* use updated strings and icons
Attachment #8816497 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to (Unavailable until Jan 4) Brian Grinstead [:bgrins] from comment #33)
> I don't see any obvious telemetry here:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizeMode.jsm#1421, nor do I see a match in
> histograms.json for anything matching 'theme' / 'selectedThemeID'.  Maybe
> Jeff knows if we have some kind of data on what themes people are using.

See "persona" in the environment section of FHR.

https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/environment.html?highlight=persona
https://dxr.mozilla.org/mozilla-central/rev/5a536a16e33798fe7b16de35c968d5bc0cbf8448/toolkit/components/telemetry/TelemetryEnvironment.jsm#537,549

I already had a query for it here: https://sql.telemetry.mozilla.org/queries/313

Updated

3 months ago
Priority: -- → P3

Comment 40

3 months ago
mozreview-review
Comment on attachment 8816497 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme

https://reviewboard.mozilla.org/r/97212/#review99808

::: browser/base/content/browser-devedition.js:78
(Diff revision 3)
>    _inferBrightness: function() {
>      ToolbarIconColor.inferFromText();
> -    // Get an inverted full screen button if the dark theme is applied.
> -    if (this.isStyleSheetEnabled &&
> -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> +
> +    if (this.isStyleSheetEnabled) {
> +      if (this.isCurrentThemeIdDark) {
> -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> +        document.documentElement.setAttribute("brighttitlebarforeground", "true");

Do we not want to do this for dark lwthemes generally? I also wonder if this is worth keeping given that the fullscreen button is now only supported on 10.9, which we do still support but only barely - given we now support 10.12, feels like 10.9 should be gone soon...

::: browser/base/content/browser-devedition.js:127
(Diff revision 3)
> +      let compactTheme = this.isCurrentThemeIdDark ? "dark" : "light";
> +      document.documentElement.setAttribute("compacttheme", compactTheme);

Feels like this should happen before enabling and/or adding the sheet to avoid flashes of partially styled content (given OMT style/compositor/rendering stuff).

::: browser/base/content/test/general/browser_devedition.js:62
(Diff revision 3)
> -add_task(function* testDevtoolsTheme() {
> -  info("Checking stylesheet and :root attributes based on devtools theme.");
> -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light");
> -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> -    "The documentElement has an attribute based on devtools theme.");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the light devtools theme.");
> +add_task(function* testLightweightThemePreview() {
> +  if (SKIP_TEST) {
> +    ok(true, "No need to run this test since themes aren't installed");
> +    return;
> +  }
> +  info("Setting compact to current and the previewing others");

Nit: spelling

::: browser/base/content/test/general/browser_devedition.js
(Diff revision 3)
> -
> -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "foobar");
> -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> -    "The documentElement has 'light' as a default for the devtoolstheme attribute");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the foobar devtools theme.");
> -  ok(!document.documentElement.hasAttribute("brighttitlebarforeground"),
> -     "The brighttitlebarforeground attribute is not set on the window with light devtools theme.");

Why is this being removd? I thought this code still existed in this cset - maybe this removal belongs in a different cset?

::: browser/base/content/test/general/browser_devedition.js
(Diff revision 3)
> -add_task(function* testLightweightThemePreview() {
> -  info("Setting devedition to current and the previewing others");
> -  LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme("firefox-devedition@mozilla.org");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is enabled.");

Why are we removing this test?

Comment 41

3 months ago
mozreview-review
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

https://reviewboard.mozilla.org/r/98868/#review99816

::: devtools/client/framework/devtools-browser.js:146
(Diff revision 1)
> +    let devtoolsTheme = Services.prefs.getCharPref("devtools.theme");
> +    if (devtoolsTheme != "dark") {
> +      devtoolsTheme = "light";
> +    }
> +
> +    win.document.documentElement.setAttribute("devtoolstheme", devtoolsTheme);

Do we actually still need to do this? And shouldn't this attribute be renamed? How do we want this syncing to work now that we're decoupling 'devtools' and thesee compact themes? I'm sorry if that was supposed to be obvious from the bug - I missed it...
Attachment #8818987 - Flags: review-

Comment 42

3 months ago
General trend here looks OK, though I think I would find it easier to review if all the renaming/moving came first. Maybe that's just me. Also feels like we should be able to get rid of the light/dark attribute entirely and just use :-moz-lwtheme-brighttext.

Updated

3 months ago
Attachment #8816497 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Tracking 53+ for this change in compact themes. Marking 53 as affected as well.
status-firefox53: --- → affected
tracking-firefox53: ? → +
(Assignee)

Comment 44

3 months ago
(In reply to :Gijs from comment #42)
> General trend here looks OK, though I think I would find it easier to review
> if all the renaming/moving came first.

Sure, I'll reorder the patches

> Also feels like
> we should be able to get rid of the light/dark attribute entirely and just
> use :-moz-lwtheme-brighttext.

Hm, good point I'll look into that
(Assignee)

Comment 45

3 months ago
(In reply to :Gijs from comment #41)
> Comment on attachment 8818987 [details]
> Bug 1314091 - Remove devtools-specific logic out of browser-devedition and
> into devtools-browser
> 
> https://reviewboard.mozilla.org/r/98868/#review99816
> 
> ::: devtools/client/framework/devtools-browser.js:146
> (Diff revision 1)
> > +    let devtoolsTheme = Services.prefs.getCharPref("devtools.theme");
> > +    if (devtoolsTheme != "dark") {
> > +      devtoolsTheme = "light";
> > +    }
> > +
> > +    win.document.documentElement.setAttribute("devtoolstheme", devtoolsTheme);
> 
> Do we actually still need to do this? And shouldn't this attribute be
> renamed? How do we want this syncing to work now that we're decoupling
> 'devtools' and thesee compact themes? I'm sorry if that was supposed to be
> obvious from the bug - I missed it...

We still need to use it for parts of devtools that live in browser.xul - specifically the splitter between the page and the toolbox and gcli  like https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/splitters.css#23 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.css
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

3 months ago
Pushed up a reordering
(Assignee)

Comment 53

3 months ago
(In reply to Brian Grinstead [:bgrins] from comment #44) 
> > Also feels like
> > we should be able to get rid of the light/dark attribute entirely and just
> > use :-moz-lwtheme-brighttext.
> 
> Hm, good point I'll look into that

Good call, that works.  Pushed up a change with :-moz-lwtheme-brighttext and :-moz-lwtheme-darktext - will go through other commits later to remove any remaining references / setting of the compacttheme attribute.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

3 months ago
(In reply to :Gijs from comment #40)
> Comment on attachment 8816497 [details]
> Bug 1314091 - WIP Expose 'compact' themes instead of the Dev Edition theme
> 
> https://reviewboard.mozilla.org/r/97212/#review99808
> 
> ::: browser/base/content/browser-devedition.js:78
> (Diff revision 3)
> >    _inferBrightness: function() {
> >      ToolbarIconColor.inferFromText();
> > -    // Get an inverted full screen button if the dark theme is applied.
> > -    if (this.isStyleSheetEnabled &&
> > -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> > +
> > +    if (this.isStyleSheetEnabled) {
> > +      if (this.isCurrentThemeIdDark) {
> > -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > +        document.documentElement.setAttribute("brighttitlebarforeground", "true");
> 
> Do we not want to do this for dark lwthemes generally? I also wonder if this
> is worth keeping given that the fullscreen button is now only supported on
> 10.9, which we do still support but only barely - given we now support
> 10.12, feels like 10.9 should be gone soon...
> 

Sorry for any confusion - this is removed later on in the last commit in the series: https://reviewboard.mozilla.org/r/98870/diff/3.  I can fold these together if you think that makes more sense.

> ::: browser/base/content/browser-devedition.js:127
> (Diff revision 3)
> > +      let compactTheme = this.isCurrentThemeIdDark ? "dark" : "light";
> > +      document.documentElement.setAttribute("compacttheme", compactTheme);
> 
> Feels like this should happen before enabling and/or adding the sheet to
> avoid flashes of partially styled content (given OMT
> style/compositor/rendering stuff).

This isn't needed anymore since using :-moz-lwtheme-brighttext in the css, so I'll remove it

> ::: browser/base/content/test/general/browser_devedition.js:62
> (Diff revision 3)
> > -add_task(function* testDevtoolsTheme() {
> > -  info("Checking stylesheet and :root attributes based on devtools theme.");
> > -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light");
> > -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> > -    "The documentElement has an attribute based on devtools theme.");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the light devtools theme.");
> > +add_task(function* testLightweightThemePreview() {
> > +  if (SKIP_TEST) {
> > +    ok(true, "No need to run this test since themes aren't installed");
> > +    return;
> > +  }
> > +  info("Setting compact to current and the previewing others");
> 
> Nit: spelling

I think this is talking about the extra 'the', maybe I'm missing something else but I'll update that part

> ::: browser/base/content/test/general/browser_devedition.js
> (Diff revision 3)
> > -
> > -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "foobar");
> > -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> > -    "The documentElement has 'light' as a default for the devtoolstheme attribute");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the foobar devtools theme.");
> > -  ok(!document.documentElement.hasAttribute("brighttitlebarforeground"),
> > -     "The brighttitlebarforeground attribute is not set on the window with light devtools theme.");
> 
> Why is this being removd? I thought this code still existed in this cset -
> maybe this removal belongs in a different cset?

Yes, this removal belongs with the last change - see comment earlier about if you want those folded in here.

> ::: browser/base/content/test/general/browser_devedition.js
> (Diff revision 3)
> > -add_task(function* testLightweightThemePreview() {
> > -  info("Setting devedition to current and the previewing others");
> > -  LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme("firefox-devedition@mozilla.org");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is enabled.");
> 
> Why are we removing this test?

This test's logic has been moved above the dummyLightweightTheme function and the previous one was removed (about the devtools attribute).  I'll reorder so the diff is easier to follow.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8816497 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

>+:root:-moz-lwtheme-brighttext .findbar-closebutton:not(:hover),

This doesn't really make sense, you should just use .findbar-closebutton:-moz-lwtheme-brighttext:not(:hover). The same goes for other parts of this patch where you used the same pattern.

Updated

3 months ago
Attachment #8819038 - Attachment is obsolete: true
(Assignee)

Comment 80

3 months ago
(In reply to Dão Gottwald [:dao] from comment #79)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> >+:root:-moz-lwtheme-brighttext .findbar-closebutton:not(:hover),
> 
> This doesn't really make sense, you should just use
> .findbar-closebutton:-moz-lwtheme-brighttext:not(:hover). The same goes for
> other parts of this patch where you used the same pattern.

Alright, thanks - I just did a find/replace so didn't consider that.  The only thing I'm worried about is changing specificity when modifying the selector
(Assignee)

Updated

3 months ago
Depends on: 1329207
(Assignee)

Comment 81

3 months ago
(In reply to Brian Grinstead [:bgrins] from comment #60)
> (In reply to :Gijs from comment #40)
> > Comment on attachment 8816497 [details]
> > Bug 1314091 - WIP Expose 'compact' themes instead of the Dev Edition theme
> > 
> > https://reviewboard.mozilla.org/r/97212/#review99808
> > 
> > ::: browser/base/content/browser-devedition.js:78
> > (Diff revision 3)
> > >    _inferBrightness: function() {
> > >      ToolbarIconColor.inferFromText();
> > > -    // Get an inverted full screen button if the dark theme is applied.
> > > -    if (this.isStyleSheetEnabled &&
> > > -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> > > +
> > > +    if (this.isStyleSheetEnabled) {
> > > +      if (this.isCurrentThemeIdDark) {
> > > -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > > +        document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > 
> > Do we not want to do this for dark lwthemes generally? I also wonder if this
> > is worth keeping given that the fullscreen button is now only supported on
> > 10.9, which we do still support but only barely - given we now support
> > 10.12, feels like 10.9 should be gone soon...
> > 
> 
> Sorry for any confusion - this is removed later on in the last commit in the
> series: https://reviewboard.mozilla.org/r/98870/diff/3.  I can fold these
> together if you think that makes more sense.

FYI I went ahead and folded this - I think it's easier to follow this way
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Depends on: 1329262
(Assignee)

Comment 88

3 months ago
Latest builds can be tested at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5440c9a8a980eb69890edaac1e0b55f9e3bf20d7
(Assignee)

Comment 89

3 months ago
Created attachment 8824515 [details]
theme-switching.gif

Updated screencast of switching themes in about:addons with latest code + icons + text
Attachment #8818947 - Attachment is obsolete: true
(Assignee)

Comment 90

3 months ago
Here's summary of progress / todos:

* Simplified and reordered the patch series based on feedback from Gijs
* Dropped the custom compacttheme attribute used for styling by using -moz-lwtheme-brighttext and -moz-lwtheme-darktext instead. This is one of a few code removals in the series due to more directly relying on the lightweight theming system (along with dropping special things like updateLWTBrightness and inferBrightness)
* Using updated icons and text for the themes in about:addons and customize mode

What still needs to be done:
* Handle migration for people currently using the dev edition theme (making sure the right one gets applied based on devtools theme color).  This'll be done in the main bug.
* Sync the devtools theme pref and the selected compact theme (so behavior matches what happens on dev edition have now).  This'll be done in the main bug.
* Set up compact themes with mozscreenshots.  This'll be done in Bug 1329262.
* Get an updated default theme icon as a png (see https://bugzilla.mozilla.org/show_bug.cgi?id=1323619#c7 for more info). This'll land in Bug 1329207.
* Trim recommended list of themes from 5 to 3.  This'll be done in Bug 1323833.

Comment 91

3 months ago
I'll look at this on Monday - sorry for the delay. :-(

Comment 92

3 months ago
mozreview-review
Comment on attachment 8816499 [details]
Bug 1314091 - Rename devedition.* to compacttheme.*;

https://reviewboard.mozilla.org/r/97216/#review103774

r=me for the rename, though of course all of this will need to land together...

::: browser/base/content/test/general/browser_compacttheme.js:30
(Diff revision 9)
>  add_task(function* startTests() {
>    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
>  
>    info("Setting the current theme to null");
>    LightweightThemeManager.currentTheme = null;
> -  ok(!DevEdition.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> +  ok(!CompactTheme.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");

Nit: we should update the messages here so they continue to make sense.

We should probably also ensure any intermittents filed against this bug (hopefully none exist, but...) get updated.
Attachment #8816499 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 93

3 months ago
mozreview-review
Comment on attachment 8818989 [details]
Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext

https://reviewboard.mozilla.org/r/98872/#review103776

::: browser/themes/shared/compacttheme.inc.css:89
(Diff revision 7)
>    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
>  }
>  
>  /* Override the lwtheme-specific styling for toolbar buttons */
> -:root[devtoolstheme="light"],
> -:root[devtoolstheme="light"] toolbar:-moz-lwtheme {
> +:root:-moz-lwtheme-darktext,
> +:root:-moz-lwtheme-darktext toolbar:-moz-lwtheme {

You can use the -moz-lwtheme-bright/darktext selector directly on a descendant element, it doesn't only apply on the root, so this selector (and probably others) can be simplified further. :-)

::: browser/themes/shared/compacttheme.inc.css:200
(Diff revision 7)
> -%define selectorPrefix :root[devtoolstheme="dark"] 
> +%define selectorPrefix :root:-moz-lwtheme-brighttext 
>  %define selectorSuffix :-moz-lwtheme

So I wonder if we can omit the prefix here entirely and just have the suffix...

Comment 94

3 months ago
mozreview-review-reply
Comment on attachment 8816499 [details]
Bug 1314091 - Rename devedition.* to compacttheme.*;

https://reviewboard.mozilla.org/r/97216/#review103774

> Nit: we should update the messages here so they continue to make sense.
> 
> We should probably also ensure any intermittents filed against this bug (hopefully none exist, but...) get updated.

Oh, I see you updated messages in the later cset. We should probably fold before landing, maybe? Should still doublecheck intermittents, so leaving the issue for now.

Comment 95

3 months ago
mozreview-review
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review103782

Clearing review because we should have the migration in here and mozreview doesn't currently let you re-request review - but this looks good to me besides what's not here yet. :-)

::: browser/components/nsBrowserGlue.js:653
(Diff revision 7)
>  
> -    if (!AppConstants.RELEASE_OR_BETA) {
> +    if (AppConstants.INSTALL_COMPACT_THEMES) {
> -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
>        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
>  
> +      // XXX: Localize + Add custom icons

This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?

::: browser/components/nsBrowserGlue.js:655
(Diff revision 7)
> -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
>        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
>  
> +      // XXX: Localize + Add custom icons
>        LightweightThemeManager.addBuiltInTheme({
> -        id: "firefox-devedition@mozilla.org",
> +        id: "firefox-compact-light@mozilla.org",

This cset should probably contain the migration as well. You should be able to just use a UI version migration in this file. That'll be a one-time thing, and we should check that the LWT manager code doesn't fail catastrophically with having the ID of a theme it doesn't know about anymore for the poor souls who do an (unsupported) backwards-migration by running the same profile in multiple versions or something.
Attachment #8818988 - Flags: review?(gijskruitbosch+bugs)

Comment 96

3 months ago
mozreview-review
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

https://reviewboard.mozilla.org/r/102962/#review103786

::: browser/base/content/defaultthemes/compactdark.icon.svg:7
(Diff revision 1)
> +    .background {
> +      fill: #727780;
> +    }

All these classes are only used once here, so the SVG guide ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines ) says they should be attributes. I also believe at least one of the width/height or viewBox is unnecessary, as is the xlink namespace. Same for the other file.

::: browser/base/content/test/general/browser_compacttheme.js:56
(Diff revision 1)
>  function dummyLightweightTheme(id) {
>    return {
>      id,
>      name: id,
> -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
>      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",

Shipping this icon to users just for this test seems silly, shouldn't we just get rid of it?
Attachment #8824467 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 97

3 months ago
mozreview-review-reply
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review103782

> This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?

Oh, I now see this is in the next commit. Sorry for missing that...
(Assignee)

Comment 98

3 months ago
(In reply to :Gijs from comment #97)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> https://reviewboard.mozilla.org/r/98870/#review103782
> 
> > This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?
> 
> Oh, I now see this is in the next commit. Sorry for missing that...

Sorry for any confusion - I can fold these changes into the main patch
(Assignee)

Comment 99

3 months ago
(In reply to :Gijs from comment #96)
> Comment on attachment 8824467 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING Use updated icons for compact themes;
> 
> https://reviewboard.mozilla.org/r/102962/#review103786
> 
> ::: browser/base/content/defaultthemes/compactdark.icon.svg:7
> (Diff revision 1)
> > +    .background {
> > +      fill: #727780;
> > +    }
> 
> All these classes are only used once here, so the SVG guide (
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines ) says they should be attributes. I also believe at least one
> of the width/height or viewBox is unnecessary, as is the xlink namespace.
> Same for the other file.

OK, I'll run them through svgo and make any other changes if necessary

> ::: browser/base/content/test/general/browser_compacttheme.js:56
> (Diff revision 1)
> >  function dummyLightweightTheme(id) {
> >    return {
> >      id,
> >      name: id,
> > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> 
> Shipping this icon to users just for this test seems silly, shouldn't we
> just get rid of it?

This file is also used in nsBrowserGlue as the headerURL for both compact themes
(Assignee)

Comment 100

3 months ago
(In reply to :Gijs from comment #92)
> Comment on attachment 8816499 [details]
> Bug 1314091 - Rename devedition.* to compacttheme.*;
> 
> https://reviewboard.mozilla.org/r/97216/#review103774
> 
> r=me for the rename, though of course all of this will need to land
> together...
> 
> ::: browser/base/content/test/general/browser_compacttheme.js:30
> (Diff revision 9)
> >  add_task(function* startTests() {
> >    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
> >  
> >    info("Setting the current theme to null");
> >    LightweightThemeManager.currentTheme = null;
> > -  ok(!DevEdition.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> > +  ok(!CompactTheme.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> 
> Nit: we should update the messages here so they continue to make sense.

Will do

> We should probably also ensure any intermittents filed against this bug
> (hopefully none exist, but...) get updated.

I don't see any here: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=browser_devedition&list_id=13385208, so I think we are good there
(Assignee)

Comment 101

3 months ago
(In reply to Brian Grinstead [:bgrins] from comment #100)
> (In reply to :Gijs from comment #92)
> > Nit: we should update the messages here so they continue to make sense.
> 
> Will do

Going to make any remaining string changes in the main patch, and they'll be folded together before landing

Comment 102

3 months ago
(In reply to Brian Grinstead [:bgrins] from comment #99)
> > ::: browser/base/content/test/general/browser_compacttheme.js:56
> > (Diff revision 1)
> > >  function dummyLightweightTheme(id) {
> > >    return {
> > >      id,
> > >      name: id,
> > > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> > >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> > 
> > Shipping this icon to users just for this test seems silly, shouldn't we
> > just get rid of it?
> 
> This file is also used in nsBrowserGlue as the headerURL for both compact
> themes

I meant the devedition.icon.png file. Sorry if that was unclear. Are we using that elsewhere?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 103

3 months ago
(In reply to :Gijs from comment #102)
> (In reply to Brian Grinstead [:bgrins] from comment #99)
> > > ::: browser/base/content/test/general/browser_compacttheme.js:56
> > > (Diff revision 1)
> > > >  function dummyLightweightTheme(id) {
> > > >    return {
> > > >      id,
> > > >      name: id,
> > > > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > > > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> > > >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> > > 
> > > Shipping this icon to users just for this test seems silly, shouldn't we
> > > just get rid of it?
> > 
> > This file is also used in nsBrowserGlue as the headerURL for both compact
> > themes
> 
> I meant the devedition.icon.png file. Sorry if that was unclear. Are we
> using that elsewhere?

Oh good point.  The only concerning thing I see is an entry in allowed-dupes.mn that talks about aurora branding: https://dxr.mozilla.org/mozilla-central/source/browser/installer/allowed-dupes.mn#222.  But I don't know why it's there, the branding logo should be in browser/branding/aurora.  I'll go ahead and remove it.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 104

3 months ago
(In reply to :Gijs from comment #93)
> Comment on attachment 8818989 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with
> :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext
> 
> https://reviewboard.mozilla.org/r/98872/#review103776
> 
> ::: browser/themes/shared/compacttheme.inc.css:89
> (Diff revision 7)
> >    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
> >  }
> >  
> >  /* Override the lwtheme-specific styling for toolbar buttons */
> > -:root[devtoolstheme="light"],
> > -:root[devtoolstheme="light"] toolbar:-moz-lwtheme {
> > +:root:-moz-lwtheme-darktext,
> > +:root:-moz-lwtheme-darktext toolbar:-moz-lwtheme {
> 
> You can use the -moz-lwtheme-bright/darktext selector directly on a
> descendant element, it doesn't only apply on the root, so this selector (and
> probably others) can be simplified further. :-)

OK, I'll do that in a new patch to make it easier to review.  Can fold that in before landing as well.

> ::: browser/themes/shared/compacttheme.inc.css:200
> (Diff revision 7)
> > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> >  %define selectorSuffix :-moz-lwtheme
> 
> So I wonder if we can omit the prefix here entirely and just have the
> suffix...

I get a build error when omitting it entirely (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR', 'selectorPrefix') but it seems we could leave it empty and switch the suffix to :-moz-lwtheme-brighttext.
(In reply to Brian Grinstead [:bgrins] from comment #104)
> > ::: browser/themes/shared/compacttheme.inc.css:200
> > (Diff revision 7)
> > > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> > >  %define selectorSuffix :-moz-lwtheme
> > 
> > So I wonder if we can omit the prefix here entirely and just have the
> > suffix...
> 
> I get a build error when omitting it entirely
> (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR',
> 'selectorPrefix') but it seems we could leave it empty and switch the suffix
> to :-moz-lwtheme-brighttext.

This is the only place using selectorPrefix. You can remove it.
(Assignee)

Comment 106

3 months ago
(In reply to Dão Gottwald [:dao] from comment #105)
> (In reply to Brian Grinstead [:bgrins] from comment #104)
> > > ::: browser/themes/shared/compacttheme.inc.css:200
> > > (Diff revision 7)
> > > > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > > > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> > > >  %define selectorSuffix :-moz-lwtheme
> > > 
> > > So I wonder if we can omit the prefix here entirely and just have the
> > > suffix...
> > 
> > I get a build error when omitting it entirely
> > (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR',
> > 'selectorPrefix') but it seems we could leave it empty and switch the suffix
> > to :-moz-lwtheme-brighttext.
> 
> This is the only place using selectorPrefix. You can remove it.

Thanks, will do
(Assignee)

Comment 107

3 months ago
(In reply to :Gijs from comment #95)
> ::: browser/components/nsBrowserGlue.js:655
> (Diff revision 7)
> > -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
> >        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
> >  
> > +      // XXX: Localize + Add custom icons
> >        LightweightThemeManager.addBuiltInTheme({
> > -        id: "firefox-devedition@mozilla.org",
> > +        id: "firefox-compact-light@mozilla.org",
> 
> This cset should probably contain the migration as well. You should be able
> to just use a UI version migration in this file. That'll be a one-time
> thing, and we should check that the LWT manager code doesn't fail
> catastrophically with having the ID of a theme it doesn't know about anymore
> for the poor souls who do an (unsupported) backwards-migration by running
> the same profile in multiple versions or something.

Just confirmed it's OK to have an uninstalled lw theme set in the pref as would be the case in this backwards-migration - you see  the default theme.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 114

3 months ago
mozreview-review
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

https://reviewboard.mozilla.org/r/98868/#review104136

Thanks a ton for migrating that to /devtools/!
Attachment #8818987 - Flags: review?(poirot.alex) → review+

Comment 115

2 months ago
mozreview-review
Comment on attachment 8818989 [details]
Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext

https://reviewboard.mozilla.org/r/98872/#review104138

This patch seems to make "Bug 1314091 - Remove devtools-specific logic out of browser-devedition" useless?
(also it is not obvious to me how -moz-lwtheme-darktext/-moz-lwtheme-brighttext are set in this patch, or what that already done in existing code?)

Comment 116

2 months ago
mozreview-review
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review104216

::: browser/base/content/defaultthemes/compactdark.icon.svg:5
(Diff revision 8)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">

I would not expect the width/height to be necessary if you're already listing a viewBox, and vice versa.

Same for the light icon.
Attachment #8818988 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 117

2 months ago
(In reply to Alexandre Poirot [:ochameau] from comment #115)
> Comment on attachment 8818989 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with
> :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext
> 
> https://reviewboard.mozilla.org/r/98872/#review104138
> 
> This patch seems to make "Bug 1314091 - Remove devtools-specific logic out
> of browser-devedition" useless?
> (also it is not obvious to me how
> -moz-lwtheme-darktext/-moz-lwtheme-brighttext are set in this patch, or what
> that already done in existing code?)

I know it's confusing but the attribute was serving two purposes before:
1. Change colors for dev edition theme
2. Change colors for devtools stuff in browser.xul (gcli, toolbox splitter)

Now, that attribute is only doing (2) so it makes more sense in /devtools/
(Assignee)

Comment 118

2 months ago
(In reply to :Gijs from comment #116)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> https://reviewboard.mozilla.org/r/98870/#review104216
> 
> ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> (Diff revision 8)
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> 
> I would not expect the width/height to be necessary if you're already
> listing a viewBox, and vice versa.
> 
> Same for the light icon.

Alright, I'll update that.  Do you have a preference for width/height vs viewBox?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 119

2 months ago
(In reply to Brian Grinstead [:bgrins] from comment #118)
> (In reply to :Gijs from comment #116)
> > Comment on attachment 8818988 [details]
> > Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> > 
> > https://reviewboard.mozilla.org/r/98870/#review104216
> > 
> > ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> > (Diff revision 8)
> > > +<?xml version="1.0" encoding="UTF-8"?>
> > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> > 
> > I would not expect the width/height to be necessary if you're already
> > listing a viewBox, and vice versa.
> > 
> > Same for the light icon.
> 
> Alright, I'll update that.  Do you have a preference for width/height vs
> viewBox?

Not really. :ntim might if you run into him.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 120

2 months ago
(In reply to :Gijs from comment #119)
> (In reply to Brian Grinstead [:bgrins] from comment #118)
> > (In reply to :Gijs from comment #116)
> > > Comment on attachment 8818988 [details]
> > > Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> > > 
> > > https://reviewboard.mozilla.org/r/98870/#review104216
> > > 
> > > ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> > > (Diff revision 8)
> > > > +<?xml version="1.0" encoding="UTF-8"?>
> > > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > > > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > > > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> > > 
> > > I would not expect the width/height to be necessary if you're already
> > > listing a viewBox, and vice versa.
> > > 
> > > Same for the light icon.
> > 
> > Alright, I'll update that.  Do you have a preference for width/height vs
> > viewBox?
> 
> Not really. :ntim might if you run into him.
I like width/height over viewBox, when viewBox is redundant (this is the case here). width/height actually constrains the dimensions when you open an SVG in a tab, or when you set an SVG as background-image, while viewBox doesn't.
(Assignee)

Comment 121

2 months ago
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

Sorry - I think I confused mozreview with this patch.  This one does need a review but it looks like it somehow carried over an r+ from a different patch in the series.
Attachment #8824467 - Flags: review+ → review?(gijskruitbosch+bugs)
(Assignee)

Updated

2 months ago
Flags: qe-verify+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 129

2 months ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4944c868dae025529c9b43c7d67e73b914611917

Comment 130

2 months ago
Heads-up: this bug is going to conflict/race with bug 1306561.

Comment 131

2 months ago
mozreview-review
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

https://reviewboard.mozilla.org/r/102962/#review104552

Looks OK. Thanks!
Attachment #8824467 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 132

2 months ago
mozreview-review
Comment on attachment 8825580 [details]
Bug 1314091 - Sync the applied devtools toolbox theme and compact themes;

https://reviewboard.mozilla.org/r/103710/#review104612

Are you also doing the opposite operation somewhere?
Changing devtools theme when you install light/dark lightweight theme?

::: devtools/client/framework/devtools-browser.js:165
(Diff revision 1)
> +    if (currentLighweightThemeID == COMPACT_LIGHT_ID && devtoolsTheme == "dark") {
> +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID);
> +    }
> +    if (currentLighweightThemeID == COMPACT_DARK_ID && devtoolsTheme == "light") {
> +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_LIGHT_ID);
> +    }

You may use shorter variable name to ease reading.
(currentTheme, currentThemeID)
Attachment #8825580 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 133

2 months ago
(In reply to Alexandre Poirot [:ochameau] from comment #132)
> Comment on attachment 8825580 [details]
> Bug 1314091 - Sync the applied devtools toolbox theme and compact themes;
> 
> https://reviewboard.mozilla.org/r/103710/#review104612
> 
> Are you also doing the opposite operation somewhere?
> Changing devtools theme when you install light/dark lightweight theme?

Yes that's done in this patch in the "lightweight-theme-changed" observer.

> ::: devtools/client/framework/devtools-browser.js:165
> (Diff revision 1)
> > +    if (currentLighweightThemeID == COMPACT_LIGHT_ID && devtoolsTheme == "dark") {
> > +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID);
> > +    }
> > +    if (currentLighweightThemeID == COMPACT_DARK_ID && devtoolsTheme == "light") {
> > +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_LIGHT_ID);
> > +    }
> 
> You may use shorter variable name to ease reading.
> (currentTheme, currentThemeID)

Done

Comment 134

2 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05090352ae09
Backed out changeset 60d0080b95fe (bug 1308407)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef98a724789
Rename devedition.* to compacttheme.*;r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4333fc0c05eb
Expose 'compact' themes instead of the Dev Edition theme;r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17adc402c82
Remove devtools-specific logic out of browser-devedition and into devtools-browser;r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbe0c7e6489
Sync the applied devtools toolbox theme and compact themes;r=ochameau

Comment 135

2 months ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/05090352ae09
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Comment 136

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cef98a724789
https://hg.mozilla.org/mozilla-central/rev/4333fc0c05eb
https://hg.mozilla.org/mozilla-central/rev/b17adc402c82
https://hg.mozilla.org/mozilla-central/rev/4bbe0c7e6489
Depends on: 1331276

Updated

2 months ago
Depends on: 1331369
Depends on: 1331449

Updated

2 months ago
Blocks: 1331679
(Assignee)

Comment 137

2 months ago
Release Note Request (optional, but appreciated)
[Why is this notable]: Two new built-in themes available
[Affects Firefox for Android]: No
[Suggested wording]: There are two new 'compact' themes available in Firefox, a dark and light version based on the Developer Edition theme 
[Links (documentation, blog post, etc)]: https://groups.google.com/forum/#!msg/firefox-dev/BKJQ8V8pd0o/jVFWD_EOEQAJ
relnote-firefox: --- → ?
Depends on: 1332201

Updated

2 months ago
No longer depends on: 1332201

Updated

2 months ago
Depends on: 1332375
Added to Fx53 Aurora release notes.
relnote-firefox: ? → 53+
Depends on: 1339754
Depends on: 1339859
[Test Plan]:
https://wiki.mozilla.org/QA/Compact Themes
QA Contact: ciprian.georgiu
You need to log in before you can comment on or make changes to this bug.