1053434 - Create a dark theme for Firefox based on the DevTools color palette

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Comment 1

[Stephen Horlander [:shorlander]]

To clarify, this bug is for creating a full browser theme based on the dark DevTools theme; for use in a DevTools specific branch.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I assume we will create the theme and then get feedback from UX? I would totally love to work on this.

Is this purely just colors or does it include tab shape etc.

Comment 3

[Brian Grinstead [:bgrins]]

Just a heads up, we will not be using all of the new dark theme colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors until Bug 947242 lands, but we should definitely use the ones from that palette for this theme.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Stephen Horlander [:shorlander] from comment #1)
> To clarify, this bug is for creating a full browser theme based on the dark
> DevTools theme; for use in a DevTools specific branch.

By 'full browser theme' do you mean a theme that is installable via the appearance tab in about:addons?  I'm also curious how big of project you think this is going to be, or if you have any preliminary mockups available so we could make a guess.

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Stephen Horlander [:shorlander] from comment #1)
> > To clarify, this bug is for creating a full browser theme based on the dark
> > DevTools theme; for use in a DevTools specific branch.
> 
> By 'full browser theme' do you mean a theme that is installable via the
> appearance tab in about:addons?  I'm also curious how big of project you
> think this is going to be, or if you have any preliminary mockups available
> so we could make a guess.

I haven't really thought about the implementation aspects. I was referring to the scope of the theme covering all of the main browser chrome.

I don't have any mockups yet, so still figuring out what and how much will change.

Comment 7

[Tim Nguyen :ntim]

Mockups : http://people.mozilla.org/~shorlander/mockups/devTools/DevFox/

Comment 8

[Andrew McCreight [:mccr8]]

This does not sound like a security issue.

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> This does not sound like a security issue.

oops, sorry.

Comment 10

[:Gijs (out for now; he/him)]

Some notes:

- we should make sure this doesn't conflict with the "private browsing" dark theme if that's still something we want to do (Stephen?)
- I'm very worried about the tabs with no space above them, because they no longer allow the user to drag the window around. We had quite a number of bugs filed about that when working on Australis - Stephen, could we just skip that and work off the other side tabs from bug 1058854 ?
- Can we reuse the existing "inverted" iconset so we don't have to create yet more copies of Toolbar.png ?
- How far do we want to take this when it comes to dialogs, panels and (context) menus and the like?
- How do we envisage this vs. OS themes and things like Windows' High Contrast Mode and/or the "use author's colors" preference?

Comment 11

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #10)
> - How do we envisage this vs. OS themes and things like Windows' High
> Contrast Mode and/or the "use author's colors" preference?
If someone wants to use this theme, it's unlikely that this user will use High contrast along with it.

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > - How do we envisage this vs. OS themes and things like Windows' High
> > Contrast Mode and/or the "use author's colors" preference?
> If someone wants to use this theme, it's unlikely that this user will use
> High contrast along with it.

Actually, that is very much false - good web developers test how their pages behave in high contrast mode (for accessibility requirements, among other things), and some (web) developers need to use high contrast mode in order to effectively use their computer.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to :Gijs Kruitbosch from comment #10)
> Some notes:
...
> - How far do we want to take this when it comes to dialogs, panels and
> (context) menus and the like?

As far as is practical given the timeline. It's very jarring to have a themed experience and then open a settings / preferences dialog and get the non-themed ui.

> - How do we envisage this vs. OS themes and things like Windows' High
> Contrast Mode and/or the "use author's colors" preference?

We should test high contrast mode to understand what the limitations are, and get feedback from Marco. I don't want to imagine problems where there are no reported ones yet, I want to tests and report actual bugs. So let's test this and at least understand what the limitations are wrt accessibility.

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: devbrowser-theme-WIP.patch

Rough WIP

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8498277 [details] [diff] [review]
devbrowser-theme-WIP.patch

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

::: browser/base/content/browser.js
@@ +2365,5 @@
> + * XXX: Only do this if it is an Aurora build.
> + * XXX: Only do it if no other themes are applied (and listen for that
> + * to change as well)
> + */
> +let DevBrowser = {

Note: please don't add new top-level objects to browser.js. They should be added to a new file browser-*.js and pre-processed in.

Comment 16

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #10)
> Some notes:
> 
> - we should make sure this doesn't conflict with the "private browsing" dark
> theme if that's still something we want to do (Stephen?)

They are both dark, but they look different and PBM will still have another indicator.

> - I'm very worried about the tabs with no space above them, because they no
> longer allow the user to drag the window around. We had quite a number of
> bugs filed about that when working on Australis - Stephen, could we just
> skip that and work off the other side tabs from bug 1058854 ?

I think we have an opportunity to do something here to achieve a specific goal without worrying about the implications on the broader release audience. I proposed we work around the drag issue by leaving some extra drag area on the sides of the tab strip.

I am fine with punting on the side tabs stuff since we don't have an exact plan there.

> - Can we reuse the existing "inverted" iconset so we don't have to create
> yet more copies of Toolbar.png ?

No, those won't match the DevTools theme.

> - How far do we want to take this when it comes to dialogs, panels and
> (context) menus and the like?

We want to style those also, but I haven't done any design work there because I was focused on primary UI due to time constraints.

> - How do we envisage this vs. OS themes and things like Windows' High
> Contrast Mode and/or the "use author's colors" preference?

It should ignore OS theme variants, but might(?) want to take into account high contrast mode. Most high contrast modes are bright on dark though, so I am not sure exactly how beneficial that would be.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] from comment #15)
> Comment on attachment 8498277 [details] [diff] [review]
> devbrowser-theme-WIP.patch
> 
> Review of attachment 8498277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +2365,5 @@
> > + * XXX: Only do this if it is an Aurora build.
> > + * XXX: Only do it if no other themes are applied (and listen for that
> > + * to change as well)
> > + */
> > +let DevBrowser = {
> 
> Note: please don't add new top-level objects to browser.js. They should be
> added to a new file browser-*.js and pre-processed in.

Will do - uploading a new patch with just the changes to browser*.js and empty css files to get more feedback

Comment 18

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #1)
> To clarify, this bug is for creating a full browser theme based on the dark
> DevTools theme; for use in a DevTools specific branch.

Does this mean it won't land on mozilla-central? Who's going to maintain this theme?

Also, why is this bug only visible to Mozilla employees?

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: devbrowser-theme-listeners.patch

A patch that just adds the listeners in browser.js and some empty CSS files where styles will live.  Gijs, I'd like some review/feedback on the approach I'm using here.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Stephen Horlander [:shorlander] from comment #1)
> > To clarify, this bug is for creating a full browser theme based on the dark
> > DevTools theme; for use in a DevTools specific branch.
> 
> Does this mean it won't land on mozilla-central? Who's going to maintain
> this theme?

The last I heard we were planning on the code living inside aurora/nightly only ifdefs, and for the actual theme to be toggled by a pref, which would be set by default for web devs using the special "devbrowser" edition of aurora to develop stuff. Last I checked it wasn't clear whether this would completely take over "regular" aurora or be a separate thing.

> Also, why is this bug only visible to Mozilla employees?

Because it's part of the birthday plans (9th of November, 10 years of Firefox) and, to that extent, "secret".

Comment 21

[Heather Arthur [:harth]]

Saw this on Brian's build. I wouldn't use devbrowser as it is. The no-space-for-dragging is a dealbreaker. And for me the tabs looking like the devtools tabs is a bit confusing, and not as pretty as those rounded tabs. Otherwise looks good!

Comment 22

[:Gijs (out for now; he/him)]

Comment on attachment 8498818 [details] [diff] [review]
devbrowser-theme-listeners.patch

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

Comments below. Generally the approach seems sane to me; I do wonder about all the CSS files though, and if it'd be cleaner to just have a single file with some ifdefs... time will tell, and we can stick with this approach for now because it mirrors browser.css.

::: browser/base/content/browser-devedition.js
@@ +3,5 @@
> + * 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/.
> +#endif
> + */

Nit: if this is preprocessed, no need for "normal" comment markers. If you want them for syntax reasons, put them inside the ifdefs, please.

@@ +15,5 @@
> +  _themePrefName: "general.skins.selectedSkin",
> +  _lwThemePrefName: "lightweightThemes.isThemeSelected",
> +  _devtoolsThemePrefName: "devtools.theme",
> +
> +  href: "chrome://browser/skin/devedition.css",

Nit: can we make this property slightly more descriptive than just "href" ? Maybe "styleSheetLocation" or something?

@@ +22,5 @@
> +  init: function () {
> +    this._readPref();
> +    Services.prefs.addObserver(this._prefName, this, false);
> +    Services.prefs.addObserver(this._lwThemePrefName, this, false);
> +    Services.prefs.addObserver(this._themePrefName, this, false);

I don't think this one is necessary as an observer; changing theme requires a restart, right? So AFAIK the actual selected theme can't change at runtime. I don't know if the pref changes immediately if you switch to a different complete theme, in which case this would actually do the wrong thing right now.

@@ +31,5 @@
> +    if (topic == "nsPref:changed")
> +      this._readPref();
> +  },
> +
> +  _readPref: function() {

Because this doesn't read a single pref anymore, maybe rename this to _update ?

@@ +40,5 @@
> +    let deveditionThemeEnabled = false;
> +    try {
> +      deveditionThemeEnabled = Services.prefs.getBoolPref(this._prefName) &&
> +        !Services.prefs.getBoolPref(this._lwThemePrefName) &&
> +        Services.prefs.getCharPref(this._themePrefName) == "classic/1.0";

There has to be a better way to do this... except I'm told that there isn't really.

You could use the chrome registry to get the URL for chrome://browser/skin/browser.css, and try to figure out if it's in the same kind of place as chrome://browser/content/browser.xul (we only ship 1 real theme, after all) but that's even more hacky, and this only fails if the user has set the pref to some non-existant value, so let's stick with this.

@@ +48,5 @@
> +      let styleSheet = this.styleSheet = document.createProcessingInstruction(
> +        'xml-stylesheet', 'href="' + this.href + '" type="text/css"');
> +      this.styleSheet.addEventListener("load", function onLoad() {
> +        styleSheet.removeEventListener("load", onLoad);
> +        ToolbarIconColor.inferFromText();

Stephen has said he wants different icons than the normal inverted ones, so I don't think we should be using this. We probably need a separate attribute that this code should set itself (because it knows what colors it's given to all the toolbars anyway, so calling into this (which causes sync layout flushes) isn't necessary).

@@ +50,5 @@
> +      this.styleSheet.addEventListener("load", function onLoad() {
> +        styleSheet.removeEventListener("load", onLoad);
> +        ToolbarIconColor.inferFromText();
> +      });
> +      document.insertBefore(this.styleSheet, document.lastChild);

Nit: I would prefer document.documentElement over document.lastChild because it's less fragile in case add-ons insert cruft after the document element.

@@ +65,5 @@
> +    // which allows specific styling based on this theme.
> +    if (deveditionThemeEnabled) {
> +      let darkTheme =
> +       Services.prefs.getCharPref(this._devtoolsThemePrefName) == "dark";
> +      document.documentElement.classList.toggle("devtools-light", !darkTheme);

We normally use attributes rather than classes on the window element; is there a compelling reason for classes here?

I'm a little wary of the light/dark stuff. The designs only feature the dark styles; especially with the discussion about the drag space / tab shape, if we use the current tab shape here, is there a compelling difference between "devtools light" and the regular theme that would require us to work on that right now (ie, what's the benefit of devtools-light over the 'regular' theme)? I think I would much prefer if we follow-up'd that (and only enabled this theme if the user had also selected the 'dark' option in the devtools prefs), but I'm open to argument otherwise.

::: browser/themes/windows/devedition-aero.css
@@ +1,5 @@
> +/* 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/. */
> +
> +%include ../shared/devedition.inc.css

This should include the non-aero variant of the windows file instead, with a define so that you can check for that case. That way you don't have to duplicate stuff between the aero and non-aero variants. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-aero.css .

Comment 23

[:Gijs (out for now; he/him)]

(oh, and considering all the CSS files are preprocessed, could do an ifdef'd license header there, too)

Comment 24

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #22)
> Stephen has said he wants different icons than the normal inverted ones, so
> I don't think we should be using this. We probably need a separate attribute
> that this code should set itself (because it knows what colors it's given to
> all the toolbars anyway, so calling into this (which causes sync layout
> flushes) isn't necessary).

Could probably do with adding a CSS var for Toolbar.png, and updating that if the devtools attribute is set on :root? (Would need to update the things that currently rely on the attribute inferFromText sets to set the var to the -inverted variant, while the devtools theme isn't enabled, with something like :root:not([devtoolstheme]) toolbar[brighttext] { --... })

Oh, on another note, once we start CSS variable-ifying stuff, this will need try pushes with talos runs (tpaint/ts_paint, probably), to ensure we're not affecting perf (too much)... I don't know how optimized our layout code's css variables stuff is. :-(

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8498818 [details] [diff] [review]
> devbrowser-theme-listeners.patch
> 
> Review of attachment 8498818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Comments below. Generally the approach seems sane to me; I do wonder about
> all the CSS files though, and if it'd be cleaner to just have a single file
> with some ifdefs... time will tell, and we can stick with this approach for
> now because it mirrors browser.css.
> 

Yes, I've just copied the structure of browser.css.  I'd also be happy to make a single shared file and use preprocessor directives for platform specific things - I'm not sure how much will be needed.  A couple of comments below:

> @@ +22,5 @@
> > +  init: function () {
> > +    this._readPref();
> > +    Services.prefs.addObserver(this._prefName, this, false);
> > +    Services.prefs.addObserver(this._lwThemePrefName, this, false);
> > +    Services.prefs.addObserver(this._themePrefName, this, false);
> 
> I don't think this one is necessary as an observer; changing theme requires
> a restart, right? So AFAIK the actual selected theme can't change at
> runtime. I don't know if the pref changes immediately if you switch to a
> different complete theme, in which case this would actually do the wrong
> thing right now.

Good point, I've just tested switching themes with removing the listener and it is working better.

> @@ +65,5 @@
> > +    // which allows specific styling based on this theme.
> > +    if (deveditionThemeEnabled) {
> > +      let darkTheme =
> > +       Services.prefs.getCharPref(this._devtoolsThemePrefName) == "dark";
> > +      document.documentElement.classList.toggle("devtools-light", !darkTheme);
> 
> We normally use attributes rather than classes on the window element; is
> there a compelling reason for classes here?
> 
> I'm a little wary of the light/dark stuff. The designs only feature the dark
> styles; especially with the discussion about the drag space / tab shape, if
> we use the current tab shape here, is there a compelling difference between
> "devtools light" and the regular theme that would require us to work on that
> right now (ie, what's the benefit of devtools-light over the 'regular'
> theme)? I think I would much prefer if we follow-up'd that (and only enabled
> this theme if the user had also selected the 'dark' option in the devtools
> prefs), but I'm open to argument otherwise.
> 

Removing the light and dark stuff for now, we can revisit this later.

Comment 26

[Brian Grinstead [:bgrins]]

> @@ +48,5 @@
> > +      let styleSheet = this.styleSheet = document.createProcessingInstruction(
> > +        'xml-stylesheet', 'href="' + this.href + '" type="text/css"');
> > +      this.styleSheet.addEventListener("load", function onLoad() {
> > +        styleSheet.removeEventListener("load", onLoad);
> > +        ToolbarIconColor.inferFromText();
> 
> Stephen has said he wants different icons than the normal inverted ones, so
> I don't think we should be using this. We probably need a separate attribute
> that this code should set itself (because it knows what colors it's given to
> all the toolbars anyway, so calling into this (which causes sync layout
> flushes) isn't necessary).
> 

Hm, I wish that we could use the standard inverted icons at least by default so non toolbar.png fixes like bugs 1018845 and 1075415 will also work here automatically.  If not we will also need new images for the bookmarks chevron / full screen button / any other buttons that get inverted but aren't included in the toolbar.png sprite to match the new icon coloring.

IMO the inverted icons look pretty good with the dark theme even if they don't perfectly match the coloring in the toolbox as per comment 16.  But I can remove this from this patch anyway since it isn't necessary right now.

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: devedition-theme-listeners.patch

Where would an appropriate place be to add a simple test for this?

Comment 28

[Tim Nguyen :ntim]

Note that you can get the same look for toolbar icons by using 0.8 opacity on current icons.

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Created attachment 8498878 [details] [diff] [review]
> devedition-theme-listeners.patch
> 
> Where would an appropriate place be to add a simple test for this?

We don't really have theme tests, so I guess browser/base/content/test/general, for want of somewhere better? Either that or in devtools.

(In reply to Tim Nguyen [:ntim] from comment #28)
> Note that you can get the same look for toolbar icons by using 0.8 opacity
> on current icons.

Interesting idea. I'd be worried about perf though...

Comment 30

[Brian Grinstead [:bgrins]]

Attachment: devedition-theme-listeners.patch

Added simple test

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8499492 [details] [diff] [review]
devedition-theme-listeners.patch

>+#ifdef 0
>+/*
>+ * 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/.
>+ */
>+#endif

This is better:

# 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/.

>+%ifdef 0
>+/* 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/. */
>+%endif

ditto

Comment 32

[:Gijs (out for now; he/him)]

Comment on attachment 8499492 [details] [diff] [review]
devedition-theme-listeners.patch

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

Closer! Now I have lots of test comments, and still a question or two about when we enable the theme. :-)

::: browser/base/content/browser-devedition.js
@@ +24,5 @@
> +    // Listen for changes to all prefs except for complete themes.
> +    // No need for this since changing a complete theme requires a
> +    // restart.
> +    Services.prefs.addObserver(this._lwThemePrefName, this, false);
> +    Services.prefs.addObserver(this._prefName, this, false);

So now we're using the devtools theme irrespective of whether the user has selected the light or dark devtools theme, is that intentional? I kind of assumed we would keep this contingent on also having the dark devtools theme enabled; sorry if that wasn't clear before - I just figured, we'd use the normal, default theme for the devtools light case, and the devtools one if the devtools theme is set to dark.

(we should keep the general pref so that we can toggle this off for normal/other aurora builds and/or if something blows up and we decide not to enable it by default or whatever)

@@ +45,5 @@
> +        Services.prefs.getCharPref(this._themePrefName) == "classic/1.0";
> +    } catch(e) { }
> +
> +    if (deveditionThemeEnabled && !this.styleSheet) {
> +      let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;

As noted on IRC, I am now scared of experimental JS stuff (after Array.prototype.contains). I asked on bug 1038259 what our odds are if we rely on this.

::: browser/base/content/test/general/browser_devedition.js
@@ +17,5 @@
> +});
> +
> +function testsCompleted() {
> +  gBrowser.removeCurrentTab();
> +  window.focus();

It surprises me this is necessary... is it?

@@ +18,5 @@
> +
> +function testsCompleted() {
> +  gBrowser.removeCurrentTab();
> +  window.focus();
> +  finish();

Don't call this here...

@@ +25,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  gBrowser.addEventListener("load", startTests, true);
> +  gBrowser.selectedTab = gBrowser.addTab();

Why are either of these necessary? If they are, you should at least remove the listener - this will fail in automation because you never remove the listener...

@@ +32,5 @@
> +function startTests() {
> +  ok (!DevEdition.styleSheet, "There is no devedition style sheet by default.");
> +
> +  info ("Removing a lightweight theme.");
> +  Services.prefs.setBoolPref(PREF_LWTHEME, false);

Isn't this the default?

@@ +61,5 @@
> +  info ("Setting browser.devedition.theme.enabled to false.");
> +  Services.prefs.setBoolPref(PREF_DEVEDITION_THEME, false);
> +  ok (!DevEdition.styleSheet, "The devedition stylesheet has been removed.");
> +
> +  testsCompleted();

call finish() here and add (in global scope):

registerCleanupFunction(testsCompleted);

so as to make sure we also tidy up if the test breaks/fails.

Comment 33

[Tim Nguyen :ntim]

I've started to work on the CSS, but the patch doesn't seem to work for me.

DevEdition.stylesheet returns null

DevEdition returns this : 
{ _prefName: "browser.devedition.theme.enabled", _themePrefName: "general.skins.selectedSkin", _lwThemePrefName: "lightweightThemes.isThemeSelected", styleSheetLocation: "chrome://browser/skin/devedition.css", styleSheet: null, init: DevEdition.init(), observe: DevEdition.observe(), _update: DevEdition._update(), uninit: DevEdition.uninit() }

Comment 34

[Tim Nguyen :ntim]

It always returns that even if the browser.devedition.theme.enabled pref is set to true.

Comment 35

[Brian Grinstead [:bgrins]]

Attachment: devedition-theme-listeners-lw-no-pref.patch

Tim, please try this one - for whatever reason lightweightThemes.isThemeSelected didn't exist so it was throwing and causing the stylesheet to never load.  I'll update the main patch to include this fix in the next round

Comment 36

[Brian Grinstead [:bgrins]]

Attachment: devedition-theme-changes-WIP.patch

Tim, here is a second patch that you can apply after the first to start from the latest theme changes if you are planning to work on Windows styles

Comment 37

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows-WIP.patch

This adds basic support for Windows.

Comment 38

[Tim Nguyen :ntim]

Attachment: Part 1 : DevEdition theme changes (WIP)

Just fixed a small typo you put inside a variable name.

Comment 39

[Brian Grinstead [:bgrins]]

Attachment: devedition-part1.patch

This patch addresses comments 31 and 32, and also sets an attribute for devtoolstheme="light"|"dark" on the documentElement to allow customization of the browser styles based on the currently applied devtools theme.  Had to change the  It's still up in the air if and when we would specifically change this theme for light coloring, but this attribute would also be very useful today for styling the developer toolbar to match the toolbox (bug 969914).

Still waiting to hear back in Bug 1038259 about if we can safely use template strings.

Comment 40

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-WIP.patch

Shared styling changes to be used across all platforms

Comment 41

[Brian Grinstead [:bgrins]]

Attachment: devedition-part3-osx-WIP.patch

Comment 42

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #22)
> > +      let styleSheet = this.styleSheet = document.createProcessingInstruction(
> > +        'xml-stylesheet', 'href="' + this.href + '" type="text/css"');
> > +      this.styleSheet.addEventListener("load", function onLoad() {
> > +        styleSheet.removeEventListener("load", onLoad);
> > +        ToolbarIconColor.inferFromText();
> 
> Stephen has said he wants different icons than the normal inverted ones, so
> I don't think we should be using this. We probably need a separate attribute
> that this code should set itself (because it knows what colors it's given to
> all the toolbars anyway, so calling into this (which causes sync layout
> flushes) isn't necessary).

ToolbarIconColor.inferFromText should definitely be called here, even if you want to override some icons with custom ones.

Comment 43

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #42)
> (In reply to :Gijs Kruitbosch from comment #22)
> > > +      let styleSheet = this.styleSheet = document.createProcessingInstruction(
> > > +        'xml-stylesheet', 'href="' + this.href + '" type="text/css"');
> > > +      this.styleSheet.addEventListener("load", function onLoad() {
> > > +        styleSheet.removeEventListener("load", onLoad);
> > > +        ToolbarIconColor.inferFromText();
> > 
> > Stephen has said he wants different icons than the normal inverted ones, so
> > I don't think we should be using this. We probably need a separate attribute
> > that this code should set itself (because it knows what colors it's given to
> > all the toolbars anyway, so calling into this (which causes sync layout
> > flushes) isn't necessary).
> 
> ToolbarIconColor.inferFromText should definitely be called here, even if you
> want to override some icons with custom ones.

Why?

Comment 44

[Dão Gottwald [:dao]]

Because the brighttext attribute is supposed to reflect reality rather than some previous state. Also, it's highly unlikely that you'll want to override all icons where that attribute is used.

Comment 45

[Dão Gottwald [:dao]]

It also looks like we'll need to update browser/themes/osx/browser.css to use [brighttext] instead of :-moz-lwtheme-brighttext. Up until now this didn't matter because the only means to get a dark theme on OS X was to apply a lightweight theme.

Comment 46

[Brian Grinstead [:bgrins]]

Attachment: devedition-part1.patch

Same as Comment 39, just removed a trailing */ from a comment

Comment 47

[:Gijs (out for now; he/him)]

Comment on attachment 8501304 [details] [diff] [review]
devedition-part1.patch

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

Closer and closer...

::: browser/base/content/browser-devedition.js
@@ +26,5 @@
> +    Services.prefs.addObserver(this._devtoolsThemePrefName, this, false);
> +  },
> +
> +  observe: function (subject, topic, data) {
> +    if (topic == "nsPref:changed")

Thinking about this, I think it'd make sense to split out the devtoolstheme pref from the other ones, and not run _update for it (ie just update the root attribute). It probably makes sense to chuck that in a function (however trivial it is) so that we can also call it from init().

IIRC the data param should tell you which pref changed here.

@@ +50,5 @@
> +    if (deveditionThemeEnabled && !this.styleSheet) {
> +      let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
> +      this.styleSheet = document.createProcessingInstruction(
> +        'xml-stylesheet', styleSheetAttr);
> +      document.insertBefore(this.styleSheet, document.documentElement);

Per comment #42/44/45, we should bring back the ToolbarIconColor call. Sorry for leading you astray there.

::: browser/base/content/test/general/browser_devedition.js
@@ +21,5 @@
> +});
> +
> +function test() {
> +  waitForExplicitFinish();
> +  gBrowser.addTab();

You've kept the added+removed tab, but AFAICT never use it... any reason not to get rid of it? :-)

Comment 48

[Brian Grinstead [:bgrins]]

> ::: browser/base/content/test/general/browser_devedition.js
> @@ +21,5 @@
> > +});
> > +
> > +function test() {
> > +  waitForExplicitFinish();
> > +  gBrowser.addTab();
> 
> You've kept the added+removed tab, but AFAICT never use it... any reason not
> to get rid of it? :-)

Ah, no.  I misunderstood the last comment - I had just copied some of the boilerplate from another test in the folder but obviously it was doing more than we need here.

Comment 49

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #39)
> Still waiting to hear back in Bug 1038259 about if we can safely use
> template strings.



20:57	Gijs	jorendorff: hey, do you have a second for a quick question?
20:57	jorendorff	Gijs: shoot
20:58	Gijs	jorendorff: the question I asked in bug 1038259 is blocking some devtools stuff - can we rely on JS template strings shipping, ie can we use them in chrome?
20:59	jorendorff	Gijs: This is really a question about future compatibility. The danger is that the JS standard committee may tweak the syntax or semantics before ES6 ships.
21:00	Gijs	jorendorff: I'm hearing "no" :)
21:00	jorendorff	Gijs: In-tree uses will are fine either way.
21:00	Gijs	ah
21:00	Gijs	ok
21:00	Gijs	jorendorff: guesstimate how likely syntax/semantics changes are at this point?
21:02	jorendorff	Gijs: Very hard for me to estimate. They haven't changed now for months, but TC39 is super capricious about anything that hasn't shipped in multiple implementations.
21:02	jorendorff	Gijs: I'd say 92% chances ES6 ships with template strings basically unchanged.
21:03	jorendorff	Make that 90%.
21:05	Gijs	jorendorff: is anyone else implementing?
21:05	jorendorff	Haven't heard anything. cpeterson might know
21:10	Mook_as	FWIW, https://status.modern.ie/templatestringses6 claims IE is in development
21:11	Gijs	Mook_as, jorendorff: OK, thanks.
21:12	jorendorff	Gijs: I thought array comprehensions were fine until they were yanked from the spec altogether
21:12	Gijs	:'(
21:13	Gijs	jorendorff: but we still do those in chrome, right, or did I miss that?
21:13	jorendorff	Gijs: Yeah, we're actually keeping them until TC39 comes up with something standard.
21:13	Gijs	jorendorff: btw, am I ok to quote this convo on bugzilla? :)
21:13	jorendorff	Gijs: sure


So ymmv. I will r+ with or without per jorendorff's "in-tree uses will are fine either way" [sic]. Note that you should push to try if you haven't already, to be sure the js parsable test (yeah, I added one after the css one...) doesn't fall over this because reflect.jsm gets sad. It shouldn't, but you never know (and I've seen it break on the previously-standardized array comprehension format). :-)

Comment 50

[Brian Grinstead [:bgrins]]

I have a try push and don't see any issues with the js parsing: https://tbpl.mozilla.org/?tree=Try&rev=651553f41efa.  I don't have my heart set on using the template strings, the code just looks a little nicer with it.

Comment 51

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #50)
> I have a try push and don't see any issues with the js parsing:
> https://tbpl.mozilla.org/?tree=Try&rev=651553f41efa.
Sweet.

> I don't have my heart
> set on using the template strings, the code just looks a little nicer with
> it.

I've since been informed we already use it in Firefox places code (I paraphrase: "SQL statement stuff is so much easier with it!"). So just go ahead and leave it like this.

Comment 52

[Brian Grinstead [:bgrins]]

Attachment: devedition-part1.patch

Updated as per comments.  I added inferFromText when the stylesheet was added / removed but didn't for when the devtools theme changes yet because it's not entirely clear when and how the devtools theme changes will affect toolbar coloring (and it seems to have a perf impact).  So I'll just add that part in for a future patch.

Comment 53

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-WIP.patch

Comment 54

[:Gijs (out for now; he/him)]

Comment on attachment 8501731 [details] [diff] [review]
devedition-part1.patch

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

LGTM!

::: browser/base/content/browser-devedition.js
@@ +28,5 @@
> +  },
> +
> +  observe: function (subject, topic, data) {
> +    if (topic == "nsPref:changed") {
> +      if (data == "devtools.theme") {

nit: data == this._devtoolsThemePrefName

Comment 55

[Brian Grinstead [:bgrins]]

Attachment: devedition-part1.patch

DRY for pref name

Comment 56

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=ea24844b3d9d

Comment 57

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/98daafd906a9

Comment 58

[Brian Grinstead [:bgrins]]

Stephen, I've been using these arrows for the panels: https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/tooltip/arrow-vertical-dark.png  and https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/tooltip/arrow-horizontal-dark.png.

Which have the right colors, but the dimensions / cropping doesn't match the existing arrows (at https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/arrow/panelarrow-vertical.png).  Can you provide a properly cropped / resized version of these two icons?

Comment 59

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

Comment 60

[Brian Grinstead [:bgrins]]

Attachment: devedition-part3-osx-WIP.patch

Comment 61

[Brian Grinstead [:bgrins]]

Attachment: devedition-part5-linux-WIP.patch

Comment 62

[Brian Grinstead [:bgrins]]

Attachment: devedition-theme-string.patch

The string itself is subject to change, but we'd like to land any string changes before the merge (I'd wait to land this until we decide what it should finally be later today or tomorrow).

What I'd like to know is what you think about this being shown in the preferences pages (both in-content and popup).  I don't have any better ideas about where this option would go, so I've put the string in browser/locales/en-US/chrome/browser/preferences/main.dtd.

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 8502625 [details] [diff] [review]
devedition-theme-string.patch

>+<!ENTITY disableDeveditionTheme.label     "Use DevTools theme for &brandShortName;">

The entity name and the string are contradictory (disable vs. use).

Customize mode contains a dropdown for selecting lightweight themes. Seems like this checkbox would fit better next to that.

Comment 64

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #63)
> Comment on attachment 8502625 [details] [diff] [review]
> devedition-theme-string.patch
> 
> >+<!ENTITY disableDeveditionTheme.label     "Use DevTools theme for &brandShortName;">
> 
> The entity name and the string are contradictory (disable vs. use).

Oops, thanks.

> Customize mode contains a dropdown for selecting lightweight themes. Seems
> like this checkbox would fit better next to that.

Yeah, that could work

Comment 65

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #63)
> Customize mode contains a dropdown for selecting lightweight themes. Seems
> like this checkbox would fit better next to that.

There's also Addons->Appearance, which may be the first place where people look if they are surprised by the change

Comment 66

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #65)
> (In reply to Dão Gottwald [:dao] from comment #63)
> > Customize mode contains a dropdown for selecting lightweight themes. Seems
> > like this checkbox would fit better next to that.
> 
> There's also Addons->Appearance, which may be the first place where people
> look if they are surprised by the change

What seems ideal would be if this showed up in the theme listing just below "Default" and acts like the others (except that it can't be removed).  When all the conditions were right then this entry would be selected instead of Default (even though the Default theme is really applied).  I don't know how much work that would be, especially since similar things may have to be done in Customize mode.

Comment 67

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

Added some icons and cleaned up the shared CSS a bit

Comment 68

[Tim Nguyen :ntim]

Attachment: devedition-part2-improvements.patch

Just a few tweaks.

Comment 69

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

(In reply to Tim Nguyen [:ntim] from comment #68)
> Created attachment 8502840 [details] [diff] [review]
> devedition-part2-improvements.patch
> 
> Just a few tweaks.

Thanks! Folded them in.

Comment 70

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98daafd906a9

Comment 71

[Jared Wein [:jaws] (please needinfo? me)]

Reopening since the 'leave-open' keyword is present and I don't think this should have been marked 'resolved'.

Comment 72

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8502625 [details] [diff] [review]
devedition-theme-string.patch

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

::: browser/locales/en-US/chrome/browser/preferences/main.dtd
@@ +37,5 @@
>  <!ENTITY setAsMyDefaultBrowser.accesskey  "D">
>  <!ENTITY isDefault.label                  "&brandShortName; is currently your default browser">
>  <!ENTITY isNotDefault.label               "&brandShortName; is not your default browser">
>  
> +<!ENTITY disableDeveditionTheme.label     "Use DevTools theme for &brandShortName;">

DevTools seems like a Mozilla-internal type of name. Is that what we want to call this? <bikeshed>Maybe "Use the Developer Tools theme for &brandShortName;"?</bikeshed>

We'll probably also need a theme name that will be used for Customization mode as well as the Add-ons Manager unless that isn't being pursued by this bug (seems like comment #66 is saying that it is not being pursued).

Comment 73

[:Gijs (out for now; he/him)]

Comment on attachment 8502625 [details] [diff] [review]
devedition-theme-string.patch

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

Concur with dão on the name and with jaws on the string. maybe "DevBrowser" if that's going to be the public name of the edition? Don't know if that's decided yet...

Note also that you may want a different DTD depending on where this goes.

Comment 74

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #62)
> Created attachment 8502625 [details] [diff] [review]
> devedition-theme-string.patch
> 
> The string itself is subject to change, but we'd like to land any string
> changes before the merge (I'd wait to land this until we decide what it
> should finally be later today or tomorrow).
> 
> What I'd like to know is what you think about this being shown in the
> preferences pages (both in-content and popup).  I don't have any better
> ideas about where this option would go, so I've put the string in
> browser/locales/en-US/chrome/browser/preferences/main.dtd.

Re-reading all the comments a third time, I guess you're asking for advice? While I'm flattered ( :-) ), this seems like a UX decision.

Adding a fake theme in about:addons IMO isn't a realistic option in the timeframe we're looking at (and even outside that timeframe, I think it's fraught with problems so I wouldn't recommend it).

Stephen/Philipp, can either of you advise on whether this makes more sense in the main preferences or in customize mode (or both) ?

Comment 75

[Brian Grinstead [:bgrins]]

Attachment: devedition-string.patch

Talked with shorlander about this - we are going to go with a checkbox in customize mode with "Use &brandShortName; Theme" as the label.  This checkbox will only show up in the Dev Edition channel (though the theme can still be preffed on in any channel by flipping a pref).

Comment 76

[Brian Grinstead [:bgrins]]

Attachment: devedition-part3-osx.patch

Dão / Gijs, I'd like some feedback on this general approach.  This patch just has the osx changes that basically extract some hardcoded or preprocessed values and turn them into CSS variables.  So after applying this patch alone the browser should look exactly the same as when it is not applied.  Then future patch would change these variables for the dev edition theme.

Beyond general feedback, here are the main questions / concerns I have:

1) In the toolkit files (popup.css, autocomplete.css) for some reason CSS variables don't work when applied to :root even though that works in the other files.  I can file a bug for this if nothing sticks out.
2) I'm not really sure everywhere that would be affected by changes to autocomplete.css.  I see it on the search box (which is what I want) but these seem like generic enough selectors that I'm not sure if I should find somewhere else to put these changes.
3) Should I change *all* preprocessor variables in browser.css to css variables or keep it to only the things that we need to change for now.  I'd imagine that there are some cases where it will not be straightforward to change the values, but I'm not sure.

Comment 77

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8503287 [details] [diff] [review]
devedition-string.patch

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

Bug 1072181 will change the brandShortName, but this bug is about the theme which this string strictly pertains to, hence why this patch is not in bug 1072181.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +716,5 @@
>  <!ENTITY customizeMode.lwthemes.menuManage "Manage">
>  <!ENTITY customizeMode.lwthemes.menuManage.accessKey "M">
>  <!ENTITY customizeMode.lwthemes.menuGetMore "Get More Themes">
>  <!ENTITY customizeMode.lwthemes.menuGetMore.accessKey "G">
> +<!ENTITY customizeMode.deveditionTheme.label "Use &brandShortName; Theme">

r+ contingent on brandShortName becoming something other than Aurora (bug 1072181).

Comment 78

[Brian Grinstead [:bgrins]]

Attachment: devedition-strings.patch

Updated reviewer in commit message

Comment 79

[Brian Grinstead [:bgrins]]

Landed string change: https://hg.mozilla.org/integration/fx-team/rev/15e3bf0b1847

Comment 80

[Brian Grinstead [:bgrins]]

Attachment: devedition-test-defaults.patch

Small change to the browser_devedition.js test to not assume defaults (this was causing failures on Gum where we are presetting the browser.devedition.theme.enabled and devtools.theme prefs with different values.

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15e3bf0b1847

Comment 82

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #76)
> 1) In the toolkit files (popup.css, autocomplete.css) for some reason CSS
> variables don't work when applied to :root even though that works in the
> other files.  I can file a bug for this if nothing sticks out.

Airport comment without looking at much else: I bet this is because the files you mentioned get included as part of XBL, and are "special" in whatever way. They don't show up in the devtools style editor either, for instance. Not sure how hard it'd be to fix this particular issue (which sounds like a bug, but I've not thought about it too hard nor looked at how exactly you're trying to use variables in there... perhaps setting the var on the root of the binding (ie the panel/popup bit for popup.css) might help? Unsure.

Comment 83

[Dão Gottwald [:dao]]

Comment on attachment 8503351 [details] [diff] [review]
devedition-part3-osx.patch

(In reply to Brian Grinstead [:bgrins] from comment #76)
> Beyond general feedback, here are the main questions / concerns I have:
> 
> 1) In the toolkit files (popup.css, autocomplete.css) for some reason CSS
> variables don't work when applied to :root even though that works in the
> other files.

Because they're scoped stylesheets?

> 2) I'm not really sure everywhere that would be affected by changes to
> autocomplete.css.  I see it on the search box (which is what I want) but
> these seem like generic enough selectors that I'm not sure if I should find
> somewhere else to put these changes.

autocomplete.css is widely used, including autocomplete popups in the content area.

> 3) Should I change *all* preprocessor variables in browser.css to css
> variables or keep it to only the things that we need to change for now.  I'd
> imagine that there are some cases where it will not be straightforward to
> change the values, but I'm not sure.

Let's avoid scope-creep.

>+:root {
>+  --space-above-tabbar: 9px;

I don't know where the idea to change this for this theme came from. It seems like a bad idea to me.

> .urlbar-history-dropmarker {
>   padding: 0 3px;
>-  list-style-image: url("chrome://browser/skin/urlbar-history-dropmarker.png");
>-  -moz-image-region: rect(0px, 11px, 14px, 0px);
>+  list-style-image: var(--urlbar-dropmarker-url);
>+  -moz-image-region: var(--urlbar-dropmarker-region);
> }
> 
> .urlbar-history-dropmarker[open="true"],
> .urlbar-history-dropmarker:hover:active {
>   -moz-image-region: rect(0px, 22px, 14px, 11px);
>   background-image: radial-gradient(circle closest-side, hsla(205,100%,70%,.3), hsla(205,100%,70%,0));
> }

You made the normal region variable (not quite sure why) but not the one for hover, which doesn't make sense as they depend on each other.

>-.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(ltr):-moz-lwtheme-brighttext,
>-.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(rtl):-moz-lwtheme-brighttext {
>+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(ltr),
>+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(rtl),
>+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-up,
>+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-down {
>   list-style-image: url("chrome://browser/skin/tabbrowser/tab-arrow-left-inverted.png");
> }
> 
>-.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr):-moz-lwtheme-brighttext,
>-.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl):-moz-lwtheme-brighttext {
>+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr),
>+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl),
>+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr),
>+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl) {
>   list-style-image: url("chrome://browser/skin/tabbrowser/tab-arrow-right-inverted.png");
> }

Why did you add redundant selectors here?

Comment 84

[:Gijs (out for now; he/him)]

Comment on attachment 8503351 [details] [diff] [review]
devedition-part3-osx.patch

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

I think Dão did the hard work here already; otherwise it looks good to me. :-)

Comment 85

[Brian Grinstead [:bgrins]]

Landed devedition-test-defaults.patch: https://hg.mozilla.org/integration/fx-team/rev/d18f4f55dd4e

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d18f4f55dd4e

Comment 87

[Stephen Horlander [:shorlander]]

Attachment: Menus and Panels Mockups - 01

Mockup for awesomebar pop-up and panels.

- Will need more inverted icons
- If we want to keep the current dimensions of the arrow the border will have to be thin

Comment 88

[Brian Grinstead [:bgrins]]

Attachment: autocompletes.png

Thanks Stephen for the designs.  Here is my current progress on the autocompletes.  I have 2 specific questions based on this:

1) For the in content popups (see the second screenshot in the image), should these have the normal white styling (unaffected by the new theme) or should they look something like they do in this screenshot (matching the search results area)?  I'm not positive if there is a way to distinguish between these two places in CSS, but I can check into that.
2) In the location bar there is a special highlight for the matching parts of the string.  Should that stay / go / be changed somehow?

Comment 89

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #83)
> Comment on attachment 8503351 [details] [diff] [review]
> devedition-part3-osx.patch
> 

Thanks for the feedback, I'll upload an updated patch shortly.

> > 2) I'm not really sure everywhere that would be affected by changes to
> > autocomplete.css.  I see it on the search box (which is what I want) but
> > these seem like generic enough selectors that I'm not sure if I should find
> > somewhere else to put these changes.
> 
> autocomplete.css is widely used, including autocomplete popups in the
> content area.

As I've said in Comment 88, I'm not sure if there is a way to specify styles that should be applied only when the popup is opened on a particular element (like the search box).  Still waiting to hear if we want styles to be applied to all popups or just this one.

> 
> >+:root {
> >+  --space-above-tabbar: 9px;
> 
> I don't know where the idea to change this for this theme came from. It
> seems like a bad idea to me.

That's how the designs are mocked up.  Stephen has suggested to instead add 50px extra padding to the left and right of the tab strip to give dragging space.

> 
> > .urlbar-history-dropmarker {
> >   padding: 0 3px;
> >-  list-style-image: url("chrome://browser/skin/urlbar-history-dropmarker.png");
> >-  -moz-image-region: rect(0px, 11px, 14px, 0px);
> >+  list-style-image: var(--urlbar-dropmarker-url);
> >+  -moz-image-region: var(--urlbar-dropmarker-region);
> > }
> > 
> > .urlbar-history-dropmarker[open="true"],
> > .urlbar-history-dropmarker:hover:active {
> >   -moz-image-region: rect(0px, 22px, 14px, 11px);
> >   background-image: radial-gradient(circle closest-side, hsla(205,100%,70%,.3), hsla(205,100%,70%,0));
> > }
> 
> You made the normal region variable (not quite sure why) but not the one for
> hover, which doesn't make sense as they depend on each other.

I've now added variables for all of these states (including hover and 2x).

> 
> >-.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(ltr):-moz-lwtheme-brighttext,
> >-.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(rtl):-moz-lwtheme-brighttext {
> >+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(ltr),
> >+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(rtl),
> >+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-up,
> >+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-down {
> >   list-style-image: url("chrome://browser/skin/tabbrowser/tab-arrow-left-inverted.png");
> > }
> > 
> >-.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr):-moz-lwtheme-brighttext,
> >-.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl):-moz-lwtheme-brighttext {
> >+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr),
> >+#TabsToolbar[brighttext] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl),
> >+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr),
> >+#TabsToolbar[brighttext] .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl) {
> >   list-style-image: url("chrome://browser/skin/tabbrowser/tab-arrow-right-inverted.png");
> > }
> 
> Why did you add redundant selectors here?

Good catch, I've removed them.

Comment 90

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

Comment 91

[Brian Grinstead [:bgrins]]

Attachment: devedition-part3-osx.patch

Comment 92

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #89)
> > >+:root {
> > >+  --space-above-tabbar: 9px;
> > 
> > I don't know where the idea to change this for this theme came from. It
> > seems like a bad idea to me.
> 
> That's how the designs are mocked up.  Stephen has suggested to instead add
> 50px extra padding to the left and right of the tab strip to give dragging
> space.

Generally I would argue that accessibility and usability shouldn't be compromised even if the target audience /tends/ to be more enabled and advanced users. There are always exceptions to that tendency, and good reasons for having the space above the tabs don't become void here. (If the reasons aren't that good, we should just get rid of that space in the default theme.)

Another concern is that letting this theme unnecessarily depart from the default theme unnecessarily increases the maintenance cost. This is also a concern I see with the square tabs, considering that our tab-related CSS is rather complex. By the way, my question about who's going to own and maintain this theme still appears to be unanswered.

Comment 93

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #92)
> (In reply to Brian Grinstead [:bgrins] from comment #89)
> > > >+:root {
> > > >+  --space-above-tabbar: 9px;
> > > 
> > > I don't know where the idea to change this for this theme came from. It
> > > seems like a bad idea to me.
> > 
> > That's how the designs are mocked up.  Stephen has suggested to instead add
> > 50px extra padding to the left and right of the tab strip to give dragging
> > space.
> 
> Generally I would argue that accessibility and usability shouldn't be
> compromised even if the target audience /tends/ to be more enabled and
> advanced users. There are always exceptions to that tendency, and good
> reasons for having the space above the tabs don't become void here. (If the
> reasons aren't that good, we should just get rid of that space in the
> default theme.)

That's discussion to have with Stephen - the most recent thing I've heard is that the extra 50px on the sides will replace the space on the top.  FWIW I also have concerns about accessibility.  As one example, I've had times when switching between external displays and my smaller laptop screen where the browser ends up  wider than the screen and I need to drag it to a side to be able to resize it - the extra space on the sides may not be showing up in this case.

> Another concern is that letting this theme unnecessarily depart from the
> default theme unnecessarily increases the maintenance cost. This is also a
> concern I see with the square tabs, considering that our tab-related CSS is
> rather complex. By the way, my question about who's going to own and
> maintain this theme still appears to be unanswered.

The less departure there is from the main theme the easier to maintain it will be (it'd be ideal if we could get this theme to eventually only consist of a set of CSS variables).  AFAIK The developer edition is going to be maintained by the devtools team.

Comment 94

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

Spent some time cleaning up the CSS and styling the secondary UIs

Comment 95

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Brian Grinstead [:bgrins] from comment #94)
> Created attachment 8506124 [details] [diff] [review]
> devedition-part2-shared-WIP.patch

The part2 patch (both latest and previous versions) break the test browser_880164_customization_context_menus.js on OS X (both on gum and locally).

Comment 96

[Stephen Horlander [:shorlander]]

Attachment: menuPanel-icons-inverted.zip - 01

Inverted Menu Panel Icons

Comment 97

[Panos Astithas (he/him) [:past] (please ni?)]

There is another broken test, test_leaf_layers_partition_browser_window.xul on Windows 7 and 8 debug builds only. Not XP, not other platforms, not optimized builds.

It starts failing ever since the first runs of the mochitest-other suite, and it was only green at that time when you backed out the shared, OS X and Linux theme changes in a separate push (which means I should now apologize for whining about that push ;-) ). I'm guessing the shared patch is the culprit, but I'm sure you can confirm it easily by backing that out and running the test locally (I would have done that myself if my Windows VM wasn't about a year old, which basically means bye-bye weekend).

Comment 98

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #95)
> (In reply to Brian Grinstead [:bgrins] from comment #94)
> > Created attachment 8506124 [details] [diff] [review]
> > devedition-part2-shared-WIP.patch
> 
> The part2 patch (both latest and previous versions) break the test
> browser_880164_customization_context_menus.js on OS X (both on gum and
> locally).

I started seeing some weird popup locations on my build about a week ago [0][1], even without any patches applied.  I'm not sure what is causing it, but browser_880164_customization_context_menus.js is failing for me locally even without anything applied, so it's very hard to debug.  I've pulled down the latest changes, clobbered, rebuilt and am still seeing the issue.  The weird thing is if I pull down a build from Gum I don't see the problem anymore, so I'm still looking into it.

[0]: https://www.dropbox.com/s/0mu0xsbyxznfuvf/Screenshot%202014-10-17%2007.35.00.png?dl=0
[1]: https://www.dropbox.com/s/6405fb1jd6bz025/Screenshot%202014-10-17%2007.34.34.png?dl=0

Comment 99

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #98)
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #95)
> > (In reply to Brian Grinstead [:bgrins] from comment #94)
> > > Created attachment 8506124 [details] [diff] [review]
> > > devedition-part2-shared-WIP.patch
> > 
> > The part2 patch (both latest and previous versions) break the test
> > browser_880164_customization_context_menus.js on OS X (both on gum and
> > locally).
> 
> I started seeing some weird popup locations on my build about a week ago
> [0][1], even without any patches applied.  I'm not sure what is causing it,
> but browser_880164_customization_context_menus.js is failing for me locally
> even without anything applied, so it's very hard to debug.  I've pulled down
> the latest changes, clobbered, rebuilt and am still seeing the issue.  The
> weird thing is if I pull down a build from Gum I don't see the problem
> anymore, so I'm still looking into it.
> 
> [0]:
> https://www.dropbox.com/s/0mu0xsbyxznfuvf/Screenshot%202014-10-17%2007.35.00.
> png?dl=0
> [1]:
> https://www.dropbox.com/s/6405fb1jd6bz025/Screenshot%202014-10-17%2007.34.34.
> png?dl=0

The menu positioning stuff might be bug 1083724?

I don't know about customization_context_menus.js and why it'd be broken. What part of it is broken - got example logs or something? :-)

Comment 100

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gijs Kruitbosch from comment #99)
> I don't know about customization_context_menus.js and why it'd be broken.
> What part of it is broken - got example logs or something? :-)

Here is one instance:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=11146&repo=gum

shownPromise in the fourth task is never fulfilled.

Comment 101

[:Gijs (out for now; he/him)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #100)
> (In reply to :Gijs Kruitbosch from comment #99)
> > I don't know about customization_context_menus.js and why it'd be broken.
> > What part of it is broken - got example logs or something? :-)
> 
> Here is one instance:
> 
> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=11146&repo=gum
> 
> shownPromise in the fourth task is never fulfilled.

Blargh, treeherder's log viewer is hard to read.

If I understand correctly from what you're saying, we never show the toolbar context menu for the url bar correctly, here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_880164_customization_context_menus.js#111

Note comments:
117   // Need to make sure not to click within an edit field.
118   let urlbarRect = urlBarContainer.getBoundingClientRect();
119   EventUtils.synthesizeMouse(urlBarContainer, 100, urlbarRect.height - 1, {type: "contextmenu", button: 2 });
120   yield shownPromise;

If the new style means that height - 1 is inside the edit field, we probably get the wrong context menu leading to a timeout...

This is OS X 10.6 debug; is it broken elsewhere?

It's not super hard to just make that test add a non-removable widget to the navbar and test on that instead, so there's no context menu conflict, so we could do that to workaround?

Then there's the question of why it fails locally before the patch (which was what Brian said), which doesn't really make sense either... perhaps that's a separate bug?

Comment 102

[:Gijs (out for now; he/him)]

Ah, so opt is also broken. That might make sense considering the explanation.

The windows failure is more annoying. You can search for other bugs with that test; it basically boils down to there being layers of chrome that overlap content. The best way to debug is... pushing a debug patch that makes it actually dump the frame tree, after which you then get to spend time trying to figure out where the overlap is (I'm not aware of us having a tool for this, but it's possible layout/graphics folks can help you there).

The alternative is figuring out what you're doing that might over/underlap content... if you're messing with the titlebar and/or removing the overflow: -moz-hidden-unscrollable stuff that we added to avoid this, that might be the thing.

Comment 103

[Brian Grinstead [:bgrins]]

Attachment: devedition-part2-shared-WIP.patch

Using the inverted menu panel icons from comment 96

Comment 104

[Brian Grinstead [:bgrins]]

Attachment: devedition-part3.patch

Comment 105

[Brian Grinstead [:bgrins]]

Comment on attachment 8507636 [details] [diff] [review]
devedition-part3.patch

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

There are still a couple of notes in there with regards to not being able to use the :root selector in certain places and browser_parseable_css.js not liking translateY(-var(--space-above-tabbar)), but it's close enough for reviewing the rest of it

Comment 106

[Brian Grinstead [:bgrins]]

(In reply to Stephen Horlander [:shorlander] from comment #96)
> Created attachment 8506582 [details]
> menuPanel-icons-inverted.zip - 01
> 
> Inverted Menu Panel Icons

Do we need special inverted icons for Yosemite?

Comment 107

[Stephen Horlander [:shorlander]]

(In reply to Brian Grinstead [:bgrins] from comment #106)
> (In reply to Stephen Horlander [:shorlander] from comment #96)
> > Created attachment 8506582 [details]
> > menuPanel-icons-inverted.zip - 01
> > 
> > Inverted Menu Panel Icons
> 
> Do we need special inverted icons for Yosemite?

No, just the one set of inverted should work everywhere.

Comment 108

[:Gijs (out for now; he/him)]

Comment on attachment 8507636 [details] [diff] [review]
devedition-part3.patch

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

r+ for everything besides autocomplete.css, with the nits addressed.

(still have reservations about the tabbar space, but adding a variable for it won't hurt - if anything, should make it easier to revert that if necessary)

::: browser/themes/osx/browser.css
@@ +35,5 @@
> +  --toolbarbutton-combined-boxshadow: 0 0 0 1px hsla(0,0%,100%,.15);
> +  --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(0,0%,0%,.15) 0, hsla(0,0%,0%,.15) 18px);
> +
> +  --urlbar-dropmarker-url: url("chrome://browser/skin/urlbar-history-dropmarker.png");
> +  --urlbar-dropmarker-region: rect(0px, 11px, 14px, 0px);

Nit: 0 for 0px throughout.

@@ +41,5 @@
> +  --urlbar-dropmarker-2x-url: url("chrome://browser/skin/[e-mail]");
> +  --urlbar-dropmarker-2x-region: rect(0px, 22px, 28px, 0px);
> +  --urlbar-dropmarker-active-2x-region: rect(0px, 44px, 28px, 22px);
> +
> +  --menupanel-list-style-image-2x: url(chrome://browser/skin/[e-mail]);

Why only for the 2x version? The 1x version is somewhere in the shared/*.inc files and will also need updating. There are still non-retina macs! :-)

@@ +4704,5 @@
> +    translateY(-var(--space-above-tabbar));
> +    even though it seems to work fine here: http://codepen.io/anon/pen/gedtj
> +    */
> +    --negative-space: -var(--space-above-tabbar);
> +    transform: translateY(var(--negative-space));

See discussion on IRC.

::: toolkit/themes/osx/global/autocomplete.css
@@ +8,5 @@
> +/* XXX: can't use the :root selector for some reason
> +(the variables will not apply in selectors like
> +.autocomplete-treebody::-moz-tree-row otherwise).
> +*/
> +.autocomplete-treebody {

As Dão pointed out, this is used pretty widely. Generally, we use platform-dependent colors there (I think?). I can point you to a number of places where OS themes make websites not display nicely if you use a dark theme, because the OS colors don't match the site, or the site sets only foreground/background and not the other.

I'd therefore prefer to have this stuff specific to the browser chrome through judicious use of selectors. We could technically use variables here and then only set the variables differently in the devedition theme for other selectors, I think? However, at that point I wonder if it isn't more sensible / less code to just override the colors for those autocomplete popups (for which we're already having to use more specific selectors) without the use of variables.

Does that make sense and/or do you think we should also be styling content autocomplete boxes?

@@ +81,5 @@
>  }
>  
>  .autocomplete-treebody::-moz-tree-row {
>    border-top: none;
> +  background-color: var(--autocomplete-background-color);

Hm. It's kind of odd that this needs adding; is it not set higher up? i.e. if this is transparent then something else needs to be white here, I would think - why can't we CSS variable that?

Comment 109

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #108)
> @@ +41,5 @@
> > +  --urlbar-dropmarker-2x-url: url("chrome://browser/skin/[e-mail]");
> > +  --urlbar-dropmarker-2x-region: rect(0px, 22px, 28px, 0px);
> > +  --urlbar-dropmarker-active-2x-region: rect(0px, 44px, 28px, 22px);
> > +
> > +  --menupanel-list-style-image-2x: url(chrome://browser/skin/[e-mail]);
> 
> Why only for the 2x version? The 1x version is somewhere in the shared/*.inc
> files and will also need updating. There are still non-retina macs! :-)

I have the change for that in part2 right now - I'll fold that into this patch.

> 
> ::: toolkit/themes/osx/global/autocomplete.css
> @@ +8,5 @@
> > +/* XXX: can't use the :root selector for some reason
> > +(the variables will not apply in selectors like
> > +.autocomplete-treebody::-moz-tree-row otherwise).
> > +*/
> > +.autocomplete-treebody {
> 
> As Dão pointed out, this is used pretty widely. Generally, we use
> platform-dependent colors there (I think?). I can point you to a number of
> places where OS themes make websites not display nicely if you use a dark
> theme, because the OS colors don't match the site, or the site sets only
> foreground/background and not the other.
> 
> I'd therefore prefer to have this stuff specific to the browser chrome
> through judicious use of selectors. We could technically use variables here
> and then only set the variables differently in the devedition theme for
> other selectors, I think? However, at that point I wonder if it isn't more
> sensible / less code to just override the colors for those autocomplete
> popups (for which we're already having to use more specific selectors)
> without the use of variables.

Is there a way to specifically target just changes to chrome popups?  I think this would be best also.

Comment 110

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #109)
> > ::: toolkit/themes/osx/global/autocomplete.css
> > @@ +8,5 @@
> > > +/* XXX: can't use the :root selector for some reason
> > > +(the variables will not apply in selectors like
> > > +.autocomplete-treebody::-moz-tree-row otherwise).
> > > +*/
> > > +.autocomplete-treebody {
> > 
> > As Dão pointed out, this is used pretty widely. Generally, we use
> > platform-dependent colors there (I think?). I can point you to a number of
> > places where OS themes make websites not display nicely if you use a dark
> > theme, because the OS colors don't match the site, or the site sets only
> > foreground/background and not the other.
> > 
> > I'd therefore prefer to have this stuff specific to the browser chrome
> > through judicious use of selectors. We could technically use variables here
> > and then only set the variables differently in the devedition theme for
> > other selectors, I think? However, at that point I wonder if it isn't more
> > sensible / less code to just override the colors for those autocomplete
> > popups (for which we're already having to use more specific selectors)
> > without the use of variables.
> 
> Is there a way to specifically target just changes to chrome popups?  I
> think this would be best also.

Not that I know of, except perhaps by using a document url prefix query for browser.xul... what autocomplete dropdowns do we have in browser.xul though? Isn't it just the urlbar and search bar? Might make more sense to just select those specifically, assuming that's possible.

Comment 111

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #110)
> > Is there a way to specifically target just changes to chrome popups?  I
> > think this would be best also.
> 
> Not that I know of, except perhaps by using a document url prefix query for
> browser.xul... what autocomplete dropdowns do we have in browser.xul though?
> Isn't it just the urlbar and search bar? Might make more sense to just
> select those specifically, assuming that's possible.

The urlbar is styled via .autocomplete-richlistbox, I'm not sure if that is used anywhere in content but it would be great if we could target it specifically for the URL bar just to be safe.

The search bar popup is inside the #PopupAutoComplete panel, but that is also used for in-content autocomplete popups, like here: http://codepen.io/michelmarrache/pen/JEGhr.  I'd like to know if there is a way to distinguish between the two using a selector, because the popup seems to be in the same place in the DOM for both.  Maybe there is an attribute for which element it is attached to?  I'm not seeing anything but I could be missing it.

Comment 112

[:Gijs (out for now; he/him)]

Comment on attachment 8507636 [details] [diff] [review]
devedition-part3.patch

Dunno why this flag wasn't set yet...

Comment 113

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #108)
> ::: toolkit/themes/osx/global/autocomplete.css
> @@ +8,5 @@
> > +/* XXX: can't use the :root selector for some reason
> > +(the variables will not apply in selectors like
> > +.autocomplete-treebody::-moz-tree-row otherwise).
> > +*/
> > +.autocomplete-treebody {
> 
> As Dão pointed out, this is used pretty widely. Generally, we use
> platform-dependent colors there (I think?). I can point you to a number of
> places where OS themes make websites not display nicely if you use a dark
> theme, because the OS colors don't match the site, or the site sets only
> foreground/background and not the other.
> 
> I'd therefore prefer to have this stuff specific to the browser chrome
> through judicious use of selectors. We could technically use variables here
> and then only set the variables differently in the devedition theme for
> other selectors, I think? However, at that point I wonder if it isn't more
> sensible / less code to just override the colors for those autocomplete
> popups (for which we're already having to use more specific selectors)
> without the use of variables.
> 
> Does that make sense and/or do you think we should also be styling content
> autocomplete boxes?

The case where it would make sense to keep variables would be if the implementations were different between all three platforms (so we would need to have multiple duplicated selectors).  But I think they are using pretty much the same selectors in all three autocomplete.css files.  I can revert changes to autocomplete.css and instead add a change to the binding that will allow us to target certain popups more specifically:

#PopupAutoCompleteRichResult[autocompleteInput="urlbar"],
#PopupAutoComplete[autocompleteInput="searchbar-textbox"] {
  /* Only target the two autocompletes we care about */
}

Then add these selectors in devedition.css in the shared patch as suggested.

Comment 114

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #113)
> (In reply to :Gijs Kruitbosch from comment #108)
> > ::: toolkit/themes/osx/global/autocomplete.css
> > @@ +8,5 @@
> > > +/* XXX: can't use the :root selector for some reason
> > > +(the variables will not apply in selectors like
> > > +.autocomplete-treebody::-moz-tree-row otherwise).
> > > +*/
> > > +.autocomplete-treebody {
> > 
> > As Dão pointed out, this is used pretty widely. Generally, we use
> > platform-dependent colors there (I think?). I can point you to a number of
> > places where OS themes make websites not display nicely if you use a dark
> > theme, because the OS colors don't match the site, or the site sets only
> > foreground/background and not the other.
> > 
> > I'd therefore prefer to have this stuff specific to the browser chrome
> > through judicious use of selectors. We could technically use variables here
> > and then only set the variables differently in the devedition theme for
> > other selectors, I think? However, at that point I wonder if it isn't more
> > sensible / less code to just override the colors for those autocomplete
> > popups (for which we're already having to use more specific selectors)
> > without the use of variables.
> > 
> > Does that make sense and/or do you think we should also be styling content
> > autocomplete boxes?
> 
> The case where it would make sense to keep variables would be if the
> implementations were different between all three platforms (so we would need
> to have multiple duplicated selectors).  But I think they are using pretty
> much the same selectors in all three autocomplete.css files.  I can revert
> changes to autocomplete.css and instead add a change to the binding that
> will allow us to target certain popups more specifically:
> 
> #PopupAutoCompleteRichResult[autocompleteInput="urlbar"],
> #PopupAutoComplete[autocompleteInput="searchbar-textbox"] {
>   /* Only target the two autocompletes we care about */
> }
> 
> Then add these selectors in devedition.css in the shared patch as suggested.

Sounds good! :-)

Comment 115

[Brian Grinstead [:bgrins]]

Attachment: devedition-osx-variables.patch

Fixed nits and removed the changes to autocomplete.css, so carrying over r+

Comment 116

[Paul Rouget [:paul]]

Brian, I'm trying to get the back/fwd arrows to work properly on Linux, but I'm not sure to understand how it's supposed to work. It doesn't appear to work on osx for example.

Comment 117

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #116)
> Brian, I'm trying to get the back/fwd arrows to work properly on Linux, but
> I'm not sure to understand how it's supposed to work.  It doesn't appear to
> work on osx for example.

Two notes about the linux changes:

1) For the back button, the changes for OSX are in the part 2 shared patch in browser/themes/osx/devedition.css.  I believe that it's done differently across platforms, but setting border-radius: 0 and background-color: #252C33 should be a start.  I'm guessing that we won't be able to variable-ify that part of it, so if you can make the changes to that file and include it in the linux patch I'll split it out and put it back into the correct place.
2) Don't worry about any changes to autocomplete.css, those will be overridden in devedition.inc.css.

I'll double check with the back button on OSX, but it seems to work for me.  Could be an issue with retina or Yosemite if you are using that.

Comment 118

[Tim Nguyen :ntim]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #116)
> Brian, I'm trying to get the back/fwd arrows to work properly on Linux, but
> I'm not sure to understand how it's supposed to work. It doesn't appear to
> work on osx for example.

On OSX, you need to target #back-button itself.

While on Linux and Windows, you need to target #back-button > .toolbarbutton-icon

Comment 119

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Going to try and split this up into two pieces to make it easier to work with.  The first one is the changes to the window, nav bar, toolbar buttons, etc.  The second patch will be all of the secondary UI for menus and panels.

Comment 120

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

Comment 121

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

In order for the QA Team to test this I need someone to give me access to bug dependencies. I will then add the people that will cover this on the QA side.

Comment 122

[Paul Rouget [:paul]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #121)
> In order for the QA Team to test this I need someone to give me access to
> bug dependencies. I will then add the people that will cover this on the QA
> side.

Done.

Comment 123

[Paul Rouget [:paul]]

Attachment: linux addendum

(This is not ready for review)

This fixes most of the issues on Linux. These are rather small changes. What's missing is text and separator colors in panels.

I'm not sure I should keep all the changes in linux/devedition.css. There are probably better places to make these changes. Brian, let me know.

Comment 124

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #122)
> (In reply to Florin Mezei, QA (:FlorinMezei) from comment #121)
> > In order for the QA Team to test this I need someone to give me access to
> > bug dependencies. I will then add the people that will cover this on the QA
> > side.
> 
> Done.

Thank you Paul, but I'm afraid there's still bug 1082108 to which I do not have access. Could you please double check this? Thank you again for the very quick response!

Comment 125

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Gijs, here is the primary patch for review.  If you apply one of the platform patches in front of this you should see the devedition UI for everything except the panels and menus.

Windows still needs some work especially with regard to what elements should be dark in maximized windows vs not, so I'd love any feedback related to that.

Comment 126

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

Moved all new secondary images into this patch, and folded in the platform changes required to be in */devedition.css.

Comment 127

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

This should be ready for review.
Also there seem to be some very specific aero styles here (such as the bookmarks menu-button states), not sure if it's worth making variables. But either way, it works fine with the devedition shared styles.

Comment 128

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

Comment 129

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #125)
> Created attachment 8509838 [details] [diff] [review]
> devedition-primary.patch
> 
> Gijs, here is the primary patch for review.  If you apply one of the
> platform patches in front of this you should see the devedition UI for
> everything except the panels and menus.
> 
> Windows still needs some work especially with regard to what elements should
> be dark in maximized windows vs not, so I'd love any feedback related to
> that.

I'm actually a little confused... AIUI, the OS X bits to prep for this have landed on Nightly, but not the Linux or Windows bits, right? Can we split things up so that we deal with this platform-by-platform, and particularly, so we don't land/review/do dev theme stuff for platforms where the prep stuff hasn't landed/been reviewed/been done yet?

IOW, can I just review the OS X bit for now and/or review the Windows prep bits (which AIUI are a different, not-yet-landed patch)? :-)

Having looked for a bit... where do the search icons come from? Do we have a bug on unifying this instead of just overriding it? In theory, we should be able to use the text color and use a single SVG in all themes. I'm also confused on how this works with the light theme, or is this just the dark one for now - the colors look hardcoded?

From a casual look, it also seems this uses rather a lot of blanket !important (e.g. back button on Windows) which I should hope isn't necessary, and there's some unfinished comments in there ( "+/* Toolbar hover should be back to default when the */")

Comment 130

[Tim Nguyen :ntim]

Comment on attachment 8509838 [details] [diff] [review]
devedition-primary.patch

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

::: browser/themes/windows/devedition.css
@@ +30,5 @@
> +  padding-top: 2px !important;
> +  padding-bottom: 2px  !important;
> +}
> +
> +/* Don't use the dark background color unless if the window is maximized */

I'm not sure about this as it makes the tabbar look very strange, especially when the tab scroll arrows appear. I'd rather keep those aero border in this case.

Comment 131

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

Fixed a small nit

Comment 132

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #129)
> (In reply to Brian Grinstead [:bgrins] from comment #125)
> > Created attachment 8509838 [details] [diff] [review]
> > devedition-primary.patch
> > 
> > Gijs, here is the primary patch for review.  If you apply one of the
> > platform patches in front of this you should see the devedition UI for
> > everything except the panels and menus.
> > 
> > Windows still needs some work especially with regard to what elements should
> > be dark in maximized windows vs not, so I'd love any feedback related to
> > that.
> 
> I'm actually a little confused... AIUI, the OS X bits to prep for this have
> landed on Nightly, but not the Linux or Windows bits, right? Can we split
> things up so that we deal with this platform-by-platform, and particularly,
> so we don't land/review/do dev theme stuff for platforms where the prep
> stuff hasn't landed/been reviewed/been done yet?
> 

The OS X bits haven't landed yet, but I could do that now (it's r+ but I wasn't sure if we should land it ahead of time or wait until the others were ready).  Do you think I should split that patch up to include just the variables that are needed for the primary changes, or leave it to include the variables needed for secondary changes as well?  FWIW the initial linux/windows patches will *not* include the secondary variables (hopefully we can handle the entire secondary UI in a follow up bug).

> IOW, can I just review the OS X bit for now and/or review the Windows prep
> bits (which AIUI are a different, not-yet-landed patch)? :-)

Sorry that this is confusing - the changes in the separate patches (window/linux/osx) do not include any visual changes at all, just adding variables.  Changes in this patch to windows/devedition.css, linux/devedition.css, and osx/devedition.css etc should be reviewable separately from the variable patches.  Would it make things easier I filed a different bug for these variable patches?

Comment 133

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #129)
> Having looked for a bit... where do the search icons come from? Do we have a
> bug on unifying this instead of just overriding it? In theory, we should be
> able to use the text color and use a single SVG in all themes. I'm also
> confused on how this works with the light theme, or is this just the dark
> one for now - the colors look hardcoded?

So, there are no inverted search icons in the tree right now.  We have this new Search.svg file that handles all the different variations that we may need for dev edition (inverted and non-inverted in both forward facing and backward facing variations).  All are toggleable via a URL fragment.  So you can load any of these:

chrome://browser/skin/devedition/Search.svg#search-icon
chrome://browser/skin/devedition/Search.svg#search-icon-inverted
chrome://browser/skin/devedition/Search.svg#search-icon-mac-inverted
chrome://browser/skin/devedition/Search.svg#search-icon-mac

I asked Stephen if he wanted to instead provide separate search-inverted.png icons for each platform, but he indicated we could stick with the SVG file in the patch.  I'm sure he could provide those images if it would be better for organization.

Comment 134

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #123)
> Created attachment 8509368 [details] [diff] [review]
> linux addendum
> 
> (This is not ready for review)
> 
> This fixes most of the issues on Linux. These are rather small changes.
> What's missing is text and separator colors in panels.
> 
> I'm not sure I should keep all the changes in linux/devedition.css. There
> are probably better places to make these changes. Brian, let me know.

Thanks, I folded these changes into the primary and secondary patches.  Did you make any updates to the variables patch (attachment 8502514 [details] [diff] [review]) or have you been working only on the linux/devedition.css file?

Comment 135

[Brian Grinstead [:bgrins]]

Comment on attachment 8509838 [details] [diff] [review]
devedition-primary.patch

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

Maybe this should have been a feedback request instead

Comment 136

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #132)
> (In reply to :Gijs Kruitbosch from comment #129)
> > (In reply to Brian Grinstead [:bgrins] from comment #125)
> > > Created attachment 8509838 [details] [diff] [review]
> > > devedition-primary.patch
> > > 
> > > Gijs, here is the primary patch for review.  If you apply one of the
> > > platform patches in front of this you should see the devedition UI for
> > > everything except the panels and menus.
> > > 
> > > Windows still needs some work especially with regard to what elements should
> > > be dark in maximized windows vs not, so I'd love any feedback related to
> > > that.
> > 
> > I'm actually a little confused... AIUI, the OS X bits to prep for this have
> > landed on Nightly, but not the Linux or Windows bits, right? Can we split
> > things up so that we deal with this platform-by-platform, and particularly,
> > so we don't land/review/do dev theme stuff for platforms where the prep
> > stuff hasn't landed/been reviewed/been done yet?
> > 
> 
> The OS X bits haven't landed yet, but I could do that now (it's r+ but I
> wasn't sure if we should land it ahead of time or wait until the others were
> ready).  Do you think I should split that patch up to include just the
> variables that are needed for the primary changes, or leave it to include
> the variables needed for secondary changes as well?  FWIW the initial
> linux/windows patches will *not* include the secondary variables (hopefully
> we can handle the entire secondary UI in a follow up bug).

I think it makes sense to land the r+'d and ready variable changes now, mostly to get more test coverage / baking on nightly. If that includes secondary UI, that sounds fine to me. It'll help avoid bitrot, too.

> > IOW, can I just review the OS X bit for now and/or review the Windows prep
> > bits (which AIUI are a different, not-yet-landed patch)? :-)
> 
> Sorry that this is confusing - the changes in the separate patches
> (window/linux/osx) do not include any visual changes at all, just adding
> variables.  Changes in this patch to windows/devedition.css,
> linux/devedition.css, and osx/devedition.css etc should be reviewable
> separately from the variable patches.  Would it make things easier I filed a
> different bug for these variable patches?

I think I'd prefer to do this per-platform. Is there some reason that's not possible? I guess some things are shared; I don't think it's the end of the world if we land things that look temporarily not-great on platforms that haven't been reviewed/completed yet - it's all behind a pref anyway.

I don't really mind having it in this bug, it just felt weird getting an r? for a patch that depended on another patch that wasn't even on the bug (I mid-aired with Tim attaching the latest version). :-)

Comment 137

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/a890f2283b74

Comment 138

[:Gijs (out for now; he/him)]

Comment on attachment 8509838 [details] [diff] [review]
devedition-primary.patch

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

I'm only looking at mac so far, because it's easier to test considering the variable bits have been reviewed already, and because there's a lot of stuff to delve into there that applies to the other themes anyway. Don't get me wrong, it generally looks pretty good already, but getting this stuff right is hard (see also: http://www.gijsk.com/blog/2014/04/why-doing-visual-refreshes-of-firefox-is-hard/ - there are a couple of points here that don't apply, like lwt, but still a heck of a lot that do). f+ on the mac/shared bits for perseverance! :-)

General notes (more below):

- this seems to unconditionally apply even when I select the light theme in devtools. My thinking was that we could continue to use the default theme there for now? Is that difficult to implement? :-)
- I don't see secondary UI being affected (dropdowns, panels, etc.). Is that right or should I be seeing this? I've only applied the r+'d mac patch and this one, perhaps I'm missing something?
- the fullscreen button almost isn't visible anymore. If we get an asset for it, you could override the -moz-mac-fullscreen-button appearance and use an image and some sizing instead, so you can actually see it clearly. :-)
- the auto-hiding of the forward button is a little broken: the button disappears completely instead of just getting disabled. The sliding away seems to work as expected.
- the back button's icon is still bigger than the forward button's, even though the buttons are the same size. There's a different icon at the 'next' spot in the grid that matches; you should be able to use that.
- there's an odd white line at the top of the window in fullscreen mode when the titlebar is disabled (default); not sure if that's fixable.
- toolbarbutton colors are opaque, instead of rgba/hsla, so now when you put toolbar buttons on the tabs toolbar, their hover effect isn't visible at all anymore
- there is some kind of weird interplay with switching in and out of the theme in customize mode and pinned tabs / overflowed tabs toolbar. STR are something like:
  * start with clean profile
  * create enough tabs to overflow, and one pinned tab
  * enable devtools theme in about:config
  * go into customize mode
  * click restore to defaults
  note how now the pinned tab and the button for the tab overflow overlap. The same happens if you e.g. use the lwt switcher in customize mode.
- the 'active' download icon is misaligned or the wrong size
- PopupNotification anchors (e.g. geolocation/addon/password prompts) in the url bar still have a white background. I don't know how well the icons will work on a black background... that last part might just want to be a followup if they don't work well.

::: browser/themes/osx/devedition.css
@@ +4,5 @@
>  
>  %include ../shared/devedition.inc.css
> +
> +/* No extra vertical padding for nav bar */
> +#nav-bar-customization-target {

Not sure why this is mac-only?

@@ +12,5 @@
> +
> +/* Resize things so that the native titlebar is in line with the tabs */
> +#main-window[tabsintitlebar] > #titlebar > #titlebar-content > #titlebar-buttonbox-container,
> +#main-window[tabsintitlebar] > #titlebar > #titlebar-content > #titlebar-secondary-buttonbox > #titlebar-fullscreen-button {
> +  margin-top: 6px;

In principle, the tabsintitlebar code should take care of this, IIRC? Can you elaborate why this was necessary?

::: browser/themes/shared/devedition.inc.css
@@ +23,5 @@
> +                                     0 8px 3px -5px #2b82bf inset,
> +                                     0 -2px 0 rgba(0,0,0,.2) inset;
> +
> +  /* Toolbar buttons */
> +  --toolbarbutton-hover-background: #192126 linear-gradient(#192126, #192126) padding-box;

I don't know how well graphics optimizes flat linear gradients these days, but can't we just set background-image: none? Or is this all for the purpose of transitioning the background-image on hover? If so, it'd benefit from a comment, or perhaps we should just ensure background-color is transitioned instead... :-)

@@ +26,5 @@
> +  /* Toolbar buttons */
> +  --toolbarbutton-hover-background: #192126 linear-gradient(#192126, #192126) padding-box;
> +  --toolbarbutton-hover-boxshadow: none;
> +  --toolbarbutton-hover-bordercolor: transparent;
> +  --toolbarbutton-active-background: #192126;

I don't think the active state should be indistinguishable (ie not have feedback) from the hover state. On OS X, I do see a change in size of the button, which looks a little weird. Is that intentional?

@@ +77,5 @@
> +  padding-right: 0;
> +}
> +
> +/* Change the base colors for the browser chrome */
> +#navigator-toolbox::after,

This is a border, at least on Windows, and I don't think setting background/color here makes sense.

@@ +88,5 @@
> +  background: var(--chrome-background-color);
> +  color: var(--chrome-color);
> +}
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),

The menubar isn't styled anywhere that I can see, is that intentional?

@@ +90,5 @@
> +}
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> +#browser-bottombox {
> +  background: var(--chrome-secondary-background-color) !important;

Why does this need to be important?

Also, on OS X I immediately noticed that the separator to the left of the menu button looks silly now; the gradients probably need to be swapped so it's lighter on the inside than on the outside, for the dark theme anyway.

@@ +98,5 @@
> +#navigator-toolbox > toolbar > .toolbarbutton-text {
> +  color: var(--chrome-color);
> +}
> +
> +#TabsToolbar:not(:-moz-lwtheme) {

Why :not(:-moz-lwtheme) ? This theme is never applied for lwthemes, right?

Note that the bookmarks toolbar on OS X still has an ugly text shadow on the bookmarks there.

@@ +102,5 @@
> +#TabsToolbar:not(:-moz-lwtheme) {
> +  text-shadow: none;
> +}
> +
> +#TabsToolbar::after {

I think this is the navbar/tabstoolbar border?

Instead of hiding it, can you set content: none? Then there shouldn't be an ::after at all.

Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap between the overflow button separator on overflowing tabs, and the navbar.

@@ +112,5 @@
> +  background-color: var(--url-and-searchbar-background-color) !important;
> +  background-image: none !important;
> +  color: var(--url-and-searchbar-color);
> +  border: none !important;
> +  box-shadow: none !important;

More !important sprinkling. I'm not sure how I feel about this - on the one hand, I guess this keeps the selectors simpler, but on the other, without delving into this too deeply, it seems pretty random, which things get !important, and which things don't. Here, for instance, the background color does but the foreground doesn't? That is peculiar. :-)

@@ +139,5 @@
> +
> +/* Don't use default colors when tabs are not in titlebar */
> +#main-window:not([customizing]) #navigator-toolbox[inFullscreen] > #TabsToolbar:not(:-moz-lwtheme),
> +#main-window:not(:-moz-any([customizing],[tabsintitlebar])) #navigator-toolbox > #TabsToolbar:not(:-moz-lwtheme) {
> +  -moz-appearance: none !important;

Both :not(:-moz-lwtheme) and !important?

@@ +143,5 @@
> +  -moz-appearance: none !important;
> +}
> +
> +#forward-button[disabled] {
> +  visibility: hidden !important;

Ah, so this is what does away with the forward button - why?

@@ +154,5 @@
> +}
> +
> +/* Tab styling */
> +.tab-close-button:not(:hover) {
> +  -moz-image-region: rect(0, 64px, 16px, 48px);

I would have thought this would be covered by the brighttext stuff?

@@ +164,5 @@
> +}
> +
> +.tab-background-middle[selected] {
> +  background-position: 0 0, 0 0px !important;
> +  background-image: url(chrome://browser/skin/tabbrowser/tab-active-middle.png),

Do you really need the png here? You're writing all over most of it with the linear gradient, AFAICT.

@@ +175,5 @@
> +  background-image: linear-gradient(#9FDFFF 0px, rgba(159,223,255,1) 2px, transparent 7px, transparent) !important
> +}
> +
> +.tab-background {
> +  visibility: hidden;

In fact, tab-background-middle is inside tab-background, so it looks like that rule is not used... same for tab-background-fill, which controls the background of the start/end svgs, which are also inside tab-background, and therefore also hidden?

@@ +225,5 @@
> +}
> +
> +#TabsToolbar > #new-tab-button:hover > .toolbarbutton-icon,
> +.tabs-newtab-button > .toolbarbutton-icon {
> +  padding: 0;

Why is this necessary only on hover for the overflowed case, but always for the non-overflow case?

@@ +230,5 @@
> +}
> +
> +.tabs-newtab-button:hover,
> +#TabsToolbar > #new-tab-button:hover {
> +  /* Important needed because it is set in browser.css because of bug 561154 */

Nit: I don't see how the browser.css thing matters; if the selector uses :-moz-any(), though...

Comment 139

[Brian Grinstead [:bgrins]]

Comment on attachment 8509928 [details] [diff] [review]
devedition-part4-windows.patch

Forwarding request to Gijs, who has more context here

Comment 140

[:Gijs (out for now; he/him)]

More notes:

- the url bar is too high compared to the search bar (they should remain the same height)
- the url bar has no margin underneath it, but 2-3px above it. Those margins should be symmetrical.

Comment 141

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

rebased

Comment 142

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #138)
> Comment on attachment 8509838 [details] [diff] [review]
> devedition-primary.patch
> 
> Review of attachment 8509838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm only looking at mac so far, because it's easier to test considering the
> variable bits have been reviewed already, and because there's a lot of stuff
> to delve into there that applies to the other themes anyway. Don't get me
> wrong, it generally looks pretty good already, but getting this stuff right
> is hard (see also:
> http://www.gijsk.com/blog/2014/04/why-doing-visual-refreshes-of-firefox-is-
> hard/ - there are a couple of points here that don't apply, like lwt, but
> still a heck of a lot that do). f+ on the mac/shared bits for perseverance!
> :-)

Thanks, this is really helpful (I commented on each point inline).  A new patch is incoming.

> 
> General notes (more below):
> 
> - this seems to unconditionally apply even when I select the light theme in
> devtools. My thinking was that we could continue to use the default theme
> there for now? Is that difficult to implement? :-)

That'd be easy to implement, pending a UX decision about it.

> - I don't see secondary UI being affected (dropdowns, panels, etc.). Is that
> right or should I be seeing this? I've only applied the r+'d mac patch and
> this one, perhaps I'm missing something?

That's right, the panels and menus are all in the devedition-secondary.patch file (which I hope to get reviewed/landed separately, since this is big enough as it is!)

> - the fullscreen button almost isn't visible anymore. If we get an asset for
> it, you could override the -moz-mac-fullscreen-button appearance and use an
> image and some sizing instead, so you can actually see it clearly. :-)

Bug 1018845 is opened for that.  I got the impression from https://bugzilla.mozilla.org/show_bug.cgi?id=1018845#c9 that it requires more than just CSS.

> - the auto-hiding of the forward button is a little broken: the button
> disappears completely instead of just getting disabled. The sliding away
> seems to work as expected.

So it's using visibility:hidden because the current clip-path is wrong (it is a circle to cover the original back button).  I updated the patch to use a new rectangular clip-path which seems to work, but I don't really know what's going on with that so I'll definitely need another set of eyes.

> - the back button's icon is still bigger than the forward button's, even
> though the buttons are the same size. There's a different icon at the 'next'
> spot in the grid that matches; you should be able to use that.

Done - switched that for 1px 'normal' state in devedition.inc.css and 1x 'active' state along with 2x normal/active in osx/devedition.css.

> - there's an odd white line at the top of the window in fullscreen mode when
> the titlebar is disabled (default); not sure if that's fixable.

Not sure yet - I'll follow up with that.

> - toolbarbutton colors are opaque, instead of rgba/hsla, so now when you put
> toolbar buttons on the tabs toolbar, their hover effect isn't visible at all
> anymore

OK, I've added some opacity but there still isn't much contrast in this case.

> - there is some kind of weird interplay with switching in and out of the
> theme in customize mode and pinned tabs / overflowed tabs toolbar. STR are
> something like:
>   * start with clean profile
>   * create enough tabs to overflow, and one pinned tab
>   * enable devtools theme in about:config
>   * go into customize mode
>   * click restore to defaults
>   note how now the pinned tab and the button for the tab overflow overlap.
> The same happens if you e.g. use the lwt switcher in customize mode.

Also going to follow up with that.

> - the 'active' download icon is misaligned or the wrong size

Maybe is just that download-glow.png icon looking funny with the dark background?  I'm not really seeing it becoming misaligned.  Do you see the same issue on a dark lw theme?

> - PopupNotification anchors (e.g. geolocation/addon/password prompts) in the
> url bar still have a white background. I don't know how well the icons will
> work on a black background... that last part might just want to be a
> followup if they don't work well.

Yes, this would probably be best in the secondary patch or not a follow up bug (for future reference: https://www.dropbox.com/s/m3wko1zf6homi0p/Screenshot%202014-10-23%2011.13.48.png?dl=0).

> ::: browser/themes/osx/devedition.css
> @@ +4,5 @@
> >  
> >  %include ../shared/devedition.inc.css
> > +
> > +/* No extra vertical padding for nav bar */
> > +#nav-bar-customization-target {
> 
> Not sure why this is mac-only?
> 

Moved into shared file.

> @@ +12,5 @@
> > +
> > +/* Resize things so that the native titlebar is in line with the tabs */
> > +#main-window[tabsintitlebar] > #titlebar > #titlebar-content > #titlebar-buttonbox-container,
> > +#main-window[tabsintitlebar] > #titlebar > #titlebar-content > #titlebar-secondary-buttonbox > #titlebar-fullscreen-button {
> > +  margin-top: 6px;
> 
> In principle, the tabsintitlebar code should take care of this, IIRC? Can
> you elaborate why this was necessary?
> 

Taking this out causes the window resize buttons / fullscreen buttons to show up pretty low: https://www.dropbox.com/s/0tpv214h5rzvqw2/Screenshot%202014-10-23%2011.18.57.png?dl=0.

This also reminded me that I should change the shared CSS to only add the 50px left/right spacing when tabs are in the titlebar.

> ::: browser/themes/shared/devedition.inc.css
> @@ +23,5 @@
> > +                                     0 8px 3px -5px #2b82bf inset,
> > +                                     0 -2px 0 rgba(0,0,0,.2) inset;
> > +
> > +  /* Toolbar buttons */
> > +  --toolbarbutton-hover-background: #192126 linear-gradient(#192126, #192126) padding-box;
> 
> I don't know how well graphics optimizes flat linear gradients these days,
> but can't we just set background-image: none? Or is this all for the purpose
> of transitioning the background-image on hover? If so, it'd benefit from a
> comment, or perhaps we should just ensure background-color is transitioned
> instead... :-)
> 

Switched to using none for the image.

> @@ +26,5 @@
> > +  /* Toolbar buttons */
> > +  --toolbarbutton-hover-background: #192126 linear-gradient(#192126, #192126) padding-box;
> > +  --toolbarbutton-hover-boxshadow: none;
> > +  --toolbarbutton-hover-bordercolor: transparent;
> > +  --toolbarbutton-active-background: #192126;
> 
> I don't think the active state should be indistinguishable (ie not have
> feedback) from the hover state. On OS X, I do see a change in size of the
> button, which looks a little weird. Is that intentional?
> 

No, the size change isn't intentional.  Looks like it is because the hover border color is transparent but the active border color is hsla(0,0%,0%,.3) and not variable-ified yet (oops).  Also, I'm happy to change the active state's bg color (and I'll do so using alpha values so we can just lighten up the hover state and darken the active).

> @@ +77,5 @@
> > +  padding-right: 0;
> > +}
> > +
> > +/* Change the base colors for the browser chrome */
> > +#navigator-toolbox::after,
> 
> This is a border, at least on Windows, and I don't think setting
> background/color here makes sense.
> 

It is used as a border but it is styled with background-color.  Although now that you mention it, this is the wrong color for it, which explains why there was only 1px below the URL bar.  Targetting #navigator-toolbox::after only and switching the bg to --chrome-secondary-background-color fixes that issue as well.

> @@ +88,5 @@
> > +  background: var(--chrome-background-color);
> > +  color: var(--chrome-color);
> > +}
> > +
> > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> 
> The menubar isn't styled anywhere that I can see, is that intentional?
> 

I had copied another selector, but I guess it doesn't matter since the menubar doesn't have moz-appearance: none.  Removed that from the selector.

> @@ +90,5 @@
> > +}
> > +
> > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> > +#browser-bottombox {
> > +  background: var(--chrome-secondary-background-color) !important;
> 
> Why does this need to be important?

Unfortunately there is a selector like #PersonalToolbar:-moz-window-inactive:not(:-moz-lwtheme), #nav-bar:-moz-window-inactive:not(:-moz-lwtheme) (http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#195) that sets the background for inactive windows.  We could either copy that selector over, keep the !important, or turn this into a variable.

> Also, on OS X I immediately noticed that the separator to the left of the
> menu button looks silly now; the gradients probably need to be swapped so
> it's lighter on the inside than on the outside, for the dark theme anyway.
> 

Ah, I had moved that part to the secondary patch on accident.  I folded that back into the primary.

> @@ +98,5 @@
> > +#navigator-toolbox > toolbar > .toolbarbutton-text {
> > +  color: var(--chrome-color);
> > +}
> > +
> > +#TabsToolbar:not(:-moz-lwtheme) {
> 
> Why :not(:-moz-lwtheme) ? This theme is never applied for lwthemes, right?
> 

Right, it's for specificity.  I think this could be converted into a variable (but I didn't b/c it's currently a preprocessor directive @loweredShadow@ that's used all over the place).

> Note that the bookmarks toolbar on OS X still has an ugly text shadow on the
> bookmarks there.
> 

I moved the text-shadow setting from .toolbarbutton-1 to #navigator-toolbox toolbarbutton.

> @@ +102,5 @@
> > +#TabsToolbar:not(:-moz-lwtheme) {
> > +  text-shadow: none;
> > +}
> > +
> > +#TabsToolbar::after {
> 
> I think this is the navbar/tabstoolbar border?
> 
> Instead of hiding it, can you set content: none? Then there shouldn't be an
> ::after at all.
> 
> Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap
> between the overflow button separator on overflowing tabs, and the navbar.
> 

After removing that line altogther I'm not even seeing what it is applying to.  I've changed it to content: none, but I think I'll have to follow up with you to figure out exactly what you mean here.

> @@ +112,5 @@
> > +  background-color: var(--url-and-searchbar-background-color) !important;
> > +  background-image: none !important;
> > +  color: var(--url-and-searchbar-color);
> > +  border: none !important;
> > +  box-shadow: none !important;
> 
> More !important sprinkling. I'm not sure how I feel about this - on the one
> hand, I guess this keeps the selectors simpler, but on the other, without
> delving into this too deeply, it seems pretty random, which things get
> !important, and which things don't. Here, for instance, the background color
> does but the foreground doesn't? That is peculiar. :-)

Going to remove !important wherever possible.

> @@ +139,5 @@
> > +
> > +/* Don't use default colors when tabs are not in titlebar */
> > +#main-window:not([customizing]) #navigator-toolbox[inFullscreen] > #TabsToolbar:not(:-moz-lwtheme),
> > +#main-window:not(:-moz-any([customizing],[tabsintitlebar])) #navigator-toolbox > #TabsToolbar:not(:-moz-lwtheme) {
> > +  -moz-appearance: none !important;
> 
> Both :not(:-moz-lwtheme) and !important?
> 

Yes, just to be triple sure that it applies ;).  No, just kidding, that's terrible.  I've got rid of the important and moved it to the osx/devedition.css file, using only the fullscreen selector since this is the only time I've seen the issue (it fixes this issue when in full screen: https://www.dropbox.com/s/gpzfeoclujcbjmm/Screenshot%202014-10-23%2012.29.28.png?dl=0).

> @@ +143,5 @@
> > +  -moz-appearance: none !important;
> > +}
> > +
> > +#forward-button[disabled] {
> > +  visibility: hidden !important;
> 
> Ah, so this is what does away with the forward button - why?
> 

See comment above about clip-path.

> @@ +154,5 @@
> > +}
> > +
> > +/* Tab styling */
> > +.tab-close-button:not(:hover) {
> > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> 
> I would have thought this would be covered by the brighttext stuff?
> 

The brighttext stuff only gets :not([selected=true]):not(:hover).  So for the selected tab it looks weird without this: https://www.dropbox.com/s/p23slmv80vqjpeq/Screenshot%202014-10-23%2012.32.21.png?dl=0.  I'll update the selector and add a comment to explain.

> @@ +164,5 @@
> > +}
> > +
> > +.tab-background-middle[selected] {
> > +  background-position: 0 0, 0 0px !important;
> > +  background-image: url(chrome://browser/skin/tabbrowser/tab-active-middle.png),
> 
> Do you really need the png here? You're writing all over most of it with the
> linear gradient, AFAICT.
> 
> @@ +175,5 @@
> > +  background-image: linear-gradient(#9FDFFF 0px, rgba(159,223,255,1) 2px, transparent 7px, transparent) !important
> > +}
> > +
> > +.tab-background {
> > +  visibility: hidden;
> 
> In fact, tab-background-middle is inside tab-background, so it looks like
> that rule is not used... same for tab-background-fill, which controls the
> background of the start/end svgs, which are also inside tab-background, and
> therefore also hidden?
> 

Well, that's easier :), just removed the above rules.

> @@ +225,5 @@
> > +}
> > +
> > +#TabsToolbar > #new-tab-button:hover > .toolbarbutton-icon,
> > +.tabs-newtab-button > .toolbarbutton-icon {
> > +  padding: 0;
> 
> Why is this necessary only on hover for the overflowed case, but always for
> the non-overflow case?
> 

I can't remember exactly, that looks like a typo.  I'm not actually seeing the case where that should matter, so I'm going to remove it for now.

> @@ +230,5 @@
> > +}
> > +
> > +.tabs-newtab-button:hover,
> > +#TabsToolbar > #new-tab-button:hover {
> > +  /* Important needed because it is set in browser.css because of bug 561154 */
> 
> Nit: I don't see how the browser.css thing matters; if the selector uses
> :-moz-any(), though...

Updated comment to read: 'Important needed because !important is used in browser.css'.  This line in the one: http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3234.

Comment 143

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

This should address your points except where otherwise noted (see Comment 142).  If you would be able to give another round of feedback on OSX that would be very helpful.

Comment 144

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

rebased

Comment 145

[Tim Nguyen :ntim]

Comment on attachment 8510658 [details] [diff] [review]
devedition-secondary.patch

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

I know this is WIP, but I just wanted to give some comments so I can start working on Windows on a fresh base.

::: browser/themes/osx/jar.mn
@@ +339,5 @@
>    skin/classic/browser/devedition/urlbar-history-dropmarker.svg            (../shared/devedition/urlbar-history-dropmarker.svg)
> +  skin/classic/browser/devedition/panelarrow-horizontal.png     (../shared/devedition/panelarrow-horizontal.png)
> +  skin/classic/browser/devedition/[e-mail]  (../shared/devedition/[e-mail])
> +  skin/classic/browser/devedition/panelarrow-vertical.png       (../shared/devedition/panelarrow-vertical.png)
> +  skin/classic/browser/devedition/[e-mail]    (../shared/devedition/[e-mail])

Can we use SVG for these please ? Windows already uses SVG by default to support different DPI levels.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +31,5 @@
> +  --panel-toolbarbutton-hover-border-color: hsla(210,4%,10%,.11);
> +  --panel-toolbarbutton-active-background-color: hsla(210,4%,10%,.12);
> +  --panel-toolbarbutton-active-border-color: hsla(210,4%,10%,.14);
> +  --panel-subviews-border-color: hsla(210,4%,10%,.3);
> +  --panel-inactive-overlay-background-color: hsla(210,4%,10%,.1);

--panel-inactive-mainview-background-color would be more descriptive here.

@@ +34,5 @@
> +  --panel-subviews-border-color: hsla(210,4%,10%,.3);
> +  --panel-inactive-overlay-background-color: hsla(210,4%,10%,.1);
> +  --panel-separator-color: hsla(210,4%,10%,.15);
> +  --panel-separator-2-color: hsla(210,4%,10%,.14);
> +  --panel-separator-3-color: hsla(210,4%,10%,.1);

These names are not very descriptive :/

@@ +41,5 @@
> +  --menupanel-quit-list-style-image-2x: url(chrome://browser/skin/[e-mail]);
> +  --menupanel-help-list-style-image: url(chrome://browser/skin/menuPanel-help.png);
> +  --menupanel-help-list-style-image-2x: url(chrome://browser/skin/[e-mail]);
> +  --menupanel-customize-list-style-image: url(chrome://browser/skin/menuPanel-customize.png);
> +  --menupanel-customize-list-style-image-2x: url(chrome://browser/skin/[e-mail]);

Can we simplify those variable names ?
Maybe you could just remove -list-style- from the names.

@@ +795,5 @@
>  }
>  
>  menuitem.panel-subview-footer@menuStateHover@,
>  .subviewbutton.panel-subview-footer@buttonStateHover@ {
> +  background-color: var(--panel-separator-color);

Hmm, I'm not sure about this. The background-color might look too light with the devedition theme enabled.

::: browser/themes/shared/devedition.inc.css
@@ +270,5 @@
> +
> +/* XXX: Make a variable */
> +#downloadsFooter {
> +  background: var(--panel-footer-background-color) !important;
> +  box-shadow: none !important;

That box-shadow is actually the top border, I'm not sure if it's a good idea to remove it.

@@ +337,5 @@
> +}
> +
> +/* End Panels */
> +
> +/* XXX: do we need this? */

I don't think we want to have a very bright white background in the middle of a dark UI.

@@ +339,5 @@
> +/* End Panels */
> +
> +/* XXX: do we need this? */
> +#notification-popup-box {
> +  background-color: transparent !important;

Maybe an rgba value (rgba(255,255,255,0.5) maybe) will be enough to fix readability.

@@ +346,5 @@
> +#notification-popup-box {
> +  border-right-width: 1px;
> +  padding-right: 8px;
> +  margin-right: 0px;
> +  border-image: linear-gradient(transparent 15%, #5a6169 15%, #5a6169 85%, transparent 85%) 1 1 !important;

I originally changed the border image, since the original border image was an arrow with a white background, and that made the #notification-popup-box look weird.

But I'm guessing this could be changed using something simpler.

@@ +350,5 @@
> +  border-image: linear-gradient(transparent 15%, #5a6169 15%, #5a6169 85%, transparent 85%) 1 1 !important;
> +}
> +
> +#notification-popup-box > image:not([id*="sharing"]):not([id*="translated"]):not(.indexedDB-notification-icon):not(#indexedDB-notification-icon):not(.default-notification-icon):not(#default-notification-icon) {
> +  filter: brightness(10) opacity(0.8);

I guess this is not needed with an rgba() value as background.

@@ +356,5 @@
> +
> +.subviewbutton > .toolbarbutton-text {
> +  color: -moz-fieldText !important;
> +}
> +

Leftover from my userstyle (I originally added this since the CSS rules I applied for .toolbarbutton-text were browser wide).
Also, we should have a variable for the text color.

Comment 146

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a890f2283b74

Comment 147

[Dão Gottwald [:dao]]

Comment on attachment 8510657 [details] [diff] [review]
devedition-primary.patch

>+    <svg:clipPath id="urlbar-back-button-clip-path-square">
>+      <svg:rect x="0" y="0" width="10000" height="10000" />
>+    </svg:clipPath>

This makes no sense. It looks like you actually don't want any clip path at all.

>+/* Linux has a title bar. No need to have extra space to drag the window. */
>+#TabsToolbar {
>+  padding-left: 0;
>+  padding-right: 0;
>+}

Why is this needed? Why would #TabsToolbar have a left/right padding in the first place?

>+/* Include extra space on left/right for dragging since there is no space above
>+   the tabs */
>+#main-window[tabsintitlebar] #TabsToolbar {
>+  padding-left: 50px;
>+  padding-right: 50px;
>+  margin: 0;
>+}

What margin are you resetting here? margin-bottom? Please be explicit. And why is that margin specific to tabsintitlebar?

>--- a/browser/themes/shared/devedition.inc.css
>+++ b/browser/themes/shared/devedition.inc.css

>+#TabsToolbar:not(:-moz-lwtheme) {
>+  text-shadow: none;
>+}

What are you overriding here? Is this OS X specific?

>+#TabsToolbar::after {
>+  content: none;
>+}

This only exists on Windows. And why are you not using display:none?

>+.searchbar-textbox, #urlbar {

\n after ,

(In reply to Brian Grinstead [:bgrins] from comment #142)
> > > +#TabsToolbar:not(:-moz-lwtheme) {
> > 
> > Why :not(:-moz-lwtheme) ? This theme is never applied for lwthemes, right?
> > 
> 
> Right, it's for specificity.

Don't do that with selector modifications that don't otherwise make sense, it leads to code that nobody understands. Use !important if you have to.

Comment 148

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #147)
> Comment on attachment 8510657 [details] [diff] [review]
> devedition-primary.patch
> 
> >+    <svg:clipPath id="urlbar-back-button-clip-path-square">
> >+      <svg:rect x="0" y="0" width="10000" height="10000" />
> >+    </svg:clipPath>
> 
> This makes no sense. It looks like you actually don't want any clip path at
> all.
> 
> >+/* Linux has a title bar. No need to have extra space to drag the window. */
> >+#TabsToolbar {
> >+  padding-left: 0;
> >+  padding-right: 0;
> >+}
> 
> Why is this needed? Why would #TabsToolbar have a left/right padding in the
> first place?

Because of the following bit you quoted. Note that that used to be without the [tabsintitlebar] selector, so the linux reset can probably just go away now - tabsintitlebar is never set there.

> >+/* Include extra space on left/right for dragging since there is no space above
> >+   the tabs */
> >+#main-window[tabsintitlebar] #TabsToolbar {
> >+  padding-left: 50px;
> >+  padding-right: 50px;
> >+  margin: 0;
> >+}
> 
> <snip>
>
> >+#TabsToolbar::after {
> >+  content: none;
> >+}
> 
> This only exists on Windows. And why are you not using display:none?

Kind of because I just told him to use content: none in my previous comment...

Comment 149

[Dão Gottwald [:dao]]

> > >+/* Linux has a title bar. No need to have extra space to drag the window. */
> > >+#TabsToolbar {
> > >+  padding-left: 0;
> > >+  padding-right: 0;
> > >+}
> > 
> > Why is this needed? Why would #TabsToolbar have a left/right padding in the
> > first place?
> 
> Because of the following bit you quoted.

That was in browser/themes/osx/devedition.css.

> > >+#TabsToolbar::after {
> > >+  content: none;
> > >+}
> > 
> > This only exists on Windows. And why are you not using display:none?
> 
> Kind of because I just told him to use content: none in my previous
> comment...

This seems like a more indirect way of saying "I don't want this displayed". Why would that be preferable over display:none?

Comment 150

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #149)
> > > >+/* Linux has a title bar. No need to have extra space to drag the window. */
> > > >+#TabsToolbar {
> > > >+  padding-left: 0;
> > > >+  padding-right: 0;
> > > >+}
> > > 
> > > Why is this needed? Why would #TabsToolbar have a left/right padding in the
> > > first place?
> > 
> > Because of the following bit you quoted.
> 
> That was in browser/themes/osx/devedition.css.

Huh, that's surprising to me; the --space-above-tabbar definition is in shared, and that will affect Windows as well, AIUI. Brian?

> > > >+#TabsToolbar::after {
> > > >+  content: none;
> > > >+}
> > > 
> > > This only exists on Windows. And why are you not using display:none?
> > 
> > Kind of because I just told him to use content: none in my previous
> > comment...
> 
> This seems like a more indirect way of saying "I don't want this displayed".
> Why would that be preferable over display:none?

Because I expected that with display:none the ::after gc will still be in the node tree, whereas content: none should just make it actually go away - it seems that isn't the case.

Personally, I still think that because the content property is what makes an ::after/::before frame actually exist, content:none is at least as clear. In any case, YMMV, I won't lose sleep over switching this back to display:none if you feel strongly.

Comment 151

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #150)
> Because I expected that with display:none the ::after gc will still be in
> the node tree, whereas content: none should just make it actually go away -
> it seems that isn't the case.

To be clear: *both* display:none/content:none make the gc disappear from the node tree, which surprised me considering that's not what happens for normal display:none nodes. :-)

Comment 152

[:Gijs (out for now; he/him)]

Comment on attachment 8509928 [details] [diff] [review]
devedition-part4-windows.patch

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

::: browser/themes/windows/browser.css
@@ +25,5 @@
> +  --toolbarbutton-active-background: hsla(210,4%,10%,.12);
> +  --toolbarbutton-active-bordercolor: hsla(210,4%,10%,.2) transparent transparent;
> +  --toolbarbutton-active-boxshadow: 0 1px 0 0 hsla(210,4%,10%,.1) inset;
> +
> +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);

Nit: just use the literal here.

However, this only seems to be used on non-win8? What's up with that?

@@ +28,5 @@
> +
> +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);
> +
> +  --toolbarbutton-combined-boxshadow: none;
> +  --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 16px);

Can't this just be a flat background-color? Also, can we make this be ...combined-separator-backgroundcolor/boxshadow , as that's what it is?

And, if we want to change these, we should also be styling the separators of the zoom and clipboard buttons, which are in panelUIOverlay.css (but also affect the in-navbar styling)

@@ +702,5 @@
> +
> +    --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 18px);
> +    --toolbarbutton-combined-boxshadow: 0 0 0 1px hsla(0,0%,100%,.2);
> +  }
> +  

Nit: whitespace

@@ -727,3 @@
>  
>  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
>  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true])::before {

As far as I can tell this selector and its win8 cousin are dead. We only use this for the menubuttons, not for the combined buttons, where we just style the real separators. I don't see a ::before in the inspector for any of the cut/copy/paste and zoom in/reset/out buttons.

@@ -774,2 @@
>      background-size: 1px 18px;
> -    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);

Some of these look redundant, so maybe getting rid of them makes sense - but the box shadow isn't redundant, as far as I can tell?

@@ +840,5 @@
>  #nav-bar .toolbarbutton-1[open] > .toolbarbutton-menubutton-dropmarker:not([disabled=true]) > .dropmarker-icon,
>  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
>  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-text,
>  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-container {
> +  background: var(--toolbarbutton-active-background);

Can't this just stay background-color?

Comment 153

[:Gijs (out for now; he/him)]

Unfortunately, I won't have time to re-feedback the OS X changes before Monday (I can make it the first thing I do then, though!). If you need feedback before then, perhaps try dao or jaws? Sorry!

Comment 154

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #147)
> Comment on attachment 8510657 [details] [diff] [review]
> devedition-primary.patch
> 
> >+    <svg:clipPath id="urlbar-back-button-clip-path-square">
> >+      <svg:rect x="0" y="0" width="10000" height="10000" />
> >+    </svg:clipPath>
> 
> This makes no sense. It looks like you actually don't want any clip path at
> all.

I don't know, when I set it to clip-path:none this is what I see: https://www.dropbox.com/s/5cvcbwc7rst1qdw/Screenshot%202014-10-24%2008.05.10.png?dl=0 versus with it applied: https://www.dropbox.com/s/66kspxydjamutn5/Screenshot%202014-10-24%2008.07.55.png?dl=0.

Comment 155

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #147)
> Comment on attachment 8510657 [details] [diff] [review]
> devedition-primary.patch
> >+/* Include extra space on left/right for dragging since there is no space above
> >+   the tabs */
> >+#main-window[tabsintitlebar] #TabsToolbar {
> >+  padding-left: 50px;
> >+  padding-right: 50px;
> >+  margin: 0;
> >+}
> 
> What margin are you resetting here? margin-bottom? Please be explicit. And
> why is that margin specific to tabsintitlebar?
> 

Margin bottom - there is a 1px overlap in osx/browser.css: http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3036.  I've updated to be more specific and added a comment.

> >--- a/browser/themes/shared/devedition.inc.css
> >+++ b/browser/themes/shared/devedition.inc.css
> 
> >+#TabsToolbar:not(:-moz-lwtheme) {
> >+  text-shadow: none;
> >+}
> 
> What are you overriding here? Is this OS X specific?
> 

Yes, looks osx specific - it's this rule: http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3044.  I'll move it to the OSX file and make it important instead of using the lwtheme selector as you suggest later.

> >+.searchbar-textbox, #urlbar {
> 
> \n after ,

Done

> > > > +#TabsToolbar:not(:-moz-lwtheme) {
> > > 
> > > Why :not(:-moz-lwtheme) ? This theme is never applied for lwthemes, right?
> > > 
> > 
> > Right, it's for specificity.
> 
> Don't do that with selector modifications that don't otherwise make sense,
> it leads to code that nobody understands. Use !important if you have to.

OK, I'll make it important.

Comment 156

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #154)
> (In reply to Dão Gottwald [:dao] from comment #147)
> > Comment on attachment 8510657 [details] [diff] [review]
> > devedition-primary.patch
> > 
> > >+    <svg:clipPath id="urlbar-back-button-clip-path-square">
> > >+      <svg:rect x="0" y="0" width="10000" height="10000" />
> > >+    </svg:clipPath>
> > 
> > This makes no sense. It looks like you actually don't want any clip path at
> > all.
> 
> I don't know, when I set it to clip-path:none this is what I see:
> https://www.dropbox.com/s/5cvcbwc7rst1qdw/Screenshot%202014-10-24%2008.05.10.
> png?dl=0 versus with it applied:
> https://www.dropbox.com/s/66kspxydjamutn5/Screenshot%202014-10-24%2008.07.55.
> png?dl=0.

Try setting overflow: -moz-hidden-unscrollable or, if that doesn't work, overflow: hidden on #urlbar-wrapper.

Comment 157

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #156)
> Try setting overflow: -moz-hidden-unscrollable or, if that doesn't work,
> overflow: hidden on #urlbar-wrapper.

Thanks, that worked.

Comment 158

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #152)
> Comment on attachment 8509928 [details] [diff] [review]
> devedition-part4-windows.patch
> 
> Review of attachment 8509928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +25,5 @@
> > +  --toolbarbutton-active-background: hsla(210,4%,10%,.12);
> > +  --toolbarbutton-active-bordercolor: hsla(210,4%,10%,.2) transparent transparent;
> > +  --toolbarbutton-active-boxshadow: 0 1px 0 0 hsla(210,4%,10%,.1) inset;
> > +
> > +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);
> 
> Nit: just use the literal here.
Will do

> However, this only seems to be used on non-win8? What's up with that?
The aero theme is more complicated unfortunately :/

> @@ +28,5 @@
> > +
> > +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);
> > +
> > +  --toolbarbutton-combined-boxshadow: none;
> > +  --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 16px);
> 
> Can't this just be a flat background-color? Also, can we make this be
> ...combined-separator-backgroundcolor/boxshadow , as that's what it is?
Sure, but the OSX variables need to be updated as well :/

> And, if we want to change these, we should also be styling the separators of
> the zoom and clipboard buttons, which are in panelUIOverlay.css (but also
> affect the in-navbar styling)
Bug 949151 should take care of that

> @@ +702,5 @@
> > +
> > +    --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 18px);
> > +    --toolbarbutton-combined-boxshadow: 0 0 0 1px hsla(0,0%,100%,.2);
> > +  }
> > +  
> 
> Nit: whitespace
Will take care of that, thanks.

> @@ -727,3 @@
> >  
> >  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> >  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true])::before {
> 
> As far as I can tell this selector and its win8 cousin are dead. We only use
> this for the menubuttons, not for the combined buttons, where we just style
> the real separators. I don't see a ::before in the inspector for any of the
> cut/copy/paste and zoom in/reset/out buttons.
Bug 949151 as well

> @@ -774,2 @@
> >      background-size: 1px 18px;
> > -    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
> 
> Some of these look redundant, so maybe getting rid of them makes sense - but
> the box shadow isn't redundant, as far as I can tell?
> 
> @@ +840,5 @@
> >  #nav-bar .toolbarbutton-1[open] > .toolbarbutton-menubutton-dropmarker:not([disabled=true]) > .dropmarker-icon,
> >  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
> >  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-text,
> >  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-container {
> > +  background: var(--toolbarbutton-active-background);
> 
> Can't this just stay background-color?
In that case, the aero theme would be broken, since it uses a background-image

Comment 159

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

This fixes *partially* bug 949151 (only updates the broken selector), and addresses the 2 nits as well.

Comment 160

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Includes suggested cleanups from previous comments, fixes for windows in full screen and restored mode, and some JS that allows the theme to be removed / added during lightweight theme picking in the customize mode or on https://addons.mozilla.org.  Should be ready for more feedback whenever possible.

Comment 161

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

Rebased

Comment 162

[Tim Nguyen :ntim]

Attachment: devedition-primary.patch

Adds a small fix for the windows search dropdown arrow.

Comment 163

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

This adds as much variables as the landed OSX patch.

Comment 164

[Tim Nguyen :ntim]

Attachment: devedition-secondary.patch

Changelog :
- Changed a variable name (inactive-overlay -> inactive-mainView)
- Switched to SVG for the panel arrows
- Cleaned up panel arrow code
- Fixed a few other things

Just use the diff bugzilla feature if you want to see a precise changelog.

Comment 165

[:Gijs (out for now; he/him)]

Comment on attachment 8511587 [details] [diff] [review]
devedition-part4-windows.patch

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

(In reply to Tim Nguyen [:ntim] from comment #158)
> (In reply to :Gijs Kruitbosch from comment #152)
> > However, this only seems to be used on non-win8? What's up with that?
> The aero theme is more complicated unfortunately :/

This doesn't really make sense to me. There's a checked style on aero as well, surely? Why doesn't that need to be updated for the devedition theme?

> > @@ +28,5 @@
> > > +
> > > +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);
> > > +
> > > +  --toolbarbutton-combined-boxshadow: none;
> > > +  --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 16px);
> > 
> > Can't this just be a flat background-color? Also, can we make this be
> > ...combined-separator-backgroundcolor/boxshadow , as that's what it is?
> Sure, but the OSX variables need to be updated as well :/
> 
> > And, if we want to change these, we should also be styling the separators of
> > the zoom and clipboard buttons, which are in panelUIOverlay.css (but also
> > affect the in-navbar styling)
> Bug 949151 should take care of that

But now you've modified them here, but without changing the original rules in panelUIOverlay.css? I don't think that's the right approach - those rules are effectively dead now, right? Either do it properly here, or file a followup bug.

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +30,5 @@
>  #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
>    /* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */
>    box-shadow: 0 0 4px rgba(0,0,0,0.2);
> +  background: var(--panel-arrowcontent-background);
> +  border: var(--panel-arrowcontent-border);

Why change the behaviour here, and only for sub-popups of the bookmarks popup?

::: toolkit/themes/windows/global/popup.css
@@ +61,2 @@
>    background-clip: padding-box;
> +  border: var(--panel-arrowcontent-border);

Shouldn't this variable just specify the border color, and leave the width and style fixed here?

@@ +111,5 @@
>  
>  %ifdef XP_WIN
>  @media (-moz-windows-default-theme) {
>    .panel-arrowcontent {
> +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);

... so that this can't get out of sync?

Comment 166

[Paul Rouget [:paul]]

Attachment: devedition-part5-linux-WIP.patch

More work on Linux.

A couple of comments:
- shared/panelarrow-*.svg are missing. I added them myself.
- I changed --identity-box-*-background-image to --identity-box-*-background. This will require an update of the mac and windows patches
- there's still work to do for close buttons and arrow content.

Comment 167

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #165)
> Comment on attachment 8511587 [details] [diff] [review]
> devedition-part4-windows.patch
> 
> Review of attachment 8511587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Tim Nguyen [:ntim] from comment #158)
> > (In reply to :Gijs Kruitbosch from comment #152)
> > > However, this only seems to be used on non-win8? What's up with that?
> > The aero theme is more complicated unfortunately :/
> 
> This doesn't really make sense to me. There's a checked style on aero as
> well, surely? Why doesn't that need to be updated for the devedition theme?
checked style = active style.
Also, the OSX patch landed without variable for that.

> > > @@ +28,5 @@
> > > > +
> > > > +  --toolbarbutton-checkedhover-backgroundcolor: var(--toolbarbutton-active-background);
> > > > +
> > > > +  --toolbarbutton-combined-boxshadow: none;
> > > > +  --toolbarbutton-combined-backgroundimage: linear-gradient(hsla(210,54%,20%,.2) 0, hsla(210,54%,20%,.2) 16px);
> > > 
> > > Can't this just be a flat background-color? Also, can we make this be
> > > ...combined-separator-backgroundcolor/boxshadow , as that's what it is?
> > Sure, but the OSX variables need to be updated as well :/
> > 
> > > And, if we want to change these, we should also be styling the separators of
> > > the zoom and clipboard buttons, which are in panelUIOverlay.css (but also
> > > affect the in-navbar styling)
> > Bug 949151 should take care of that
> 
> But now you've modified them here, but without changing the original rules
> in panelUIOverlay.css? I don't think that's the right approach - those rules
> are effectively dead now, right? Either do it properly here, or file a
> followup bug.
Will do.

> ::: browser/themes/windows/customizableui/panelUIOverlay.css
> @@ +30,5 @@
> >  #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> >    /* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */
> >    box-shadow: 0 0 4px rgba(0,0,0,0.2);
> > +  background: var(--panel-arrowcontent-background);
> > +  border: var(--panel-arrowcontent-border);
> 
> Why change the behaviour here, and only for sub-popups of the bookmarks
> popup?
Because otherwise, with the secondary patch, the sub popups of the bookmarks popup will be white, while the popup itself will be dark.

> ::: toolkit/themes/windows/global/popup.css
> @@ +61,2 @@
> >    background-clip: padding-box;
> > +  border: var(--panel-arrowcontent-border);
> 
> Shouldn't this variable just specify the border color, and leave the width
> and style fixed here?
> 
> @@ +111,5 @@
> >  
> >  %ifdef XP_WIN
> >  @media (-moz-windows-default-theme) {
> >    .panel-arrowcontent {
> > +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);
> 
> ... so that this can't get out of sync?
The OSX variable landed like this. So it'd make more sense to change everything in one follow up patch.

Comment 168

[Paul Rouget [:paul]]

Attachment: devedition-part5-linux.patch

Comment 169

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Brian Grinstead [:bgrins] from comment #137)
> https://hg.mozilla.org/integration/fx-team/rev/a890f2283b74

I see lots of warnings when running tests the cset landed here:

[JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for 'transform'.  Falling back to 'initial'." {file: "chrome://browser/skin/browser.css" line: 7287 column: 226408 source: " translateY(calc(0px - var(--negative-space)))"}]

Comment 170

[:Gijs (out for now; he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #169)
> (In reply to Brian Grinstead [:bgrins] from comment #137)
> > https://hg.mozilla.org/integration/fx-team/rev/a890f2283b74
> 
> I see lots of warnings when running tests the cset landed here:
> 
> [JavaScript Warning: "Property contained reference to invalid variable. 
> Error in parsing value for 'transform'.  Falling back to 'initial'." {file:
> "chrome://browser/skin/browser.css" line: 7287 column: 226408 source: "
> translateY(calc(0px - var(--negative-space)))"}]

bug 1088789

Comment 171

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #167)
> (In reply to :Gijs Kruitbosch from comment #165)
> > ::: browser/themes/windows/customizableui/panelUIOverlay.css
> > @@ +30,5 @@
> > >  #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> > >    /* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */
> > >    box-shadow: 0 0 4px rgba(0,0,0,0.2);
> > > +  background: var(--panel-arrowcontent-background);
> > > +  border: var(--panel-arrowcontent-border);
> > 
> > Why change the behaviour here, and only for sub-popups of the bookmarks
> > popup?
> Because otherwise, with the secondary patch, the sub popups of the bookmarks
> popup will be white, while the popup itself will be dark.

That still doesn't explain changing the behaviour for the default theme...

Comment 172

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #169)
> (In reply to Brian Grinstead [:bgrins] from comment #137)
> > https://hg.mozilla.org/integration/fx-team/rev/a890f2283b74
> 
> I see lots of warnings when running tests the cset landed here:
> 
> [JavaScript Warning: "Property contained reference to invalid variable. 
> Error in parsing value for 'transform'.  Falling back to 'initial'." {file:
> "chrome://browser/skin/browser.css" line: 7287 column: 226408 source: "
> translateY(calc(0px - var(--negative-space)))"}]

Sorry, this was fixed in Bug 1088789 (currently on fx team)

Comment 173

[Brian Grinstead [:bgrins]]

Comment on attachment 8511948 [details] [diff] [review]
devedition-part5-linux.patch

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

I haven't been able to test this because it needs rebasing, but it looks pretty good except for a few notes.  Could you please rebase against the latest devedition-primary patch?

The panel arrows shouldn't be needed for this patch - they are added in devedition-secondary.patch (and are actually panelarrow-*.png).

::: browser/themes/linux/browser.css
@@ -607,5 @@
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled=true]):-moz-any(:hover:active, [open="true"]) > .toolbarbutton-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1[open="true"] > .toolbarbutton-menubutton-dropmarker:not([disabled=true]) > .dropmarker-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-container > .toolbarbutton-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon {
> -  background-color: rgba(154,154,154,.5);

It looks like this background color got removed.  Should this become be background: --toolbarbutton-active-background?

@@ +975,5 @@
>    -moz-margin-end: 4px;
>  }
>  
>  #identity-box.verifiedIdentity:not(:-moz-lwtheme) {
> +  color: var(--identity-box-chrome-color);

I'm actually thinking this won't work, because the same variables will be redefined in identity-block.inc.css (which is included in this file and will overwrite the default values of these variables).  So it will end up using the values from that file instead of the ones defined in the :root here.

It seems like it would be simpler to add in ifdef linux to http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block.inc.css#8 so that the proper colors are set in the first place (and this entire rule can be removed)

::: browser/themes/shared/devedition.inc.css
@@ +35,5 @@
>    --toolbarbutton-text-shadow: none;
>  
>    /* Identity box */
>    --identity-box-chrome-color: #46afe3;
> +  --identity-box-chrome-background: linear-gradient(#5F6670 0, #5F6670 100%);

This variable is used in browser/themes/shared/identity-block.inc.css, and I'm not sure actually sure if we could change it to background (looks like the position/size/repeat are set in a different rule than the image).  See the note above about getting rid of this override in browser.css and moving it to identity-block.inc.css anyway.

@@ +327,1 @@
>  #notification-popup-box {

I switched this to opacity: .6 in the most up to date devedition-primary patch.  Not sure what is better.

Comment 174

[Brian Grinstead [:bgrins]]

Comment on attachment 8511587 [details] [diff] [review]
devedition-part4-windows.patch

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

::: toolkit/themes/windows/global/popup.css
@@ +111,5 @@
>  
>  %ifdef XP_WIN
>  @media (-moz-windows-default-theme) {
>    .panel-arrowcontent {
> +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);

I think we want the ability to style the full border in this variable though.  For instance, the default osx popup doesn't have any border width so if we changed this to just a color we would have to add a 1px transparent border by default (http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/popup.css#10).

Comment 175

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #174)
> Comment on attachment 8511587 [details] [diff] [review]
> devedition-part4-windows.patch
> 
> Review of attachment 8511587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/windows/global/popup.css
> @@ +111,5 @@
> >  
> >  %ifdef XP_WIN
> >  @media (-moz-windows-default-theme) {
> >    .panel-arrowcontent {
> > +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);
> 
> I think we want the ability to style the full border in this variable
> though.  For instance, the default osx popup doesn't have any border width
> so if we changed this to just a color we would have to add a 1px transparent
> border by default
> (http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> popup.css#10).

In that case, it sounds like we need to have more than one variable, or just deal with OS X by having the OS X devedition.css actually set a 1px border width?

Having to re-set the variable to some other value in the default theme (which, AIUI, is what the current theme does) is kind of fragile if/when we have to update the value.

Comment 176

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #171)
> (In reply to Tim Nguyen [:ntim] from comment #167)
> > (In reply to :Gijs Kruitbosch from comment #165)
> > > ::: browser/themes/windows/customizableui/panelUIOverlay.css
> > > @@ +30,5 @@
> > > >  #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> > > >    /* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */
> > > >    box-shadow: 0 0 4px rgba(0,0,0,0.2);
> > > > +  background: var(--panel-arrowcontent-background);
> > > > +  border: var(--panel-arrowcontent-border);
> > > 
> > > Why change the behaviour here, and only for sub-popups of the bookmarks
> > > popup?
> > Because otherwise, with the secondary patch, the sub popups of the bookmarks
> > popup will be white, while the popup itself will be dark.
> 
> That still doesn't explain changing the behaviour for the default theme...
The styles here were emulating the arrow panel styles, but without the arrow, as mentioned here :
"/* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */".

Comment 177

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #175)
> (In reply to Brian Grinstead [:bgrins] from comment #174)
> > Comment on attachment 8511587 [details] [diff] [review]
> > devedition-part4-windows.patch
> > 
> > Review of attachment 8511587 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/themes/windows/global/popup.css
> > @@ +111,5 @@
> > >  
> > >  %ifdef XP_WIN
> > >  @media (-moz-windows-default-theme) {
> > >    .panel-arrowcontent {
> > > +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);
> > 
> > I think we want the ability to style the full border in this variable
> > though.  For instance, the default osx popup doesn't have any border width
> > so if we changed this to just a color we would have to add a 1px transparent
> > border by default
> > (http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> > popup.css#10).
> 
> In that case, it sounds like we need to have more than one variable, or just
> deal with OS X by having the OS X devedition.css actually set a 1px border
> width?

We could do it that way (change it to border-color then have a width hardcoded in the osx file).  Would you prefer this change and the variable name change from --toolbarbutton-combined-boxshadow to --toolbarbutton-combined-separator-boxshadow (including updating osx) happens within this patch or should it be done separately once windows/linux have landed?

Comment 178

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #177)
> (In reply to :Gijs Kruitbosch from comment #175)
> > (In reply to Brian Grinstead [:bgrins] from comment #174)
> > > Comment on attachment 8511587 [details] [diff] [review]
> > > devedition-part4-windows.patch
> > > 
> > > Review of attachment 8511587 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: toolkit/themes/windows/global/popup.css
> > > @@ +111,5 @@
> > > >  
> > > >  %ifdef XP_WIN
> > > >  @media (-moz-windows-default-theme) {
> > > >    .panel-arrowcontent {
> > > > +    --panel-arrowcontent-border: 1px solid hsla(210,4%,10%,.2);
> > > 
> > > I think we want the ability to style the full border in this variable
> > > though.  For instance, the default osx popup doesn't have any border width
> > > so if we changed this to just a color we would have to add a 1px transparent
> > > border by default
> > > (http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> > > popup.css#10).
> > 
> > In that case, it sounds like we need to have more than one variable, or just
> > deal with OS X by having the OS X devedition.css actually set a 1px border
> > width?
> 
> We could do it that way (change it to border-color then have a width
> hardcoded in the osx file).  Would you prefer this change and the variable
> name change from --toolbarbutton-combined-boxshadow to
> --toolbarbutton-combined-separator-boxshadow (including updating osx)
> happens within this patch or should it be done separately once windows/linux
> have landed?

Whichever is easier for you; I'm aware that we're under schedule pressure here.

Comment 179

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #178)
> > 
> > We could do it that way (change it to border-color then have a width
> > hardcoded in the osx file).  Would you prefer this change and the variable
> > name change from --toolbarbutton-combined-boxshadow to
> > --toolbarbutton-combined-separator-boxshadow (including updating osx)
> > happens within this patch or should it be done separately once windows/linux
> > have landed?
> 
> Whichever is easier for you; I'm aware that we're under schedule pressure
> here.

OK, let's do those refactors after the windows and linux patches land (especially since the CSS variable changes may have to be backed out depending on the perf regression).

Comment 180

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

Removed the .toolbaritem-combined-button selectors changes.

Comment 181

[Tim Nguyen :ntim]

Try pushes :
Plain try push : https://tbpl.mozilla.org/?tree=Try&rev=3a265e31a207

Primary patch + Windows patch : https://tbpl.mozilla.org/?tree=Try&rev=2b4ac3c14d53

Only Windows patch : https://tbpl.mozilla.org/?tree=Try&rev=b27d9c2cc2db

Comment 182

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #173)
> 
> @@ +327,1 @@
> >  #notification-popup-box {
> 
> I switched this to opacity: .6 in the most up to date devedition-primary
> patch.  Not sure what is better.

Now I remember why I switched this to opacity, at least on OSX there is a border image with an arrow that is white.  Using opacity allowed it to not stick out so bad but to still have the arrow match.

Comment 183

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #173)
> Comment on attachment 8511948 [details] [diff] [review]
> devedition-part5-linux.patch
> 
> Review of attachment 8511948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't been able to test this because it needs rebasing, but it looks
> pretty good except for a few notes.  Could you please rebase against the
> latest devedition-primary patch?
> 
> The panel arrows shouldn't be needed for this patch - they are added in
> devedition-secondary.patch (and are actually panelarrow-*.png).
> 
> ::: browser/themes/linux/browser.css
> @@ -607,5 @@
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled=true]):-moz-any(:hover:active, [open="true"]) > .toolbarbutton-icon,
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1[open="true"] > .toolbarbutton-menubutton-dropmarker:not([disabled=true]) > .dropmarker-icon,
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-container > .toolbarbutton-icon,
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon {
> > -  background-color: rgba(154,154,154,.5);
> 
> It looks like this background color got removed.  Should this become be
> background: --toolbarbutton-active-background?
> 
> @@ +975,5 @@
> >    -moz-margin-end: 4px;
> >  }
> >  
> >  #identity-box.verifiedIdentity:not(:-moz-lwtheme) {
> > +  color: var(--identity-box-chrome-color);
> 
> I'm actually thinking this won't work, because the same variables will be
> redefined in identity-block.inc.css (which is included in this file and will
> overwrite the default values of these variables).  So it will end up using
> the values from that file instead of the ones defined in the :root here.
> 
> It seems like it would be simpler to add in ifdef linux to
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-
> block.inc.css#8 so that the proper colors are set in the first place (and
> this entire rule can be removed)
> 

As per a discussion with :jaws on IRC, we should actually be able to leave this alone since this is here to add an opaque background to the semi-transparent default background in identity-block.inc.css.  In our case with a fully opaque --identity-box-chrome-background-image, this color will not matter.

Comment 184

[Tim Nguyen :ntim]

Attachment: devedition-part4-windows.patch

Introduced a new variable to fix white identity box issue.

Comment 185

[:Gijs (out for now; he/him)]

Comment on attachment 8512297 [details] [diff] [review]
devedition-part4-windows.patch

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

So this is OK apart from the remove content/display/width properties and the ordering of the new variable, I think.

However, please don't land this until the baseline and "Windows only" try pushes are finished, and you've retriggered the "o" and "s" talos runs on those, and looked at compare-talos to see about perf impact...

::: browser/themes/windows/browser.css
@@ +17,5 @@
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-wrapper
>  %define conditionalForwardWithUrlbarWidth 30
>  
> +:root {
> +  --verified-identity-box-backgroundcolor: #FFF;

Über-nit: this shouldn't be the first thing in here. Keep the same ordering as the rest of the file.

@@ -763,5 @@
>    #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
>    #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled])::before {
> -    content: "";
> -    display: -moz-box;
> -    width: 1px;

Shouldn't remove these.

Comment 186

[Brian Grinstead [:bgrins]]

Attachment: devedition-linux.patch

So this doesn't include the secondary UI variables needed for panels and menus, but it has enough for the primary patch (which I will upload next).  Also, the URL icon will be handled in the primary patch as well, since there the url history dropmarker is just using -moz-appearance: toolbarbutton-dropdown (nothing to override with variables).

Comment 187

[:Gijs (out for now; he/him)]

Comment on attachment 8511586 [details] [diff] [review]
devedition-primary.patch

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

Again, only looked at OS X and shared stuff so far. This looks better, but there's still quite some things that need help. Let's start with the last round, liberally snipped to omit bits that are either fixed, spun off, or hard/impossible to fix.

(In reply to Brian Grinstead [:bgrins] from comment #142)
> (In reply to :Gijs Kruitbosch from comment #138)
> > - this seems to unconditionally apply even when I select the light theme in
> > devtools. My thinking was that we could continue to use the default theme
> > there for now? Is that difficult to implement? :-)
> 
> That'd be easy to implement, pending a UX decision about it.

Was a UX decision made or solicited?

> > - there's an odd white line at the top of the window in fullscreen mode when
> > the titlebar is disabled (default); not sure if that's fixable.
> 
> Not sure yet - I'll follow up with that.

This is still here.

> > - there is some kind of weird interplay with switching in and out of the
> > theme in customize mode and pinned tabs / overflowed tabs toolbar. STR are
> > something like:
> >   * start with clean profile
> >   * create enough tabs to overflow, and one pinned tab
> >   * enable devtools theme in about:config
> >   * go into customize mode
> >   * click restore to defaults
> >   note how now the pinned tab and the button for the tab overflow overlap.
> > The same happens if you e.g. use the lwt switcher in customize mode.
> 
> Also going to follow up with that.

This is still here, and also makes the border between the tabs and the navbar disappear... not really sure what's going on.

> > @@ +88,5 @@
> > > +  background: var(--chrome-background-color);
> > > +  color: var(--chrome-color);
> > > +}
> > > +
> > > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> > 
> > The menubar isn't styled anywhere that I can see, is that intentional?
> > 
> 
> I had copied another selector, but I guess it doesn't matter since the
> menubar doesn't have moz-appearance: none.  Removed that from the selector.

This made it back in - I don't know if it's necessary on Windows/Linux, or something?

> > @@ +102,5 @@
> > > +#TabsToolbar:not(:-moz-lwtheme) {
> > > +  text-shadow: none;
> > > +}
> > > +
> > > +#TabsToolbar::after {
> > 
> > I think this is the navbar/tabstoolbar border?
> > 
> > Instead of hiding it, can you set content: none? Then there shouldn't be an
> > ::after at all.
> > 
> > Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap
> > between the overflow button separator on overflowing tabs, and the navbar.
> > 
> 
> After removing that line altogther I'm not even seeing what it is applying
> to.  I've changed it to content: none, but I think I'll have to follow up
> with you to figure out exactly what you mean here.

This: http://imgur.com/Gdi8eMh . Note how the line next to the < button is 1px less tall than the tab separators. I don't actually for sure know why this is, but it should ideally be fixed. :-)

I'm going to leave f?dao and hope he can weigh in on the Windows side.

::: browser/themes/osx/devedition.css
@@ +8,5 @@
> +   the tabs */
> +#main-window[tabsintitlebar] #TabsToolbar {
> +  padding-left: 50px;
> +  padding-right: 50px;
> +  margin-bottom: 0; /* Don't the inner highlight at the top of the nav-bar */

Nit: please fix the comment. Don't what?

::: browser/themes/shared/devedition.inc.css
@@ +190,5 @@
> +  border-color: transparent;
> +}
> +
> +.tabbrowser-tab {
> +  pointer-events: auto !important;

Why is this necessary?

@@ +197,5 @@
> +}
> +
> +.tabbrowser-tab[selected] {
> +  color: var(--chrome-tab-selection-color);
> +  background-color: var(--chrome-tab-selection-background-color) !important;

Can I have a comment why you need the !important here? :-)

Comment 188

[Brian Grinstead [:bgrins]]

 > (In reply to Brian Grinstead [:bgrins] from comment #142)
> > (In reply to :Gijs Kruitbosch from comment #138)
> > > - this seems to unconditionally apply even when I select the light theme in
> > > devtools. My thinking was that we could continue to use the default theme
> > > there for now? Is that difficult to implement? :-)
> > 
> > That'd be easy to implement, pending a UX decision about it.
> 
> Was a UX decision made or solicited?
> 

Stephen, if the light devtools theme is applied, should we (1) keep the dark browser theme or (2) switch to Australis styling?  Ideally there would be a (3) here, which is use the devedition theme but with light colors, but that's not likely to happen for the initial release.

Comment 189

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Changes since the last version -

* adds in variables for --toolbarbutton-active-bordercolor and --toolbarbutton-active-bordercolor
* improves the combined button styling.

Also folds in the linux devedition.css changes.  Carrying over the f?dao for the windows stuff from the last patch (you'll have to have the latest devedition-part4-windows patch applied also to see most changes).

Comment 190

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #187)
> > > - there's an odd white line at the top of the window in fullscreen mode when
> > > the titlebar is disabled (default); not sure if that's fixable.
> > 
> > Not sure yet - I'll follow up with that.
> 
> This is still here.
> 

This also exists on dark lw themes, like https://addons.mozilla.org/en-US/firefox/addon/just-black/.  Not sure what is causing it.

Comment 191

[Brian Grinstead [:bgrins]]

Attachment: devedition-secondary.patch

rebased, added missing SVG files, updated a few variables that weren't matching in 8511588

Comment 192

[Paul Rouget [:paul]]

Brian, you asked me earlier to look at the hover/active state of the bookmark widget on Linux. As far as I can tell, it works as expect. All these 6 variables:
--toolbarbutton-hover-background
--toolbarbutton-hover-boxshadow
--toolbarbutton-hover-bordercolor
--toolbarbutton-active-background
--toolbarbutton-active-boxshadow
--toolbarbutton-active-bordercolor
... are properly used, for toolbarbuttons and the bookmark combined buttons.

Comment 193

[Paul Rouget [:paul]]

Attachment: notification-box addendum

For the notification box, this works as expected for Linux and osx.

Comment 194

[Paul Rouget [:paul]]

Brian, you asked me to look at the panels. They appear to look as good as in osx.

Comment 195

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #185)
> Comment on attachment 8512297 [details] [diff] [review]
> devedition-part4-windows.patch
> 
> Review of attachment 8512297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is OK apart from the remove content/display/width properties and the
> ordering of the new variable, I think.
> 
> However, please don't land this until the baseline and "Windows only" try
> pushes are finished, and you've retriggered the "o" and "s" talos runs on
> those, and looked at compare-talos to see about perf impact...
> @@ -763,5 @@
> >    #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> >    #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled])::before {
> > -    content: "";
> > -    display: -moz-box;
> > -    width: 1px;
> 
> Shouldn't remove these.

This is already set on non-aero with the same values and selectors. So it makes sense to me to remove them.

Comment 196

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #195)
> (In reply to :Gijs Kruitbosch from comment #185)
> > > -    content: "";
> > > -    display: -moz-box;
> > > -    width: 1px;
> > 
> > Shouldn't remove these.
> 
> This is already set on non-aero with the same values and selectors. So it
> makes sense to me to remove them.

You're right, I missed that from looking at the interdiff. Carry on!

I've retriggered the XP and Win7 t-o and t-s runs for the plain and the windows patch. When they've finished (check https://tbpl.mozilla.org/?tree=Try&rev=3a265e31a207 and https://tbpl.mozilla.org/?tree=Try&rev=b27d9c2cc2db -- all of them should have 5 runs), please check the tpaint, ts_paint, CART and TART sections on: http://compare-talos.mattn.ca/?oldRevs=3a265e31a207&newRev=b27d9c2cc2db&server=graphs.mozilla.org&submit=true

Comment 197

[:Gijs (out for now; he/him)]

Comment on attachment 8512329 [details] [diff] [review]
devedition-linux.patch

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

This looks sane to me, but see the below for a small potential issue.

::: browser/themes/linux/browser.css
@@ +23,5 @@
> +  --toolbarbutton-hover-boxshadow: 0 1px 0 hsla(0,0%,100%,.3) inset, 0 0 0 1px hsla(0,0%,100%,.2) inset, 0 1px 0 hsla(0,0%,0%,.03)
> +  --toolbarbutton-hover-bordercolor: rgb(154,154,154);
> +
> +  --toolbarbutton-active-boxshadow: 0 1px 1px hsla(0,0%,0%,.1) inset, 0 0 1px hsla(0,0%,0%,.3) inset;
> +  --toolbarbutton-active-bordercolor: rgb(154,154,154);

Note: the linux theme uses a single color, and uses it in border: 1px solid --var(); style. The Windows theme uses 3 separate colors in some cases, and uses it with border-color: --var(). A quick test seems to indicate that something along the lines of:

border: 1px solid red green blue;

is not valid CSS.

I don't know if the devtools theme is going to use multiple colors, but if it is, the border: properties below should be split so that we set border-color to the variable. In fact, maybe that's a good idea just to be safe.

Comment 198

[:Gijs (out for now; he/him)]

Oh, and the Linux base patch, too, probably wants a try push...

Comment 199

[Tim Nguyen :ntim]

Attachment: devedition-windows.patch (r=Gijs)

Comment 200

[Tim Nguyen :ntim]

Talos results look good to me :
CART :
Win XP : 2.79%
Win 7 : -0.34%
TART :
Win XP : -0.49%
Win 7 : -0.38%
ts_paint :
Win XP : -0.23%
Win 7 : 0.57%
tpaint :
Win XP : 1.96%
Win 7 : 0.6%

Comment 201

[Tim Nguyen :ntim]

checkin-needed for the devedition-part4-windows.patch only !

Comment 202

[Paul Rouget [:paul]]

Attachment: devedition-linux.patch (r=Gijs)

Comment 203

[Paul Rouget [:paul]]

linux patch: https://tbpl.mozilla.org/?tree=Try&rev=c215218d723b
primary patch + linux patch: https://tbpl.mozilla.org/?tree=Try&rev=9054ea1015d8

Comment 204

[Paul Rouget [:paul]]

Cancelling tries comment 203. For some reason... empty diff.

Comment 205

[Tim Nguyen :ntim]

Paul, you need to provide a plain try push (with no patches), and one with just the Linux patch.

Comment 206

[Paul Rouget [:paul]]

Here we are:
https://tbpl.mozilla.org/?tree=Try&rev=25cab8108303
https://tbpl.mozilla.org/?tree=Try&rev=3ed0d065ff91
https://tbpl.mozilla.org/?tree=Try&rev=185e05cb96ea

Comment 207

[Brian Grinstead [:bgrins]]

Comment on attachment 8501784 [details] [diff] [review]
devedition-part1.patch

Approval Request Comment
[Feature/regressing bug #]: DevEdition
[User impact if declined]: We need this for the DevEdition release.  This patch doesn't have any style changes, but adds the necessary pref and JavaScript changes to toggle it on and off.
[Describe test coverage new/current, TBPL]: Adds a test for the theme change
[Risks and why]: It adds a new pref and some listeners in the main browser.js file, but it's been on m-c for 2 weeks without any issues: https://bugzilla.mozilla.org/show_bug.cgi?id=1053434#c57.
[String/UUID change made/needed]: none

Comment 208

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #187)
> > > - there is some kind of weird interplay with switching in and out of the
> > > theme in customize mode and pinned tabs / overflowed tabs toolbar. STR are
> > > something like:
> > >   * start with clean profile
> > >   * create enough tabs to overflow, and one pinned tab
> > >   * enable devtools theme in about:config
> > >   * go into customize mode
> > >   * click restore to defaults
> > >   note how now the pinned tab and the button for the tab overflow overlap.
> > > The same happens if you e.g. use the lwt switcher in customize mode.
> > 
> > Also going to follow up with that.
> 
> This is still here, and also makes the border between the tabs and the
> navbar disappear... not really sure what's going on.
> 

So, if I call gBrowser.tabContainer._positionPinnedTabs(); after removing the devedition stylesheet this is fixed.  I'm assuming I shouldn't be doing this since that's an underscore prefixed function - is there is a better way to accomplish this?  I'm not really seeing the border between the tabs and navbar disappearing with or without the change, so I can't confirm it fixes that problem too.  I'll upload the patch with this change to see if it resolves both issues for you.

> > > @@ +88,5 @@
> > > > +  background: var(--chrome-background-color);
> > > > +  color: var(--chrome-color);
> > > > +}
> > > > +
> > > > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> > > 
> > > The menubar isn't styled anywhere that I can see, is that intentional?
> > > 
> > 
> > I had copied another selector, but I guess it doesn't matter since the
> > menubar doesn't have moz-appearance: none.  Removed that from the selector.
> 
> This made it back in - I don't know if it's necessary on Windows/Linux, or
> something?
> 

It's for the Windows menu bar - here is what it looks like without the :not(#toolbar-menubar) rule: https://www.dropbox.com/s/murackfjdzij1p6/Screenshot%202014-10-28%2009.37.53.png?dl=0.  And with: https://www.dropbox.com/s/7ooceiatn8g5or7/Screenshot%202014-10-28%2009.38.51.png?dl=0.

> > > @@ +102,5 @@
> > > > +#TabsToolbar:not(:-moz-lwtheme) {
> > > > +  text-shadow: none;
> > > > +}
> > > > +
> > > > +#TabsToolbar::after {
> > > 
> > > I think this is the navbar/tabstoolbar border?
> > > 
> > > Instead of hiding it, can you set content: none? Then there shouldn't be an
> > > ::after at all.
> > > 
> > > Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap
> > > between the overflow button separator on overflowing tabs, and the navbar.
> > > 
> > 
> > After removing that line altogther I'm not even seeing what it is applying
> > to.  I've changed it to content: none, but I think I'll have to follow up
> > with you to figure out exactly what you mean here.
> 
> This: http://imgur.com/Gdi8eMh . Note how the line next to the < button is
> 1px less tall than the tab separators. I don't actually for sure know why
> this is, but it should ideally be fixed. :-)
> 

It's because of http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#121, I've added a rule with margin-bottom: 0, which fixes this.  Ideally we could switch the @tabToolbarNavbarOverlap@ variable to a CSS variable, but I don't want to add anything else that might affect perf for v1.

> ::: browser/themes/osx/devedition.css
> @@ +8,5 @@
> > +   the tabs */
> > +#main-window[tabsintitlebar] #TabsToolbar {
> > +  padding-left: 50px;
> > +  padding-right: 50px;
> > +  margin-bottom: 0; /* Don't the inner highlight at the top of the nav-bar */
> 
> Nit: please fix the comment. Don't what?
> 

Don't 'overlap' - updated comment.

> ::: browser/themes/shared/devedition.inc.css
> @@ +190,5 @@
> > +  border-color: transparent;
> > +}
> > +
> > +.tabbrowser-tab {
> > +  pointer-events: auto !important;
> 
> Why is this necessary?
> 

To override http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#278.  Actually, the !important wasn't necessary though so I removed it.

> @@ +197,5 @@
> > +}
> > +
> > +.tabbrowser-tab[selected] {
> > +  color: var(--chrome-tab-selection-color);
> > +  background-color: var(--chrome-tab-selection-background-color) !important;
> 
> Can I have a comment why you need the !important here? :-)

Actually just needed to move this selector below the next list (the :hover style was overriding selected), so no more important.

Comment 209

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Carrying over f?dao.  Gijs, see comments 188, 190, and 208 in response to your previous feedback.  I was going to hold off asking for formal review until getting further feedback on windows/linux and a UX review (which Stephen is working on), but if you can review / give further feedback on the shared/osx/linux bits I wouldn't complain.

Note: I'm aware of the issue with the white hover state with the back/forward buttons on Yosemite, which I believe is because the styles here include :hover which makes the selector more specific than the one in osx/devedition.css: https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1695.  This is another time where I'm not clear if it's better to update the selector list to match, or to just use !important.  Anyway, I've updated the selector list for now which hopefully will fix that (although I don't have Yosemite set up so I haven't tested it).

Comment 210

[Brian Grinstead [:bgrins]]

Comment on attachment 8501784 [details] [diff] [review]
devedition-part1.patch

Wait, nevermind - this is already on Aurora

Comment 211

[Tim Nguyen :ntim]

Comment on attachment 8512795 [details] [diff] [review]
devedition-primary.patch

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

::: browser/themes/shared/devedition.inc.css
@@ +82,5 @@
> +
> +#tabbrowser-tabs,
> +#TabsToolbar,
> +.tab-background-middle[selected],
> +#tab-background-fill,

These should be removed, it seems that those came back.

::: browser/themes/windows/devedition.css
@@ +36,5 @@
> +/* Ensure that the entire background is styled when in maximized customize mode */
> +#main-window[sizemode="maximized"] #browser-panel {
> +  background: var(--chrome-background-color) !important;
> +}
> +

You can simplify this :
#main-window:not([sizemode="maximized"]) #browser-panel,	
#titlebar-content {	
  background: transparent;	
}

Comment 212

[Brian Grinstead [:bgrins]]

Comment on attachment 8512458 [details] [diff] [review]
notification-box addendum

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

Asked Stephen about whether to use a transparent background here, and he said "I like fully transparent, but it might conflict with some of the icons".  So we are going to try transparent and see how things look.  Either way, the changes in devedition-secondary for notification-popup are not needed so I'll remove them.  I think this would be simpler to get this effect with just:

#notification-popup-box {
  border-radius: 0;
  border: none;
  background: transparent;
}

Which I'll fold into the primary patch

Comment 213

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #206)
> Here we are:
> https://tbpl.mozilla.org/?tree=Try&rev=25cab8108303
> https://tbpl.mozilla.org/?tree=Try&rev=3ed0d065ff91
> https://tbpl.mozilla.org/?tree=Try&rev=185e05cb96ea

Those pushes don't seem to include talos (should include -t all in the push syntax).  Regardless, I have a push from earlier in the day:

http://compare-talos.mattn.ca/?oldRevs=dcfb1ac1935c&newRev=6047a4901abb&server=graphs.mozilla.org&submit=true

Here is the same one with PGO results separated from non PGO:

http://compare-talos.mattn.ca/dev/?oldBranch=Fx-Team&oldRevs=dcfb1ac1935c&newBranch=Try&newRev=6047a4901abb&submit=true

Comment 214

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #213)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #206)
> > Here we are:
> > https://tbpl.mozilla.org/?tree=Try&rev=25cab8108303
> > https://tbpl.mozilla.org/?tree=Try&rev=3ed0d065ff91
> > https://tbpl.mozilla.org/?tree=Try&rev=185e05cb96ea
> 
> Those pushes don't seem to include talos (should include -t all in the push
> syntax).  Regardless, I have a push from earlier in the day:
> 
> http://compare-talos.mattn.ca/
> ?oldRevs=dcfb1ac1935c&newRev=6047a4901abb&server=graphs.mozilla.
> org&submit=true
> 
> Here is the same one with PGO results separated from non PGO:
> 
> http://compare-talos.mattn.ca/dev/?oldBranch=Fx-
> Team&oldRevs=dcfb1ac1935c&newBranch=Try&newRev=6047a4901abb&submit=true

These have no retriggers; because of talos noise, you should ensure that each of the "o" and "s" runs has been run at least 5 times on the platform(s) you care about (linux and linux64, in this case).

Comment 215

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #214)
> > http://compare-talos.mattn.ca/dev/?oldBranch=Fx-
> > Team&oldRevs=dcfb1ac1935c&newBranch=Try&newRev=6047a4901abb&submit=true
> 
> These have no retriggers; because of talos noise, you should ensure that
> each of the "o" and "s" runs has been run at least 5 times on the
> platform(s) you care about (linux and linux64, in this case).

OK, I've triggered ~ 10 times for each test.  The results seem fine for linux on cart, tpaint and tspaint - should we checkin the linux patch or wait for more data?

Comment 216

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #215)
> (In reply to :Gijs Kruitbosch from comment #214)
> > > http://compare-talos.mattn.ca/dev/?oldBranch=Fx-
> > > Team&oldRevs=dcfb1ac1935c&newBranch=Try&newRev=6047a4901abb&submit=true
> > 
> > These have no retriggers; because of talos noise, you should ensure that
> > each of the "o" and "s" runs has been run at least 5 times on the
> > platform(s) you care about (linux and linux64, in this case).
> 
> OK, I've triggered ~ 10 times for each test.  The results seem fine for
> linux on cart, tpaint and tspaint - should we checkin the linux patch or
> wait for more data?

It's a linux patch, so if linux is OK (which it looks to be), I think we can land it. There's no sane reason as to how that patch would affect any other platforms. :-)

Comment 217

[Brian Grinstead [:bgrins]]

Checkin for linux patch: https://hg.mozilla.org/integration/fx-team/rev/6a9de9e03b97

Comment 218

[Brian Grinstead [:bgrins]]

Checkin for windows patch: https://hg.mozilla.org/integration/fx-team/rev/141414b0f7c7

Comment 219

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a9de9e03b97
https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7

Comment 220

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #208)
> (In reply to :Gijs Kruitbosch from comment #187)
> > > > - there is some kind of weird interplay with switching in and out of the
> > > > theme in customize mode and pinned tabs / overflowed tabs toolbar. STR are
> > > > something like:
> > > >   * start with clean profile
> > > >   * create enough tabs to overflow, and one pinned tab
> > > >   * enable devtools theme in about:config
> > > >   * go into customize mode
> > > >   * click restore to defaults
> > > >   note how now the pinned tab and the button for the tab overflow overlap.
> > > > The same happens if you e.g. use the lwt switcher in customize mode.
> > > 
> > > Also going to follow up with that.
> > 
> > This is still here, and also makes the border between the tabs and the
> > navbar disappear... not really sure what's going on.
> > 
> 
> So, if I call gBrowser.tabContainer._positionPinnedTabs(); after removing
> the devedition stylesheet this is fixed.  I'm assuming I shouldn't be doing
> this since that's an underscore prefixed function - is there is a better way
> to accomplish this?  I'm not really seeing the border between the tabs and
> navbar disappearing with or without the change, so I can't confirm it fixes
> that problem too.  I'll upload the patch with this change to see if it
> resolves both issues for you.

This is difficult to say... from a quick look, it seems that the code there relies on the boundingclientrect of the tabs within their container, which changes in this theme switch. I don't think we have any other code that alters this (adding buttons to the left of the tabs, for instance, will be outside the container. Dão would know better than me what to do here.

> > > > @@ +88,5 @@
> > > > > +  background: var(--chrome-background-color);
> > > > > +  color: var(--chrome-color);
> > > > > +}
> > > > > +
> > > > > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> > > > 
> > > > The menubar isn't styled anywhere that I can see, is that intentional?
> > > > 
> > > 
> > > I had copied another selector, but I guess it doesn't matter since the
> > > menubar doesn't have moz-appearance: none.  Removed that from the selector.
> > 
> > This made it back in - I don't know if it's necessary on Windows/Linux, or
> > something?
> > 
> 
> It's for the Windows menu bar - here is what it looks like without the
> :not(#toolbar-menubar) rule:
> https://www.dropbox.com/s/murackfjdzij1p6/Screenshot%202014-10-28%2009.37.53.
> png?dl=0.  And with:
> https://www.dropbox.com/s/7ooceiatn8g5or7/Screenshot%202014-10-28%2009.38.51.
> png?dl=0.

I think that we should ideally make sure that the menubar gets the same background as the tabstoolbar, and there isn't such an odd "gap" there - I imagine that for that to work, we'd need to give the toolbox a background and ensure that the menubar's foggy background isn't applied in aero themes. I'm OK with followup-ing that in the interest of landing this sooner rather than later, though.

> > > > @@ +102,5 @@
> > > > > +#TabsToolbar:not(:-moz-lwtheme) {
> > > > > +  text-shadow: none;
> > > > > +}
> > > > > +
> > > > > +#TabsToolbar::after {
> > > > 
> > > > I think this is the navbar/tabstoolbar border?
> > > > 
> > > > Instead of hiding it, can you set content: none? Then there shouldn't be an
> > > > ::after at all.
> > > > 
> > > > Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap
> > > > between the overflow button separator on overflowing tabs, and the navbar.
> > > > 
> > > 
> > > After removing that line altogther I'm not even seeing what it is applying
> > > to.  I've changed it to content: none, but I think I'll have to follow up
> > > with you to figure out exactly what you mean here.
> > 
> > This: http://imgur.com/Gdi8eMh . Note how the line next to the < button is
> > 1px less tall than the tab separators. I don't actually for sure know why
> > this is, but it should ideally be fixed. :-)
> > 
> 
> It's because of
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.
> css#121, I've added a rule with margin-bottom: 0, which fixes this.  Ideally
> we could switch the @tabToolbarNavbarOverlap@ variable to a CSS variable,
> but I don't want to add anything else that might affect perf for v1.

This sounds sane. Can you file a followup bug about this, though? :-)
 
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +190,5 @@
> > > +  border-color: transparent;
> > > +}
> > > +
> > > +.tabbrowser-tab {
> > > +  pointer-events: auto !important;
> > 
> > Why is this necessary?
> > 
> 
> To override
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.
> css#278.  Actually, the !important wasn't necessary though so I removed it.

Can you put a comment here and say that we normally rely on the pointer events on the other tab elements, but that this theme hides those elements and therefore we need the pointer events on this layer instead?

Comment 221

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #220)
> > > > > @@ +88,5 @@
> > > > > > +  background: var(--chrome-background-color);
> > > > > > +  color: var(--chrome-color);
> > > > > > +}
> > > > > > +
> > > > > > +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> > > > > 
> > > > > The menubar isn't styled anywhere that I can see, is that intentional?
> > > > > 
> > > > 
> > > > I had copied another selector, but I guess it doesn't matter since the
> > > > menubar doesn't have moz-appearance: none.  Removed that from the selector.
> > > 
> > > This made it back in - I don't know if it's necessary on Windows/Linux, or
> > > something?
> > > 
> > 
> > It's for the Windows menu bar - here is what it looks like without the
> > :not(#toolbar-menubar) rule:
> > https://www.dropbox.com/s/murackfjdzij1p6/Screenshot%202014-10-28%2009.37.53.
> > png?dl=0.  And with:
> > https://www.dropbox.com/s/7ooceiatn8g5or7/Screenshot%202014-10-28%2009.38.51.
> > png?dl=0.
> 
> I think that we should ideally make sure that the menubar gets the same
> background as the tabstoolbar, and there isn't such an odd "gap" there - I
> imagine that for that to work, we'd need to give the toolbox a background
> and ensure that the menubar's foggy background isn't applied in aero themes.
> I'm OK with followup-ing that in the interest of landing this sooner rather
> than later, though.
> 

Filed 1091012

> > > > > @@ +102,5 @@
> > > > > > +#TabsToolbar:not(:-moz-lwtheme) {
> > > > > > +  text-shadow: none;
> > > > > > +}
> > > > > > +
> > > > > > +#TabsToolbar::after {
> > > > > 
> > > > > I think this is the navbar/tabstoolbar border?
> > > > > 
> > > > > Instead of hiding it, can you set content: none? Then there shouldn't be an
> > > > > ::after at all.
> > > > > 
> > > > > Also, this plus the navbar/tabs/titlebar overlap means there's a 1px gap
> > > > > between the overflow button separator on overflowing tabs, and the navbar.
> > > > > 
> > > > 
> > > > After removing that line altogther I'm not even seeing what it is applying
> > > > to.  I've changed it to content: none, but I think I'll have to follow up
> > > > with you to figure out exactly what you mean here.
> > > 
> > > This: http://imgur.com/Gdi8eMh . Note how the line next to the < button is
> > > 1px less tall than the tab separators. I don't actually for sure know why
> > > this is, but it should ideally be fixed. :-)
> > > 
> > 
> > It's because of
> > http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.
> > css#121, I've added a rule with margin-bottom: 0, which fixes this.  Ideally
> > we could switch the @tabToolbarNavbarOverlap@ variable to a CSS variable,
> > but I don't want to add anything else that might affect perf for v1.
> 
> This sounds sane. Can you file a followup bug about this, though? :-)

Filed 1091001

>  
> > > ::: browser/themes/shared/devedition.inc.css
> > > @@ +190,5 @@
> > > > +  border-color: transparent;
> > > > +}
> > > > +
> > > > +.tabbrowser-tab {
> > > > +  pointer-events: auto !important;
> > > 
> > > Why is this necessary?
> > > 
> > 
> > To override
> > http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.
> > css#278.  Actually, the !important wasn't necessary though so I removed it.
> 
> Can you put a comment here and say that we normally rely on the pointer
> events on the other tab elements, but that this theme hides those elements
> and therefore we need the pointer events on this layer instead?

Done - I'll also upload a new patch because I think I've had to rebase since the last upload, and also done a few UX requests in the meantime

Comment 222

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener.patch

Breaking the JS changes into their own patch.  The main thing this patch does is fix the lightweight theme preview as seen in the customize UI or the addons page.  It also includes the pinned tabs position fix.

I'm not actually sure this is the best way to do it - I would have preferred to just add a check to document.documentElement.hasAttribute("lwtheme") in place of the current check on the lightweightThemes.isThemeSelected pref in the hope that it would cover both previewed and selected cases.  But it didn't work as I expected - during the "lightweight-theme-styling-update" event, the attribute is exactly the opposite of what I would think.  If data is null then the attribute exists, but if data is the theme being previewed it doesn't.

Dão, as mentioned in Comment 220, do you have any thoughts about the call to tabContainer._positionPinnedTabs?  Is there another way to do this, or should I un-underscore-prefix that function if this is the best way to accomplish what I'm trying to do?

Comment 223

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Rebased, some UX fixes, and without js.  Gijs, going to ask for review on the osx/linux/shared changes with the knowledge that I will still need to make some UX updates as Stephen completes testing - but assuming those interdiffs will be pretty simple.  Leaving feedback for Dão for the windows changes.

Comment 224

[Brian Grinstead [:bgrins]]

Comment on attachment 8512415 [details] [diff] [review]
devedition-secondary.patch

Marking secondary patch obsolete - moved this over to bug 1090548

Comment 225

[Dão Gottwald [:dao]]

Comment on attachment 8513609 [details] [diff] [review]
lwtheme-preview-listener.patch

(In reply to Brian Grinstead [:bgrins] from comment #222)
> I'm not actually sure this is the best way to do it - I would have preferred
> to just add a check to document.documentElement.hasAttribute("lwtheme") in
> place of the current check on the lightweightThemes.isThemeSelected pref in
> the hope that it would cover both previewed and selected cases.  But it
> didn't work as I expected - during the "lightweight-theme-styling-update"
> event, the attribute is exactly the opposite of what I would think.  If data
> is null then the attribute exists, but if data is the theme being previewed
> it doesn't.

The lwtheme attribute is set in response to lightweight-theme-styling-update. If your observer runs before that, the attribute won't be updated yet.

Comment 226

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #190)
> (In reply to :Gijs Kruitbosch from comment #187)
> > > > - there's an odd white line at the top of the window in fullscreen mode when
> > > > the titlebar is disabled (default); not sure if that's fixable.
> > > 
> > > Not sure yet - I'll follow up with that.
> > 
> > This is still here.
> > 
> 
> This also exists on dark lw themes, like
> https://addons.mozilla.org/en-US/firefox/addon/just-black/.  Not sure what
> is causing it.

Filed Bug 1091110 for the light border on dark lw themes in OSX

Comment 227

[:Gijs (out for now; he/him)]

Comment on attachment 8513609 [details] [diff] [review]
lwtheme-preview-listener.patch

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

r=me, assuming dao is OK with the positionPinnedTabs thing.

Comment 228

[:Gijs (out for now; he/him)]

Comment on attachment 8513612 [details] [diff] [review]
devedition-primary.patch

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

This was going to be r=me for OS X and shared styling, with the two comments below addressed. However... I noticed the back/fwd button have no hover style on either OS X or Linux. Can we fix that? :-)

Linux needs some more work. There are some comments below already. The other thing I was wondering is why the menubar is unstyled on Linux, too (which looks quite odd/jarring), and why the buttons have a different border-radius style.

::: browser/themes/linux/devedition.css
@@ +4,5 @@
>  
>  %include ../shared/devedition.inc.css
> +
> +.close-icon,
> +.tab-close-button:not([selected]):not(:hover):not([brighttext]) {

Not sure why there's a :not([brighttext]) here; the consequence of this is that I'm seeing dark close icons instead of the bright close icons that I see on OS X. Not intentional, I presume?

@@ +10,5 @@
> +}
> +
> +/* Square back and forward buttons */
> +#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> +#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {

This is on the icons; the buttons are too big on my Ubuntu VM: http://imgur.com/rmvGHLF

They also seem to have an odd semi-transparent border...

@@ +11,5 @@
> +
> +/* Square back and forward buttons */
> +#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> +#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> +  height: 22px;

You shouldn't need to set the height of these.

::: browser/themes/shared/devedition.inc.css
@@ +54,5 @@
> +
> +  /* Menu button separator */
> +  --panel-ui-button-background-image: linear-gradient(to bottom, #5E6670, #5E6670);
> +  --panel-ui-button-background-size: 1px calc(100% - 1px);
> +  --panel-ui-button-background-position: 0px 0px;

This should be 1px 0px. The background in the default theme is basically "light, dark, light", with the visual "border" being in the middle. This "border" should be in the same position to avoid it looking like it shifts when you toggle the theme (and to keep the spacing of the icon symmetrical).

::: browser/themes/shared/devedition/search.svg
@@ +23,5 @@
> +    fill: #B6BABF;
> +  }
> +
> +  use[id*="-mac"] {
> +    transform: translate(16px) scale(-1,1);

nit: scaleX(-1)

Comment 229

[Dão Gottwald [:dao]]

Comment on attachment 8513609 [details] [diff] [review]
lwtheme-preview-listener.patch

>+  _toggleStyleSheet: function(deveditionThemeEnabled) {
>     if (deveditionThemeEnabled && !this.styleSheet) {
>       let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
>       let styleSheet = this.styleSheet = document.createProcessingInstruction(
>         'xml-stylesheet', styleSheetAttr);
>       this.styleSheet.addEventListener("load", function onLoad() {
>         styleSheet.removeEventListener("load", onLoad);
>         ToolbarIconColor.inferFromText();
>       });
>       document.insertBefore(this.styleSheet, document.documentElement);
>     } else if (!deveditionThemeEnabled && this.styleSheet) {
>       this.styleSheet.remove();
>       this.styleSheet = null;
>+      gBrowser.tabContainer._positionPinnedTabs();
>       ToolbarIconColor.inferFromText();
>     }
>   },

I'm assuming you're calling _positionPinnedTabs because the measurements done in that method become off when removing your stylesheet, because that stylesheet changed the dimensions of the tabs, the scroll buttons etc.. Given that, I don't understand why you wouldn't need to call _positionPinnedTabs when /adding/ your stylesheet too.

Comment 230

[:Gijs (out for now; he/him)]

Attachment: Yosemite back button in :-moz-window-inactive

I promised I'd look at the yosemite stuff. That looks mostly fine, except for this: when you remove focus from the window, the back buttons get this look...

Probably just a question of overriding this set of selectors:

http://hg.mozilla.org/mozilla-central/annotate/675913ddbb55/browser/themes/osx/browser.css#l1763

Comment 231

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #229)
> Comment on attachment 8513609 [details] [diff] [review]
> lwtheme-preview-listener.patch
> 
> >+  _toggleStyleSheet: function(deveditionThemeEnabled) {
> >     if (deveditionThemeEnabled && !this.styleSheet) {
> >       let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
> >       let styleSheet = this.styleSheet = document.createProcessingInstruction(
> >         'xml-stylesheet', styleSheetAttr);
> >       this.styleSheet.addEventListener("load", function onLoad() {
> >         styleSheet.removeEventListener("load", onLoad);
> >         ToolbarIconColor.inferFromText();
> >       });
> >       document.insertBefore(this.styleSheet, document.documentElement);
> >     } else if (!deveditionThemeEnabled && this.styleSheet) {
> >       this.styleSheet.remove();
> >       this.styleSheet = null;
> >+      gBrowser.tabContainer._positionPinnedTabs();
> >       ToolbarIconColor.inferFromText();
> >     }
> >   },
> 
> I'm assuming you're calling _positionPinnedTabs because the measurements
> done in that method become off when removing your stylesheet, because that
> stylesheet changed the dimensions of the tabs, the scroll buttons etc..
> Given that, I don't understand why you wouldn't need to call
> _positionPinnedTabs when /adding/ your stylesheet too.

Right, I had only tested going from dev edition to classic but surely it is a problem going the other way too.  So you are OK in general with calling _positionPinnedTabs here, as long as it's done in both places?

Comment 232

[Brian Grinstead [:bgrins]]

ni? for Comment 231

Comment 233

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener.patch

Dão, awaiting your approval for the use of _positionPinnedTabs as in Comment 231.  I've updated the patch to call it in both adding and removing scenarios.

Gijs, re-asking for review here because I fixed a bug in the customize mode dropdown where the default theme ID comes through in a "lightweight-theme-styling-update" message when picking it from the theme dropdown.  This was causing a bug when:

1) Apply dev edition theme
2) open customize mode -> theme dropdown
3) Select a lw theme
4) Select the default theme

At this point the devedition theme never got reapplied, because the default theme came through as a lightweight theme update (so we force removed the stylesheet).  With this change I ignore that message just as if no lw theme was applied.  I also added a test for this.

Comment 234

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8514457 [details] [diff] [review]
lwtheme-preview-listener.patch

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

Taking the review for Dao and answering your question. This should be good now that we are calling _positionPinnedTabs in both directions.

Comment 235

[Brian Grinstead [:bgrins]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #234)
> Comment on attachment 8514457 [details] [diff] [review]
> lwtheme-preview-listener.patch
> 
> Review of attachment 8514457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Taking the review for Dao and answering your question. This should be good
> now that we are calling _positionPinnedTabs in both directions.

Great! Here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=634677921483

Comment 236

[Dão Gottwald [:dao]]

Comment on attachment 8514457 [details] [diff] [review]
lwtheme-preview-listener.patch

>   observe: function (subject, topic, data) {
>+    if (topic == "lightweight-theme-styling-update") {
>+      let newTheme = JSON.parse(data);
>+      if (!newTheme || newTheme.id === this.defaultThemeID) {

You should never get this notification with the default theme ID. The default theme is not a lightweight theme. Please file a bug on this.

Comment 237

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/c0f433bceae7

Comment 238

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Thanks for the review Gijs.  Re-requesting for osx/shared/linux pieces.  Matt has taken a look at the Windows code and ran the build through mozscreenshots, so he has some feedback that I believe he will post later.  I've left notes from your prior review inline:

(In reply to :Gijs Kruitbosch from comment #228)
> Comment on attachment 8513612 [details] [diff] [review]
> devedition-primary.patch
> 
> Review of attachment 8513612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This was going to be r=me for OS X and shared styling, with the two comments
> below addressed. However... I noticed the back/fwd button have no hover
> style on either OS X or Linux. Can we fix that? :-)
> 

Fixed

> Linux needs some more work. There are some comments below already. The other
> thing I was wondering is why the menubar is unstyled on Linux, too (which
> looks quite odd/jarring), and why the buttons have a different border-radius
> style.

I'm not sure exactly what you mean by unstyled menubar here.  I'm seeing something like this: https://www.dropbox.com/s/lrulcbfqh7thos6/Screenshot%202014-10-30%2018.30.42.png?dl=0, which I looks alright to me since it matches the title bar above.  Also, which buttons have a different border-radius style?  I've updated some of the back/forward styling from your notes later, so maybe that fixes it?

> ::: browser/themes/linux/devedition.css
> @@ +4,5 @@
> >  
> >  %include ../shared/devedition.inc.css
> > +
> > +.close-icon,
> > +.tab-close-button:not([selected]):not(:hover):not([brighttext]) {
> 
> Not sure why there's a :not([brighttext]) here; the consequence of this is
> that I'm seeing dark close icons instead of the bright close icons that I
> see on OS X. Not intentional, I presume?
> 

OK, so there were a couple of things wrong with this.  First, it was including close-icon which it shouldn't have been.  Second, there was an lw theme selector in browser.css that wasn't being applied so I changed in linux/browser.css to use TabsToolbar[brighttext] instead of :-moz-lwtheme-brighttext.  The only change we needed to make now in this file is to .tab-close-button[selected]:not(:hover).

> @@ +10,5 @@
> > +}
> > +
> > +/* Square back and forward buttons */
> > +#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> > +#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> 
> This is on the icons; the buttons are too big on my Ubuntu VM:
> http://imgur.com/rmvGHLF
> 
> They also seem to have an odd semi-transparent border...

Fixed and fixed

> 
> @@ +11,5 @@
> > +
> > +/* Square back and forward buttons */
> > +#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> > +#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> > +  height: 22px;
> 
> You shouldn't need to set the height of these.
> 

Removed

> ::: browser/themes/shared/devedition.inc.css
> @@ +54,5 @@
> > +
> > +  /* Menu button separator */
> > +  --panel-ui-button-background-image: linear-gradient(to bottom, #5E6670, #5E6670);
> > +  --panel-ui-button-background-size: 1px calc(100% - 1px);
> > +  --panel-ui-button-background-position: 0px 0px;
> 
> This should be 1px 0px. The background in the default theme is basically
> "light, dark, light", with the visual "border" being in the middle. This
> "border" should be in the same position to avoid it looking like it shifts
> when you toggle the theme (and to keep the spacing of the icon symmetrical).
> 

Updated

> ::: browser/themes/shared/devedition/search.svg
> @@ +23,5 @@
> > +    fill: #B6BABF;
> > +  }
> > +
> > +  use[id*="-mac"] {
> > +    transform: translate(16px) scale(-1,1);
> 
> nit: scaleX(-1)

Updated

Comment 239

[Dão Gottwald [:dao]]

Comment on attachment 8513612 [details] [diff] [review]
devedition-primary.patch

>--- a/browser/themes/linux/devedition.css
>+++ b/browser/themes/linux/devedition.css

>+.tab-close-button:not([selected]):not(:hover):not([brighttext]) {

The brighttext attribute will never be set here.

>+/* Square back and forward buttons */
>+#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
>+#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
>+  height: 22px;

This isn't going to work with large font sizes that make the location bar taller.

>--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>@@ -15,16 +15,25 @@
> %define exitSubviewGutterWidth 38px
> %define buttonStateHover :not(:-moz-any([disabled],[open],:active)):hover
> %define menuStateHover :not(:-moz-any([disabled],:active))[_moz-menuactive]
> %define buttonStateActive :not([disabled]):-moz-any([open],:hover:active)
> %define menuStateActive :not([disabled])[_moz-menuactive]:active
> 
> %include ../browser.inc
> 
>+:root {
>+  --panel-ui-button-background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, hsla(0,0%,100%,0)),
>+                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.3) 30%, hsla(210,54%,20%,.3) 70%, hsla(210,54%,20%,0)),
>+                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, hsla(0,0%,100%,0));
>+  --panel-ui-button-background-size: 1px calc(100% - 1px), 1px calc(100% - 1px), 1px  calc(100% - 1px) !important;
>+  --panel-ui-button-background-position: 0px 0px, 1px 0px, 2px 0px;
>+  --panel-ui-button-background-repeat: no-repeat;
>+}

> #PanelUI-button {
>-  background-image: linear-gradient(to bottom, transparent, hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, transparent),
>-                    linear-gradient(to bottom, transparent, hsla(210,54%,20%,.3) 30%, hsla(210,54%,20%,.3) 70%, transparent),
>-                    linear-gradient(to bottom, transparent, hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, transparent);
>-  background-size: 1px calc(100% - 1px), 1px calc(100% - 1px), 1px  calc(100% - 1px) !important;
>-  background-position: 0px 0px, 1px 0px, 2px 0px;
>-  background-repeat: no-repeat;
>+  background-image: var(--panel-ui-button-background-image);
>+  background-size: var(--panel-ui-button-background-size);
>+  background-position: var(--panel-ui-button-background-position);
>+  background-repeat: var(--panel-ui-button-background-repeat);
> }

Don't replace transparent with hsla(0,0%,100%,0) & Co.

>--- a/browser/themes/shared/devedition.inc.css
>+++ b/browser/themes/shared/devedition.inc.css

>+  --chrome-tab-background-color: #1C2126;
>+  --chrome-tab-color: #ced3d9;
>+  --chrome-tab-hover-background-color: hsla(206,37%,4%,.5);
>+  --chrome-tab-separator-color: #464C50;
>+  --chrome-tab-selection-color: #f5f7fa;
>+  --chrome-tab-selection-background-color: #1a4666;
>+  --chrome-tab-selection-box-shadow: 0 2px 0 #d7f1ff inset,
>+                                     0 8px 3px -5px #2b82bf inset,
>+                                     0 -2px 0 rgba(0,0,0,.2) inset;
>+  --chrome-navigator-toolbox-separator-color: rgba(0,0,0,.2);

I don't see the point of the "chrome-" prefixes here.

>+  /* Identity box */
>+  --identity-box-chrome-color: #46afe3;
>+  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);

ditto

>+#main-window .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox {
>+  padding-left: 0;
>+  padding-right: 0;
>+}

Remove #main-window

>+#TabsToolbar {
>+  text-shadow: none !important;
>+  color: var(--chrome-color) !important; /* Make sure that the brighttext attribute is added */

What platform are these two things actually needed for?

>+#TabsToolbar::after {
>+  content: none;
>+}

Like I said, this only exists on Windows. It shouldn't be in the shared file. Also, use display: none.

>+#nav-bar {
>+  margin-top:0 !important;

Add a space after the colon

>--- a/browser/themes/windows/devedition.css
>+++ b/browser/themes/windows/devedition.css

>\ No newline at end of file

Add newline

Comment 240

[Dão Gottwald [:dao]]

Comment on attachment 8514728 [details] [diff] [review]
devedition-primary.patch

>--- a/browser/themes/linux/browser.css
>+++ b/browser/themes/linux/browser.css

> /* Tab close button */
> .tab-close-button:not([selected]):not(:hover) {
>   background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 64, 16, 48);
> }

>-.tab-close-button:not([selected]):not(:hover):-moz-lwtheme-darktext {
>+.tab-close-button:not([selected]):not(:hover) { {
>   background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 96, 16, 80);
> }

The syntax is broken, and if this rule worked, it would make the other one above entirely redundant.

Looks like a good number of my comments on the previous version still apply here.

Comment 241

[Brian Grinstead [:bgrins]]

Comment on attachment 8514728 [details] [diff] [review]
devedition-primary.patch

Leaving the feedback request for Matt for the Windows changes

Comment 242

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #239)
> Comment on attachment 8513612 [details] [diff] [review]
> devedition-primary.patch
> 
> >--- a/browser/themes/linux/devedition.css
> >+++ b/browser/themes/linux/devedition.css
> 
> >+.tab-close-button:not([selected]):not(:hover):not([brighttext]) {
> 
> The brighttext attribute will never be set here.

Already fixed in the most updated patch

> >+/* Square back and forward buttons */
> >+#back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> >+#forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> >+  height: 22px;
> 
> This isn't going to work with large font sizes that make the location bar
> taller.
> 

Already fixed in the most updated patch

> >--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css
> >+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css
> >@@ -15,16 +15,25 @@
> > %define exitSubviewGutterWidth 38px
> > %define buttonStateHover :not(:-moz-any([disabled],[open],:active)):hover
> > %define menuStateHover :not(:-moz-any([disabled],:active))[_moz-menuactive]
> > %define buttonStateActive :not([disabled]):-moz-any([open],:hover:active)
> > %define menuStateActive :not([disabled])[_moz-menuactive]:active
> > 
> > %include ../browser.inc
> > 
> >+:root {
> >+  --panel-ui-button-background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, hsla(0,0%,100%,0)),
> >+                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.3) 30%, hsla(210,54%,20%,.3) 70%, hsla(210,54%,20%,0)),
> >+                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, hsla(0,0%,100%,0));
> >+  --panel-ui-button-background-size: 1px calc(100% - 1px), 1px calc(100% - 1px), 1px  calc(100% - 1px) !important;
> >+  --panel-ui-button-background-position: 0px 0px, 1px 0px, 2px 0px;
> >+  --panel-ui-button-background-repeat: no-repeat;
> >+}
> 
> > #PanelUI-button {
> >-  background-image: linear-gradient(to bottom, transparent, hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, transparent),
> >-                    linear-gradient(to bottom, transparent, hsla(210,54%,20%,.3) 30%, hsla(210,54%,20%,.3) 70%, transparent),
> >-                    linear-gradient(to bottom, transparent, hsla(0,0%,100%,.3) 30%, hsla(0,0%,100%,.3) 70%, transparent);
> >-  background-size: 1px calc(100% - 1px), 1px calc(100% - 1px), 1px  calc(100% - 1px) !important;
> >-  background-position: 0px 0px, 1px 0px, 2px 0px;
> >-  background-repeat: no-repeat;
> >+  background-image: var(--panel-ui-button-background-image);
> >+  background-size: var(--panel-ui-button-background-size);
> >+  background-position: var(--panel-ui-button-background-position);
> >+  background-repeat: var(--panel-ui-button-background-repeat);
> > }
> 
> Don't replace transparent with hsla(0,0%,100%,0) & Co.
> 

Done

> >--- a/browser/themes/shared/devedition.inc.css
> >+++ b/browser/themes/shared/devedition.inc.css
> 
> >+  --chrome-tab-background-color: #1C2126;
> >+  --chrome-tab-color: #ced3d9;
> >+  --chrome-tab-hover-background-color: hsla(206,37%,4%,.5);
> >+  --chrome-tab-separator-color: #464C50;
> >+  --chrome-tab-selection-color: #f5f7fa;
> >+  --chrome-tab-selection-background-color: #1a4666;
> >+  --chrome-tab-selection-box-shadow: 0 2px 0 #d7f1ff inset,
> >+                                     0 8px 3px -5px #2b82bf inset,
> >+                                     0 -2px 0 rgba(0,0,0,.2) inset;
> >+  --chrome-navigator-toolbox-separator-color: rgba(0,0,0,.2);
> 
> I don't see the point of the "chrome-" prefixes here.
> 

The intention was so that we didn't have a variable called '--color', '--background-color', etc by being a little more descriptive in what it is styling.  I don't feel too strongly about that particular prefix.  The stuff that's just targeting tabs could probably be shortened to 'tab-*'.

> >+  /* Identity box */
> >+  --identity-box-chrome-color: #46afe3;
> >+  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> 
> ditto
> 
> >+#main-window .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox {
> >+  padding-left: 0;
> >+  padding-right: 0;
> >+}
> 
> Remove #main-window
> 

Done

> >+#TabsToolbar {
> >+  text-shadow: none !important;
> >+  color: var(--chrome-color) !important; /* Make sure that the brighttext attribute is added */
> 
> What platform are these two things actually needed for?
> 
> >+#TabsToolbar::after {
> >+  content: none;
> >+}
> 
> Like I said, this only exists on Windows. It shouldn't be in the shared
> file. Also, use display: none.
> 

Done

> >+#nav-bar {
> >+  margin-top:0 !important;
> 
> Add a space after the colon

Done

> 
> >--- a/browser/themes/windows/devedition.css
> >+++ b/browser/themes/windows/devedition.css
> 
> >\ No newline at end of file
> 
> Add newline

Done

Comment 243

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #240)
> Comment on attachment 8514728 [details] [diff] [review]
> devedition-primary.patch
> 
> >--- a/browser/themes/linux/browser.css
> >+++ b/browser/themes/linux/browser.css
> 
> > /* Tab close button */
> > .tab-close-button:not([selected]):not(:hover) {
> >   background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 64, 16, 48);
> > }
> 
> >-.tab-close-button:not([selected]):not(:hover):-moz-lwtheme-darktext {
> >+.tab-close-button:not([selected]):not(:hover) { {
> >   background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 96, 16, 80);
> > }
> 
> The syntax is broken, and if this rule worked, it would make the other one
> above entirely redundant.
> 

Oops, copy / paste failure.  I'll readd the :-moz-lwtheme-darktext on that selector.


> Looks like a good number of my comments on the previous version still apply
> here.

I've tried to address everything, but I could have missed things - which comments still apply?

Comment 244

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

Addresses feedback from Dão.  See Comment 238 for more info about the patch

Comment 245

[Carsten Book [:Tomcat]]

(In reply to Brian Grinstead [:bgrins] from comment #237)
> https://hg.mozilla.org/integration/fx-team/rev/c0f433bceae7

Hey Brian,

sorry had to back this out since this caused frequent memory leaks in windows 7 debug bc-1 tests like shown here on the retrigger in https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=c0f433bceae7

Comment 246

[Matthew N. [:MattN]]

Comment on attachment 8514769 [details] [diff] [review]
devedition-primary.patch

I did my review with Windows screenshots on XP and Win7 (Glass, Basic, & Classic) on attachment 8513612 [details] [diff] [review]. Comments are at https://reviewboard.mozilla.org/r/131/
"The two issues which affect usability and therefore should be fixed sooner rather than later are the PB indicator and the black menu bar text."

Note that I didn't test Win8.

Comment 247

[Carsten Book [:Tomcat]]

(In reply to Brian Grinstead [:bgrins] from comment #235)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #234)
> > Comment on attachment 8514457 [details] [diff] [review]
> > lwtheme-preview-listener.patch
> > 
> > Review of attachment 8514457 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Taking the review for Dao and answering your question. This should be good
> > now that we are calling _positionPinnedTabs in both directions.
> 
> Great! Here is a try push:
> https://tbpl.mozilla.org/?tree=Try&rev=634677921483

i also retriggered the bc1-win7 debug tests a few times and seems this leak is also now there in 7/10 test runs :(

Comment 248

[:Gijs (out for now; he/him)]

Attachment: linux unthemed menubar

The menubar matching the titlebar depends on your gnome/gtk theme. This is what it looks like for me.

Comment 249

[:Gijs (out for now; he/him)]

Attachment: home button mac

Comment 250

[:Gijs (out for now; he/him)]

Attachment: home button linux

This shows the difference in border-radius

Comment 251

[:Gijs (out for now; he/him)]

Attachment: Buttons on Linux in the tabbar have the wrong background

Comment 252

[:Gijs (out for now; he/him)]

Attachment: Linux - Hover state of bookmark items on bookmark toolbar should get dark text

Comment 253

[:Gijs (out for now; he/him)]

Comment on attachment 8515005 [details]
Linux - Hover state of bookmark items on bookmark toolbar should get dark text

NB: bright text lightweight themes get this right - not sure what's going wrong in the devtools case

Comment 254

[:Gijs (out for now; he/him)]

Comment on attachment 8514769 [details] [diff] [review]
devedition-primary.patch

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

I have a feeling the next one will be the one as far as OS X is concerned!

Apart from the code nits below, I have one more thing on OS X, spurred on by Matt's private browsing testing: if you turn on the titlebar and open a private window, the titlebar styling is messed up. :-(

For Linux, I added screenshots for a bunch of things (including the border-radius question you asked). The other "glaring" things are:
- as mentioned in bug 1091711, the new tab button looks 'off' (NB: also affects lw themes, but /should/ just be a question of using brighttext attribute...)
- the bookmarks button has a broken hover state when in the navbar - the separator in the middle goes missing, and there's an odd 'dip' in the contour of the dark hover background near where the separator should be. Probably a border-radius thing on the individual parts of the button?

::: browser/themes/shared/devedition.inc.css
@@ +164,5 @@
> +/* Tab styling - make sure to use an inverted icon for the selected tab
> +   (brighttext only covers the unselected tabs) */
> +.tab-close-button[selected=true]:not(:hover) {
> +  -moz-image-region: rect(0, 64px, 16px, 48px);
> +}

This doesn't work on Linux; maybe make it mac/windows only, either with an ifdef or by having it in both the windows and mac stylesheets? (I'd lean towards the ifdef)

@@ +167,5 @@
> +  -moz-image-region: rect(0, 64px, 16px, 48px);
> +}
> +@media (min-resolution: 2dppx) {
> +  .tab-close-button[selected=true]:not(:hover) {
> +    -moz-image-region: rect(0, 128px, 32px, 96px);

This should be OS X only, so as not to break windows hidpi, I think

Comment 255

[:Gijs (out for now; he/him)]

... or, of course, I could poke some more and find more OS X issues:

- the pinned tab glow is a little messed up on OS X as well
- opening customize mode makes the tabs jump down when using tabsintitlebar


I think at some point we should just start landing stuff and incrementally fix fallout. I am... honestly not quite sure what our odds are of having this polished and finished on aurora by nov. 10. :-(

Comment 256

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #255)
> ... or, of course, I could poke some more and find more OS X issues:
> 
> - the pinned tab glow is a little messed up on OS X as well
> - opening customize mode makes the tabs jump down when using tabsintitlebar
> 

That is actually on purpose - I bumped it down because otherwise there was nowhere on the window to drag it around while in customize mode.

> 
> I think at some point we should just start landing stuff and incrementally
> fix fallout. I am... honestly not quite sure what our odds are of having
> this polished and finished on aurora by nov. 10. :-(

Yes, if we get it landed we can start opening up more specific bugs and I can get some help

Comment 257

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #254)
> - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> affects lw themes, but /should/ just be a question of using brighttext
> attribute...)

Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for the new tab button (as I mention in https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?

Comment 258

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #256)
> (In reply to :Gijs Kruitbosch from comment #255)
> > I think at some point we should just start landing stuff and incrementally
> > fix fallout. I am... honestly not quite sure what our odds are of having
> > this polished and finished on aurora by nov. 10. :-(
> 
> Yes, if we get it landed we can start opening up more specific bugs and I
> can get some help

Not to mention that this is preffed off by default in nightly / m-c and won't change anything unless if you've set the pref.

Comment 259

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #250)
> Created attachment 8514997 [details]
> home button linux
> 
> This shows the difference in border-radius

So I actually wasn't planning on changing the platform-specific border radius for toolbarbuttons (3px on osx and 2px on windows/linux).  I guess that's a shorlander question.

Comment 260

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #254)
> ::: browser/themes/shared/devedition.inc.css
> @@ +164,5 @@
> > +/* Tab styling - make sure to use an inverted icon for the selected tab
> > +   (brighttext only covers the unselected tabs) */
> > +.tab-close-button[selected=true]:not(:hover) {
> > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > +}
> 
> This doesn't work on Linux; maybe make it mac/windows only, either with an
> ifdef or by having it in both the windows and mac stylesheets? (I'd lean
> towards the ifdef)
> 

This is already overridden in linux/devedition.css.  I'd rather not ifdef for this case (since we already have separate files for each platform and it'd be only one rule to duplicate anyway).  I could move it into the windows/osx files if you think that's better than just letting the linux file override it?

> @@ +167,5 @@
> > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > +}
> > +@media (min-resolution: 2dppx) {
> > +  .tab-close-button[selected=true]:not(:hover) {
> > +    -moz-image-region: rect(0, 128px, 32px, 96px);
> 
> This should be OS X only, so as not to break windows hidpi, I think

Updated

Comment 261

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #260)
> (In reply to :Gijs Kruitbosch from comment #254)
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +164,5 @@
> > > +/* Tab styling - make sure to use an inverted icon for the selected tab
> > > +   (brighttext only covers the unselected tabs) */
> > > +.tab-close-button[selected=true]:not(:hover) {
> > > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > > +}
> > 
> > This doesn't work on Linux; maybe make it mac/windows only, either with an
> > ifdef or by having it in both the windows and mac stylesheets? (I'd lean
> > towards the ifdef)
> > 
> 
> This is already overridden in linux/devedition.css.  I'd rather not ifdef
> for this case (since we already have separate files for each platform and
> it'd be only one rule to duplicate anyway).

Agreed.

> I could move it into the
> windows/osx files if you think that's better than just letting the linux
> file override it?

+1

Comment 262

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #260)
> (In reply to :Gijs Kruitbosch from comment #254)
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +164,5 @@
> > > +/* Tab styling - make sure to use an inverted icon for the selected tab
> > > +   (brighttext only covers the unselected tabs) */
> > > +.tab-close-button[selected=true]:not(:hover) {
> > > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > > +}
> > 
> > This doesn't work on Linux; maybe make it mac/windows only, either with an
> > ifdef or by having it in both the windows and mac stylesheets? (I'd lean
> > towards the ifdef)
> > 
> 
> This is already overridden in linux/devedition.css.  I'd rather not ifdef
> for this case (since we already have separate files for each platform and
> it'd be only one rule to duplicate anyway).  I could move it into the
> windows/osx files if you think that's better than just letting the linux
> file override it?

I mean, it's just ignored, not actually overridden (-moz-image-region vs. background-image)... for clarity's sake (my initial comment on the patch was "why does Linux have its own override for the selected tab case, when there's already one in shared?") I guess moving it would be preferable. :-)

Comment 263

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #257)
> (In reply to :Gijs Kruitbosch from comment #254)
> > - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> > affects lw themes, but /should/ just be a question of using brighttext
> > attribute...)
> 
> Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for
> the new tab button (as I mention in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?

I couldn't find any open bugs; bug 947356 looks like it could have addressed it, but didn't? I guess that with full black background, the blue of the default gtk icon provides enough contrast. On the other hand, the default "Space Fantasy" theme that we ship on beta+ is dark blue-ish, and the new tab button all but disappears in that.

Just like the toolbar buttons, I imagine we can just copy the windows file for the [+] icon to linux and apply the appropriate CSS for the inverted case.

Comment 264

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #263)
> (In reply to Brian Grinstead [:bgrins] from comment #257)
> > (In reply to :Gijs Kruitbosch from comment #254)
> > > - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> > > affects lw themes, but /should/ just be a question of using brighttext
> > > attribute...)
> > 
> > Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for
> > the new tab button (as I mention in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?
> 
> I couldn't find any open bugs; bug 947356 looks like it could have addressed
> it, but didn't? I guess that with full black background, the blue of the
> default gtk icon provides enough contrast. On the other hand, the default
> "Space Fantasy" theme that we ship on beta+ is dark blue-ish, and the new
> tab button all but disappears in that.
> 
> Just like the toolbar buttons, I imagine we can just copy the windows file
> for the [+] icon to linux and apply the appropriate CSS for the inverted
> case.

Sounds good to me - can we do this in a new dark lw theme bug like for how we handled the download glow icon?

Comment 265

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #263)
> (In reply to Brian Grinstead [:bgrins] from comment #257)
> > (In reply to :Gijs Kruitbosch from comment #254)
> > > - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> > > affects lw themes, but /should/ just be a question of using brighttext
> > > attribute...)
> > 
> > Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for
> > the new tab button (as I mention in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?
> 
> I couldn't find any open bugs; bug 947356 looks like it could have addressed
> it, but didn't?

I think you're looking for https://bugzilla.mozilla.org/show_bug.cgi?id=940326

Comment 266

[Dão Gottwald [:dao]]

Comment on attachment 8514769 [details] [diff] [review]
devedition-primary.patch

>--- a/browser/themes/shared/devedition.inc.css
>+++ b/browser/themes/shared/devedition.inc.css

>+  /* Toolbar buttons */
>+  --toolbarbutton-hover-background: rgba(25, 33, 38, .6) linear-gradient(rgba(25, 33, 38, .6), rgba(25, 33, 38, .6)) padding-box;
>+  --toolbarbutton-hover-boxshadow: none;
>+  --toolbarbutton-hover-bordercolor: rgba(25, 33, 38, .6);
>+  --toolbarbutton-active-background: rgba(25, 33, 38, 1) linear-gradient(rgba(25, 33, 38, 1), rgba(25, 33, 38, 1)) border-box;

rgba(..., 1) should just be rgb(...), and please remove the spaces from within these color values. Also, why are you using the same value as the background color and for the background image (which isn't really a gradient but a solid background image)? Why set a background image at all here?

Comment 267

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #265)
> (In reply to :Gijs Kruitbosch from comment #263)
> > (In reply to Brian Grinstead [:bgrins] from comment #257)
> > > (In reply to :Gijs Kruitbosch from comment #254)
> > > > - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> > > > affects lw themes, but /should/ just be a question of using brighttext
> > > > attribute...)
> > > 
> > > Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for
> > > the new tab button (as I mention in
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?
> > 
> > I couldn't find any open bugs; bug 947356 looks like it could have addressed
> > it, but didn't?
> 
> I think you're looking for
> https://bugzilla.mozilla.org/show_bug.cgi?id=940326

we can fix the devtools/lwtheme issue with the existing windows inverted asset. that can't be said for that bug, which is asking for updated linux assets, AFAICT.

Comment 268

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #267)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #265)
> > (In reply to :Gijs Kruitbosch from comment #263)
> > > (In reply to Brian Grinstead [:bgrins] from comment #257)
> > > > (In reply to :Gijs Kruitbosch from comment #254)
> > > > > - as mentioned in bug 1091711, the new tab button looks 'off' (NB: also
> > > > > affects lw themes, but /should/ just be a question of using brighttext
> > > > > attribute...)
> > > > 
> > > > Is there a bug opened for not using moz-icon://stock/gtk-add?size=menu for
> > > > the new tab button (as I mention in
> > > > https://bugzilla.mozilla.org/show_bug.cgi?id=1091711#c3)?
> > > 
> > > I couldn't find any open bugs; bug 947356 looks like it could have addressed
> > > it, but didn't?
> > 
> > I think you're looking for
> > https://bugzilla.mozilla.org/show_bug.cgi?id=940326
> 
> we can fix the devtools/lwtheme issue with the existing windows inverted
> asset. that can't be said for that bug, which is asking for updated linux
> assets, AFAICT.

I've commented in 940326 - I don't see any reason why we can't just use the windows icons in that bug instead of waiting for assets.

Comment 269

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary-with-windows-fixes.patch

(In reply to Matthew N. [:MattN] (focused on Loop) from comment #246)
> Comment on attachment 8514769 [details] [diff] [review]
> devedition-primary.patch
> 
> I did my review with Windows screenshots on XP and Win7 (Glass, Basic, &
> Classic) on attachment 8513612 [details] [diff] [review]. Comments are at
> https://reviewboard.mozilla.org/r/131/
> "The two issues which affect usability and therefore should be fixed sooner
> rather than later are the PB indicator and the black menu bar text."
> 

Thanks!  I left more comments on reviewboard, but I believe that I have fixes for the PB indicator, the black menu bar text, the non-disabled scroll arrows, and the window controls overlapping.  So I'm going to ask you for review on the windows/devedition.css file.

Comment 270

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

(In reply to :Gijs Kruitbosch from comment #248)
> Created attachment 8514988 [details]
> linux unthemed menubar
> 
> The menubar matching the titlebar depends on your gnome/gtk theme. This is
> what it looks like for me.

OK, I've added the following rules, which seem to make it work for Ambiance/Radiance themes on Ubuntu:

#toolbar-menubar {
  -moz-appearance: none;
}
#toolbar-menubar menubar {
  color: var(--chrome-color);
}
#toolbar-menubar menu:not([open]) {
  color: inherit;
}

I have a feeling it may be harder than this to cover all cases (go figure), so if it becomes a big change I'd like to do it in a follow up if you don't mind.

(In reply to :Gijs Kruitbosch from comment #254)
> Comment on attachment 8514769 [details] [diff] [review]
> devedition-primary.patch
> 
> Review of attachment 8514769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a feeling the next one will be the one as far as OS X is concerned!
> 
> Apart from the code nits below, I have one more thing on OS X, spurred on by
> Matt's private browsing testing: if you turn on the titlebar and open a
> private window, the titlebar styling is messed up. :-(
> 

Removed the titlebar-content styling altogether - it isn't needed.  It fixes this problem.

> - the bookmarks button has a broken hover state when in the navbar - the
> separator in the middle goes missing, and there's an odd 'dip' in the
> contour of the dark hover background near where the separator should be.
> Probably a border-radius thing on the individual parts of the button?

Looks like any button in the navbar is getting the default hover state, which tbh doesn't look bad at all on the dark navbar besides that border radius.  I'll look at it in the meantime, but this is all non-default UI, so could this possibly be handled in a follow up?

> ::: browser/themes/shared/devedition.inc.css
> @@ +164,5 @@
> > +/* Tab styling - make sure to use an inverted icon for the selected tab
> > +   (brighttext only covers the unselected tabs) */
> > +.tab-close-button[selected=true]:not(:hover) {
> > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > +}
> 
> This doesn't work on Linux; maybe make it mac/windows only, either with an
> ifdef or by having it in both the windows and mac stylesheets? (I'd lean
> towards the ifdef)
> 
> @@ +167,5 @@
> > +  -moz-image-region: rect(0, 64px, 16px, 48px);
> > +}
> > +@media (min-resolution: 2dppx) {
> > +  .tab-close-button[selected=true]:not(:hover) {
> > +    -moz-image-region: rect(0, 128px, 32px, 96px);
> 
> This should be OS X only, so as not to break windows hidpi, I think

Split these out into each platform's files

(In reply to :Gijs Kruitbosch from comment #255)
> ... or, of course, I could poke some more and find more OS X issues:
> 
> - the pinned tab glow is a little messed up on OS X as well

Yeah, I've asked shorlander for a design on this

(In reply to :Gijs Kruitbosch from comment #253)
> Comment on attachment 8515005 [details]
> Linux - Hover state of bookmark items on bookmark toolbar should get dark
> text
> 
> NB: bright text lightweight themes get this right - not sure what's going
> wrong in the devtools case

It's because the bookmarks-item use -moz-appearance for styling.  I added a 'fix' that just copies the selector list for the toolbarbutton-1 styling, but I'm not sure if that's what we want to do long-term.

Comment 271

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #270)
> > Comment on attachment 8515005 [details]
> > Linux - Hover state of bookmark items on bookmark toolbar should get dark
> > text
> > 
> > NB: bright text lightweight themes get this right - not sure what's going
> > wrong in the devtools case
> 
> It's because the bookmarks-item use -moz-appearance for styling.  I added a
> 'fix' that just copies the selector list for the toolbarbutton-1 styling,
> but I'm not sure if that's what we want to do long-term.

Just realized that this is the exact same problem with the nav-bar buttons also.  We could alternatively *not* set the text color on hover for the bookmarks buttons and just let the default styling take over as it does on lw themes

Comment 272

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #270)
> (In reply to :Gijs Kruitbosch from comment #248)
> > Created attachment 8514988 [details]
> > linux unthemed menubar
> > 
> > The menubar matching the titlebar depends on your gnome/gtk theme. This is
> > what it looks like for me.
> 
> OK, I've added the following rules, which seem to make it work for
> Ambiance/Radiance themes on Ubuntu:

> #toolbar-menubar menubar {

#main-menubar

> #toolbar-menubar menu:not([open]) {

#main-menubar > menu:not([open]), otherwise you're targeting menus in the menupopups.

Comment 273

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary.patch

(In reply to Brian Grinstead [:bgrins] from comment #271)
> (In reply to Brian Grinstead [:bgrins] from comment #270)
> > > Comment on attachment 8515005 [details]
> > > Linux - Hover state of bookmark items on bookmark toolbar should get dark
> > > text
> > > 
> > > NB: bright text lightweight themes get this right - not sure what's going
> > > wrong in the devtools case
> > 
> > It's because the bookmarks-item use -moz-appearance for styling.  I added a
> > 'fix' that just copies the selector list for the toolbarbutton-1 styling,
> > but I'm not sure if that's what we want to do long-term.
> 
> Just realized that this is the exact same problem with the nav-bar buttons
> also.  We could alternatively *not* set the text color on hover for the
> bookmarks buttons and just let the default styling take over as it does on
> lw themes

Yeah, so if in linux/devedition.css I add this rule instead:

/* Allow buttons with -moz-appearance set to look normal on hover and open states */
#navigator-toolbox toolbarbutton:not(.subviewbutton):-moz-any(:hover, [open="true"]) {
  color: initial;
}

Then this allows the normal hover / open effect (which on linux is using -moz-appearance).  I can then remove the huge duplicated selector list - I think this is better because it is consistent with how the other themes work on the linux platform.  If these buttons get custom styling at some point they can use the existing variables.  What do you think?

(In reply to Dão Gottwald [:dao] from comment #272)
> (In reply to Brian Grinstead [:bgrins] from comment #270)
> > (In reply to :Gijs Kruitbosch from comment #248)
> > > Created attachment 8514988 [details]
> > > linux unthemed menubar
> > > 
> > > The menubar matching the titlebar depends on your gnome/gtk theme. This is
> > > what it looks like for me.
> > 
> > OK, I've added the following rules, which seem to make it work for
> > Ambiance/Radiance themes on Ubuntu:
> 
> > #toolbar-menubar menubar {
> 
> #main-menubar
> 
> > #toolbar-menubar menu:not([open]) {
> 
> #main-menubar > menu:not([open]), otherwise you're targeting menus in the
> menupopups.

Thanks, I've updated the patch

Comment 274

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #188)
>  > (In reply to Brian Grinstead [:bgrins] from comment #142)
> > > (In reply to :Gijs Kruitbosch from comment #138)
> > > > - this seems to unconditionally apply even when I select the light theme in
> > > > devtools. My thinking was that we could continue to use the default theme
> > > > there for now? Is that difficult to implement? :-)
> > > 
> > > That'd be easy to implement, pending a UX decision about it.
> > 
> > Was a UX decision made or solicited?
> > 
> 
> Stephen, if the light devtools theme is applied, should we (1) keep the dark
> browser theme or (2) switch to Australis styling?  Ideally there would be a
> (3) here, which is use the devedition theme but with light colors, but
> that's not likely to happen for the initial release.

Heard back about this - for now we will switch to Australis when the user switches to the light devtools theme.

Comment 275

[Brian Grinstead [:bgrins]]

> > - the bookmarks button has a broken hover state when in the navbar - the
> > separator in the middle goes missing, and there's an odd 'dip' in the
> > contour of the dark hover background near where the separator should be.
> > Probably a border-radius thing on the individual parts of the button?
> 
> Looks like any button in the navbar is getting the default hover state,
> which tbh doesn't look bad at all on the dark navbar besides that border
> radius.  I'll look at it in the meantime, but this is all non-default UI, so
> could this possibly be handled in a follow up?

I was confused here when I was replied here because I misread this and I was adding the buttons to the *menu* bar, not the nav bar.   Now I see what you meant.  There are separate issues with it on the menu bar and the nav bar, but I'll focus first on fixing the nav bar since that is the default location.

Comment 276

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #275)
> > > - the bookmarks button has a broken hover state when in the navbar - the
> > > separator in the middle goes missing, and there's an odd 'dip' in the
> > > contour of the dark hover background near where the separator should be.
> > > Probably a border-radius thing on the individual parts of the button?
> > 
> > Looks like any button in the navbar is getting the default hover state,
> > which tbh doesn't look bad at all on the dark navbar besides that border
> > radius.  I'll look at it in the meantime, but this is all non-default UI, so
> > could this possibly be handled in a follow up?
> 
> I was confused here when I was replied here because I misread this and I was
> adding the buttons to the *menu* bar, not the nav bar.   Now I see what you
> meant.  There are separate issues with it on the menu bar and the nav bar,
> but I'll focus first on fixing the nav bar since that is the default
> location.

I looked more into this - the 'dip' seems to be an issue even on stable Linux AFAICT.  It's just not as noticeable because you can actually see a border.  Currently, the --toolbarbutton-hover-bordercolor matches the --toolbarbutton-hover-bordercolor.  I could see updating the border color for all buttons to be a bit lighter than the hover color to make things look a little nicer for this case.

Comment 277

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #276)
> (In reply to Brian Grinstead [:bgrins] from comment #275)
> > > > - the bookmarks button has a broken hover state when in the navbar - the
> > > > separator in the middle goes missing, and there's an odd 'dip' in the
> > > > contour of the dark hover background near where the separator should be.
> > > > Probably a border-radius thing on the individual parts of the button?
> > > 
> > > Looks like any button in the navbar is getting the default hover state,
> > > which tbh doesn't look bad at all on the dark navbar besides that border
> > > radius.  I'll look at it in the meantime, but this is all non-default UI, so
> > > could this possibly be handled in a follow up?
> > 
> > I was confused here when I was replied here because I misread this and I was
> > adding the buttons to the *menu* bar, not the nav bar.   Now I see what you
> > meant.  There are separate issues with it on the menu bar and the nav bar,
> > but I'll focus first on fixing the nav bar since that is the default
> > location.
> 
> I looked more into this - the 'dip' seems to be an issue even on stable
> Linux AFAICT.  It's just not as noticeable because you can actually see a
> border.  Currently, the --toolbarbutton-hover-bordercolor matches the
> --toolbarbutton-hover-bordercolor.  I could see updating the border color
> for all buttons to be a bit lighter than the hover color to make things look
> a little nicer for this case.

OK. Let's file a followup bug about this.

(In reply to Brian Grinstead [:bgrins] from comment #270)
> Created attachment 8515168 [details] [diff] [review]
> devedition-primary.patch
> 
> 
> (In reply to :Gijs Kruitbosch from comment #248)
> > Created attachment 8514988 [details]
> > linux unthemed menubar
> > 
> > The menubar matching the titlebar depends on your gnome/gtk theme. This is
> > what it looks like for me.
> 
> OK, I've added the following rules, which seem to make it work for
> Ambiance/Radiance themes on Ubuntu:
> 
> #toolbar-menubar {
>   -moz-appearance: none;
> }
> #toolbar-menubar menubar {
>   color: var(--chrome-color);
> }
> #toolbar-menubar menu:not([open]) {
>   color: inherit;
> }
> 
> I have a feeling it may be harder than this to cover all cases (go figure),
> so if it becomes a big change I'd like to do it in a follow up if you don't
> mind.

Yeah, I trust that this works, esp. after Dão's suggestions, and we can followup if necessary after landing this. Considering it's preffed off, and just CSS, I'm fairly happy just landing this in the state it's currently in.

> (In reply to :Gijs Kruitbosch from comment #255)
> > - the pinned tab glow is a little messed up on OS X as well
> 
> Yeah, I've asked shorlander for a design on this

OK, let's do a followup here, too.

Comment 278

[:Gijs (out for now; he/him)]

Comment on attachment 8515187 [details] [diff] [review]
devedition-primary.patch

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

r=me on the OS X and Linux bits based on the interdiff.

::: browser/themes/linux/devedition.css
@@ +19,5 @@
> +  color: inherit;
> +}
> +
> +/* Allow buttons with -moz-appearance set to look normal on hover and open states */
> +#navigator-toolbox toolbarbutton:not(.subviewbutton):-moz-any(:hover, [open="true"]) {

Nit: By convention, we only modify things with .toolbarbutton-1 so add-ons can opt out of our styling. So we should be using that here. Unfortunately, that doesn't fix the bookmarks case, so we should add a selector for toolbarbutton.bookmark-item to this rule:

#navigator-toolbox .toolbarbutton-1:not(.subviewbutton):-moz-any(:hover, [open="true"]),
toolbarbutton.bookmark-item:not(.subviewbutton):-moz-any(:hover, [open="true"]) {
...
}

(untested if that still overrides the right things, so please check. If not, please leave this out and file a followup)

Comment 279

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener.patch

Still working on tracking down the leak.  It seems to be somewhere in the test, as this push doesn't leak: https://tbpl.mozilla.org/?tree=Try&rev=cc324f595384.  I've also tried commenting out the _positionPinnedTabs stuff, which didn't seem to make a difference.

This version removes the devedition styling when the devtools light theme is applied, switching to australis instead so it will match better.

Comment 280

[:Gijs (out for now; he/him)]

Comment on attachment 8515371 [details] [diff] [review]
lwtheme-preview-listener.patch

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

FTR, the other test here that uses LWTManager is disabled in e10s... dunno if this needs the same treatment or not...

Otherwise, rs=me in terms of the update as compared to the previous patch, which is fairly trivial.

If this doesn't solve leaks yet, please give my suggestion on IRC (make styleSheet load handler the object with a handleEvent method, instead of a closure'd callback function, and always removeEventListener when deleting this.styleSheet) a shot. Might be worth doing anyway in terms of code simplicity.

::: browser/base/content/browser-devedition.js
@@ +31,5 @@
>  
>    observe: function (subject, topic, data) {
> +    if (topic == "lightweight-theme-styling-update") {
> +      let newTheme = JSON.parse(data);
> +      if (!newTheme || newTheme.id === this.defaultThemeID) {

As Dão said, please file a followup bug for this. We should fix the case that makes this happen.

Comment 281

[Matthew N. [:MattN]]

Comment on attachment 8515138 [details] [diff] [review]
devedition-primary-with-windows-fixes.patch

Comments from Win7 (Glass, Basic, & Classic) are at https://reviewboard.mozilla.org/r/141/

I didn't re-test XP but I don't think it's a huge deal for an initial landing for something isolated like this.

Comment 282

[Brian Grinstead [:bgrins]]

Attachment: devedition-primary-r=Gijs,MattN.patch

For checkin - includes minor fixes from most recent reviews

Comment 283

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/019609619a48

Comment 284

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

(In reply to Brian Grinstead [:bgrins] from comment #283)
> https://hg.mozilla.org/integration/fx-team/rev/019609619a48

Looks like this is causing permaorange on fx-team: windows M-e10 (bc1)
https://tbpl.mozilla.org/php/getParsedLog.php?id=51591349&tree=Fx-Team

Comment 285

[Brian Grinstead [:bgrins]]

(In reply to Mark Finkle (:mfinkle) from comment #284)
> (In reply to Brian Grinstead [:bgrins] from comment #283)
> > https://hg.mozilla.org/integration/fx-team/rev/019609619a48
> 
> Looks like this is causing permaorange on fx-team: windows M-e10 (bc1)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=51591349&tree=Fx-Team

That's weird, the only reason I could think of is that the devedition.theme.enabled pref isn't getting cleared at the end of the browser_devedition.js test, but I'm not seeing any warnings in the log.  Let me see if anything else sticks out as the cause.

Comment 286

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #285)
> (In reply to Mark Finkle (:mfinkle) from comment #284)
> > (In reply to Brian Grinstead [:bgrins] from comment #283)
> > > https://hg.mozilla.org/integration/fx-team/rev/019609619a48
> > 
> > Looks like this is causing permaorange on fx-team: windows M-e10 (bc1)
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=51591349&tree=Fx-Team
> 
> That's weird, the only reason I could think of is that the
> devedition.theme.enabled pref isn't getting cleared at the end of the
> browser_devedition.js test, but I'm not seeing any warnings in the log.  Let
> me see if anything else sticks out as the cause.

It's e10s-only, which is also odd...

Comment 287

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #286)
> (In reply to Brian Grinstead [:bgrins] from comment #285)
> > (In reply to Mark Finkle (:mfinkle) from comment #284)
> > > (In reply to Brian Grinstead [:bgrins] from comment #283)
> > > > https://hg.mozilla.org/integration/fx-team/rev/019609619a48
> > > 
> > > Looks like this is causing permaorange on fx-team: windows M-e10 (bc1)
> > > https://tbpl.mozilla.org/php/getParsedLog.php?id=51591349&tree=Fx-Team
> > 
> > That's weird, the only reason I could think of is that the
> > devedition.theme.enabled pref isn't getting cleared at the end of the
> > browser_devedition.js test, but I'm not seeing any warnings in the log.  Let
> > me see if anything else sticks out as the cause.
> 
> It's e10s-only, which is also odd...

I'm confused now.  On windows or osx, when running `./mach mochitest browser/base/content/test/general/browser_devedition.js --jsdebugger` and commenting out the finish() call in that test, you can see that the stylesheet is indeed gone at the end, but the browser is still dark!  Even if you go and toggle the pref while the test window is still opened the browser is still dark.  I don't know if the fact that it is running in the test harness is making the browser display not refresh when the stylesheet is removed?  This obviously works when running the browser.

I think this is going to take some more debugging so we should probably back this out in the meantime.  I'm going to do a try push with that test disabled in windows e10s just to make sure that this is the issue.

Comment 288

[Brian Grinstead [:bgrins]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/c243417f2477

Comment 289

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #287) 
> I'm confused now.  On windows or osx, when running `./mach mochitest
> browser/base/content/test/general/browser_devedition.js --jsdebugger` and
> commenting out the finish() call in that test, you can see that the
> stylesheet is indeed gone at the end, but the browser is still dark!  Even
> if you go and toggle the pref while the test window is still opened the
> browser is still dark.  I don't know if the fact that it is running in the
> test harness is making the browser display not refresh when the stylesheet
> is removed?  This obviously works when running the browser.
> 
> I think this is going to take some more debugging so we should probably back
> this out in the meantime.  I'm going to do a try push with that test
> disabled in windows e10s just to make sure that this is the issue.

So skipping the browser_devedition test in e10s on windows fixes the error, but I don't understand why.  Here's a try push: https://tbpl.mozilla.org/?tree=Try&rev=70a8ffdb663d.  It seems the dark theme is still showing up after the test ends even though the stylesheet has been removed.  I will need to dig into further, but it's clearly not a problem when the browser is running (and only causing a test failure on windows-opt-e10s) so I'm tempted to just skip the test.

Comment 290

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener.patch

So the leak from the previous test changes turned out to be due to making two calls to LWTManager.previewTheme in a row without calling resetTheme()[0].  In the process I tried doing a refactor that Gijs suggested (using handleEvent to make sure that we could always unbind the load handler for the style sheet).  This didn't fix the leaks [1], but I like it more than how it was being done before so I'm going to leave it.  Copying over r+ with the handleEvent changes since that was a simple refactor on Gijs' suggestion.

Here is a try push [2] that didn't leak, and one I just pushed [3] for good measure.

[0]: https://tbpl.mozilla.org/?tree=Try&rev=81c78b47b336
[1]: https://tbpl.mozilla.org/?tree=Try&rev=7c90b22f4725
[2]: https://tbpl.mozilla.org/?tree=Try&rev=8d66c957afdc
[3]: https://tbpl.mozilla.org/?tree=Try&rev=896bd7ea388d

Comment 291

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #290)
> Created attachment 8515566 [details] [diff] [review]
> lwtheme-preview-listener.patch

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8515371&action=interdiff&newid=8515566&headers=1

Comment 292

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener.patch

OK, I think I've tracked this down, this patch should fix both the intermittent leak from the js changes (see Comment 290) and the weird windows e10s failure.

I think the call to manually set and reset general.skins.selectedSkin in the browser_devedition.js test was messing things up.  Removing those calls locally the 'sticky' styling problem goes away, so I'm fairly sure that it will resolve the problem.  It's not an important part of the test (it was kind of left over from when I had a listener for the theme changing), so I just removed it.

Pushed to try with both patches: https://tbpl.mozilla.org/?tree=Try&rev=522a448c8ed2

Gijs, does this look good to you?

Comment 293

[:Gijs (out for now; he/him)]

Comment on attachment 8515643 [details] [diff] [review]
lwtheme-preview-listener.patch

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

I'm a little uneasy about the previewTheme/resetPreview thing... can you elaborate on why the rest is necessary and/or we can never see two previewTheme calls without a resetPreview one inbetween?

::: browser/base/content/browser-devedition.js
@@ +80,5 @@
>  
> +    this._toggleStyleSheet(deveditionThemeEnabled);
> +  },
> +
> +  handleEvent: function() {

Nit: it's good form to use the event parameter and check the event type, even if right now, all we're registering for is "load" events.

::: browser/base/content/test/general/browser_devedition.js
@@ +98,5 @@
> +  ok (!DevEdition.styleSheet, "The devedition stylesheet is not enabled after a lightweight theme preview.");
> +  LightweightThemeManager.resetPreview();
> +  LightweightThemeManager.previewTheme(dummyLightweightTheme("preview1"));
> +  ok (!DevEdition.styleSheet, "The devedition stylesheet is not enabled after a second lightweight theme preview.");
> +  LightweightThemeManager.resetPreview();

So can it never happen in the real world that we fire two previewTheme calls without a resetPreview inbetween? I'm a little confused that this would throw anything off...

Comment 294

[Brian Grinstead [:bgrins]]

Attachment: lwtheme-preview-listener-r=Gijs.patch

(In reply to :Gijs Kruitbosch from comment #293)
> Comment on attachment 8515643 [details] [diff] [review]
> lwtheme-preview-listener.patch
> 
> Review of attachment 8515643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little uneasy about the previewTheme/resetPreview thing... can you
> elaborate on why the rest is necessary and/or we can never see two
> previewTheme calls without a resetPreview one inbetween?
> 
> ::: browser/base/content/browser-devedition.js
> @@ +80,5 @@
> >  
> > +    this._toggleStyleSheet(deveditionThemeEnabled);
> > +  },
> > +
> > +  handleEvent: function() {
> 
> Nit: it's good form to use the event parameter and check the event type,
> even if right now, all we're registering for is "load" events.
> 

Done

> ::: browser/base/content/test/general/browser_devedition.js
> @@ +98,5 @@
> > +  ok (!DevEdition.styleSheet, "The devedition stylesheet is not enabled after a lightweight theme preview.");
> > +  LightweightThemeManager.resetPreview();
> > +  LightweightThemeManager.previewTheme(dummyLightweightTheme("preview1"));
> > +  ok (!DevEdition.styleSheet, "The devedition stylesheet is not enabled after a second lightweight theme preview.");
> > +  LightweightThemeManager.resetPreview();
> 
> So can it never happen in the real world that we fire two previewTheme calls
> without a resetPreview inbetween? I'm a little confused that this would
> throw anything off...

It seems that resetPreview is typically called before previewTheme is called again.  For instance, within customize mode the previewTheme is called on focus / mouseover and resetPreview is called on blur / mouseout [0].  The addon manager always calls resetPreview [1].  I think it's safe to say that there is no other cases in test coverage that call it this way, since removing the usage here stops the leak.

I don't completely understand why calling two previews in a row is an issue on Windows debug builds, but when I looked into the lwtmanager code it is not doing the exact same thing in between these two cases.  In the previewTheme with an existing previewTimer it just calls _previewTimer.cancel() [2] instead of resetPreview() which also nulls it out and notifies windows [3].

The weird thing is that it really should make no difference at all as far as the devedition listener is concerned.  The consequence of the different behavior from the above paragraph is that we have a sequence of events like this:

// No resetPreview
previewTheme("preview0");
observe {"id":"preview0"} -> call _toggleStyleSheet(false)
previewTheme("preview1");
observe {"id":"preview1"} -> call _toggleStyleSheet(false)

Instead of:

// With resetPreview
previewTheme("preview0");
{"id":"preview0"} -> call _toggleStyleSheet(false)
resetPreview()
null -> call _updateStyleSheetFromPrefs() which will toggleStyleSheet(true/false)
previewTheme("preview1");
{"id":"preview1"} -> call _toggleStyleSheet(false)

But if you look at the code in devedition.js it's clear that doing two previews in a row the second _toggleStyleSheet won't do anything.

For instance, on this push [4], I added consecutive previewTheme() calls in the test, but did it when PREF_DEVEDITION_THEME was false, which made the devedition stuff only call _toggleStyleSheet(false) a bunch of different times in a row, but it still leaks.

[0]: http://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1311
[1]: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#359
[2]: http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#156
[3]: http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#167
[4]: https://tbpl.mozilla.org/?tree=Try&rev=827ee2bb03af / https://hg.mozilla.org/try/rev/3a628e295e47

Comment 295

[Brian Grinstead [:bgrins]]

And the most clear reduced test case that I can find to reproduce the leak: https://tbpl.mozilla.org/?tree=Try&rev=a93c88e77c75.  In this changeset (https://hg.mozilla.org/try/rev/467f9665f4ba) I remove everything that devedition does except for adding / removing an observer for lightweight-theme-styling-update.  The test is reduced to doing subsequent calls to previewTheme and returning with an ok(true).  And the push is leaking just the same as the patch with the full devedition machinery.

With this in mind, I'm going to reland the patches.

Comment 296

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/e6e66d6d4a92
https://hg.mozilla.org/integration/fx-team/rev/6db8a34248f3

Comment 297

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6e66d6d4a92
https://hg.mozilla.org/mozilla-central/rev/6db8a34248f3

(TM tracks landing on trunk, status flags track branch landings)

Comment 298

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #236)
> Comment on attachment 8514457 [details] [diff] [review]
> lwtheme-preview-listener.patch
> 
> >   observe: function (subject, topic, data) {
> >+    if (topic == "lightweight-theme-styling-update") {
> >+      let newTheme = JSON.parse(data);
> >+      if (!newTheme || newTheme.id === this.defaultThemeID) {
> 
> You should never get this notification with the default theme ID. The
> default theme is not a lightweight theme. Please file a bug on this.

Filed Bug 1093368

Comment 299

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #295)
> And the most clear reduced test case that I can find to reproduce the leak:
> https://tbpl.mozilla.org/?tree=Try&rev=a93c88e77c75.  In this changeset
> (https://hg.mozilla.org/try/rev/467f9665f4ba) I remove everything that
> devedition does except for adding / removing an observer for
> lightweight-theme-styling-update.  The test is reduced to doing subsequent
> calls to previewTheme and returning with an ok(true).  And the push is
> leaking just the same as the patch with the full devedition machinery.
> 
> With this in mind, I'm going to reland the patches.

Filed Bug 1093372 about this leak

Comment 300

[Dão Gottwald [:dao]]

Comment on attachment 8515754 [details] [diff] [review]
lwtheme-preview-listener-r=Gijs.patch

>+  LightweightThemeManager.previewTheme(dummyLightweightTheme("{972ce4c6-7e08-4474-a285-3208198ce6fd}"));
>+  ok (DevEdition.styleSheet, "The devedition stylesheet is still enabled after the default theme is applied.");

What the heck is that? :/ dummyLightweightTheme("{972ce4c6-7e08-4474-a285-3208198ce6fd}") certainly isn't the default theme.

Comment 301

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf155bbfcdd7
https://hg.mozilla.org/releases/mozilla-aurora/rev/a468e06bd0bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c0675ad8c5a
https://hg.mozilla.org/releases/mozilla-aurora/rev/60fa1d966ca0
https://hg.mozilla.org/releases/mozilla-aurora/rev/f627be943750