1434476 - Allow changing the background color of the selected tab

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement, P5)

Description

[Tim Nguyen :ntim]

It would be nice to be able to change the background color of the selected tab à-la old developer edition theme.

Comment 1

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

To fix this bug, you will need to edit the following files:

toolkit/components/extensions/schemas/theme.json
  This file defines the schema for the theming API. You will need to add a new item to the "colors" property. You can use the name "selected_tab_background".

toolkit/components/extensions/ext-theme.js:
  This is the file that parses and loads the theme file from the manifest. In the loadColors function you will need to add another case statement for "selected_tab_background". This can go in the final block that assigns the cssColor to the property of the same name on lwtStyles.

toolkit/modules/LightweightThemeConsumer.jsm:
  This file reads the values that ext-theme.js loaded and applies it to CSS variables. You will need to add a line to the kCSSVarsMap at the top of the file to map the "selected_tab_background" string to a CSS custom property. You can use "--lwt-selected-tab-background-color".

browser/themes/shared/tabs.inc.css:
  This is where the CSS custom property will actually get used. At line 531 [1] you will need to change the background-image to add an extra gradient to the top that uses the --lwt-selected-tab-background-color if it exists and transparent if it doesn't. This would end up looking like:
> 
> background-image: linear-gradient(var(--lwt-selected-tab-background-color, transparent), var(--lwt-selected-tab-background-color, transparent)),
>                   linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
>                   var(--lwt-header-image);
> 
> 

[1] https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/browser/themes/shared/tabs.inc.css#529

Now it should all work (assuming I didn't miss a step). But every time that we add a new property we want to add an automated test for it. I will leave this step for you to figure out, but you can look at the tests in toolkit/components/extensions/test/browser/browser_ext_themes_tab_text.js to get an idea of what to do.

If you have any questions please write them here and either myself or :ntim can answer them. Please also use the "needinfo" flag at the bottom of this page to flag us for your questions so you make sure that we see them.

Comment 2

[u602518]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> To fix this bug, you will need to edit the following files:
> 
> toolkit/components/extensions/schemas/theme.json
>   This file defines the schema for the theming API. You will need to add a
> new item to the "colors" property. You can use the name
> "selected_tab_background".
> 
> toolkit/components/extensions/ext-theme.js:
>   This is the file that parses and loads the theme file from the manifest.
> In the loadColors function you will need to add another case statement for
> "selected_tab_background". This can go in the final block that assigns the
> cssColor to the property of the same name on lwtStyles.
> 
> toolkit/modules/LightweightThemeConsumer.jsm:
>   This file reads the values that ext-theme.js loaded and applies it to CSS
> variables. You will need to add a line to the kCSSVarsMap at the top of the
> file to map the "selected_tab_background" string to a CSS custom property.
> You can use "--lwt-selected-tab-background-color".
> 
> browser/themes/shared/tabs.inc.css:
>   This is where the CSS custom property will actually get used. At line 531
> [1] you will need to change the background-image to add an extra gradient to
> the top that uses the --lwt-selected-tab-background-color if it exists and
> transparent if it doesn't. This would end up looking like:
> > 
> > background-image: linear-gradient(var(--lwt-selected-tab-background-color, transparent), var(--lwt-selected-tab-background-color, transparent)),
> >                   linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
> >                   var(--lwt-header-image);
> > 
> > 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> eeb7190f9ad6f1a846cd6df09986325b3f2c3117/browser/themes/shared/tabs.inc.
> css#529
> 
> Now it should all work (assuming I didn't miss a step). But every time that
> we add a new property we want to add an automated test for it. I will leave
> this step for you to figure out, but you can look at the tests in
> toolkit/components/extensions/test/browser/browser_ext_themes_tab_text.js to
> get an idea of what to do.
> 
> If you have any questions please write them here and either myself or :ntim
> can answer them. Please also use the "needinfo" flag at the bottom of this
> page to flag us for your questions so you make sure that we see them.

So lwtStyles is an object that stores CSS color values? 

In this case, if case "selected_tab_background" is true, I will assign a new property to lwtStyles with its value being the cssColor, is that right?

Comment 3

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

Yes, lwtStyles is an object that stores CSS color values as well as other values that are used by themes, such as background-images.

Yes, a new property will be added to lwtStyles.

Comment 4

[u602518]

Ok. I'm writing the automated test now. Like you said, I tried to figure things out by looking at toolkit/components/extensions/test/browser/browser_ext_themes_tab_text.js. I've created a new file and added a task (add_task) to do this:


>    Assert.equal(window.getComputedStyle(selectedTab).getPropertyValue("background-color"),
>              "rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
>              "Selected tab background color should be set.");

My question is: how do I run this test to check if it's working properly? I've read the guide at https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests but I would like to run it only for this test in particular.

Comment 5

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

To run only a single test, for example the browser_ext_themes_tab_text.js test, you would run the following command:

`./mach test toolkit/components/extensions/test/browser/browser_ext_themes_tab_text.js`

Once you get that test passing, you should run all of the tests in the directory to make sure that one of the existing tests didn't break with your changes. To do that you can run:

`./mach test  toolkit/components/extensions/test/browser/`

Comment 6

[u602518]

Attachment: Bug 1434476 - Allow changing the background color of the selected tab.

***

Review commit: https://reviewboard.mozilla.org/r/218986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218986/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review224700


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/ext-theme.js:166
(Diff revision 1)
>          case "button_background":
>          case "button_background_hover":
>          case "button_background_active":
>            this.lwtStyles[color] = cssColor;
>            break;
> +		case "selected_tab_background":

Error: Expected indentation of 8 spaces but found 2 tabs. [eslint: indent]

::: toolkit/components/extensions/ext-theme.js:166
(Diff revision 1)
>          case "button_background":
>          case "button_background_hover":
>          case "button_background_active":
>            this.lwtStyles[color] = cssColor;
>            break;
> +		case "selected_tab_background":

Error: Unexpected tab character. [eslint: no-tabs]

::: toolkit/components/extensions/ext-theme.js:167
(Diff revision 1)
>          case "button_background_hover":
>          case "button_background_active":
>            this.lwtStyles[color] = cssColor;
>            break;
> +		case "selected_tab_background":
> +		  this.lwtStyles.selected_tab_background = cssColor;

Error: Unexpected tab character. [eslint: no-tabs]

::: toolkit/components/extensions/ext-theme.js:168
(Diff revision 1)
>          case "button_background_active":
>            this.lwtStyles[color] = cssColor;
>            break;
> +		case "selected_tab_background":
> +		  this.lwtStyles.selected_tab_background = cssColor;
> +		  break;

Error: Unexpected tab character. [eslint: no-tabs]

Comment 8

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/1-2/

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review224788

::: toolkit/components/extensions/ext-theme.js:128
(Diff revision 2)
>        if (Array.isArray(val)) {
>          cssColor = "rgb" + (val.length > 3 ? "a" : "") + "(" + val.join(",") + ")";
>        }
>  
>        switch (color) {
> +        case "selected_tab_background":

We agreed with :jaws to use "selected_tab" instead of "selected_tab_background"

::: toolkit/components/extensions/schemas/theme.json:74
(Diff revision 2)
>              "type": "object",
>              "optional": true,
>              "properties": {
> +			  "selected_tab_background": {
> +				"$ref": "ThemeColor",
> +				"optional": true

nit: fix indentation.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:7
(Diff revision 2)
> +
> +// This test checks whether applied WebExtension themes that attempt to change
> +// the background color of selected tab are applied correctly.
> +
> +add_task(async function test_tab_background_color_property () {
> +	const TAB_BACKGROUND_COLOR = '#9400ff';

nit: fix indentation through out this test (should be 2 spaces)

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:34
(Diff revision 2)
> +	let selectedTab = document.querySelector(".tabbrowser-tab[selected]");
> +
> +	Assert.equal("rgb("+ hexToRGB(window.getComputedStyle(selectedTab).getPropertyValue("--lwt-selected-tab-background-color")).join(", ") + ")",
> +			"rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
> +			"Selected tab background color should be set.");
> +	

nit: trailing whitespace

Comment 10

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/2-3/

Comment 11

[u602518]

Ok. I think most problems with indentation are result of a badly configured vim. Just updated the patch.

Comment 12

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

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review225404

Thanks! This is close :)

::: browser/themes/shared/tabs.inc.css:531
(Diff revision 3)
>  /* Lightweight theme on tabs */
>  #tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background[selected=true]:-moz-lwtheme {
>    background-attachment: scroll, fixed;
>    background-color: transparent;
> -  background-image: linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
> +  background-image: linear-gradient(var(--lwt-selected-tab-background-color, transparent), var(--lwt-selected-tab-background-color, transparent)),
> +  					linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),

nit, looks like some tab characters are still here.

::: toolkit/components/extensions/ext-theme.js:128
(Diff revision 3)
> +        case "selected_tab":
> +          this.lwtStyles.selected_tab_color = cssColor;

We can use this.lwtStyles.selected_tab instead of this.lwtStyles.selected_tab_color. Since we'll use the same property name as the color name, then you can just add the color name to the list at the bottom of the switch statement.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:20
(Diff revision 3)
> +          "accentcolor": ACCENT_COLOR,
> +          "textcolor": TEXT_COLOR,
> +          "selected_tab": TAB_BACKGROUND_COLOR,
> +        },
> +      },
> +  	},

nit, some tab characters here that should be switched to spaces.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:31
(Diff revision 3)
> +  await extension.startup();
> +
> +  info("Checking selected tab color");
> +  let selectedTab = document.querySelector(".tabbrowser-tab[selected]");
> +
> +  Assert.equal("rgb("+ hexToRGB(window.getComputedStyle(selectedTab).getPropertyValue("--lwt-selected-tab-background-color")).join(", ") + ")",

This should be getting the backgroundImage property instead of the variable value, since we want to make sure that the backgroundImage is actually being controlled by the theme.

::: toolkit/modules/LightweightThemeConsumer.jsm:36
(Diff revision 3)
>    ["--lwt-toolbarbutton-icon-fill", "icon_color"],
>    ["--lwt-toolbarbutton-icon-fill-attention", "icon_attention_color"],
>    ["--lwt-toolbarbutton-background", "button_background"],
>    ["--lwt-toolbarbutton-hover-background", "button_background_hover"],
>    ["--lwt-toolbarbutton-active-background", "button_background_active"],
> +  ["--lwt-selected-tab-background-color", "selected_tab_color"]

This should be "selected_tab".

Comment 13

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review225404

> We can use this.lwtStyles.selected_tab instead of this.lwtStyles.selected_tab_color. Since we'll use the same property name as the color name, then you can just add the color name to the list at the bottom of the switch statement.

Ok, so I will change to this.lwtStyles[color] = cssColor, in a similar manner to the last case in the switch statement.

> This should be getting the backgroundImage property instead of the variable value, since we want to make sure that the backgroundImage is actually being controlled by the theme.

I'm using the debugger and backgroundImage is set to 'None', even though things are apparently working (the tab color is changing). I've used window.getComputedStyle(selectedTab).getPropertyValue("backgroundImage")

Where should I look to fix that?

Comment 14

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

You will need to get the styles from an anonymous element inside of the tab to find what the background-image is set to. You can look at https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/extensions/test/browser/browser_ext_themes_tab_loading.js#36 for an example of what a different test had to do.

In your case the anonymous element has a className of "tab-background". See https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/base/content/tabbrowser.xml#7788-7789 to see what the markup looks like.

Comment 15

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/3-4/

Comment 16

[u602518]

Ok. Since the return value of window.getComputedStyle(tabBackground, null).getPropertyValue("background-image") is a long string, I've used a regular expression to extract the RGB information from it. The match() function returns two RGB values, but since they are supposed to be equal I'm using only the first of them in the test.

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review226374

::: browser/themes/shared/tabs.inc.css:528
(Diff revision 4)
>    background-attachment: scroll, fixed;
>    background-color: transparent;
> -  background-image: linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
> +  background-image: linear-gradient(var(--lwt-selected-tab-background-color, transparent), var(--lwt-selected-tab-background-color, transparent)),
> +                    linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
>                      var(--lwt-header-image);
>    background-position: 0 0, right top;
>    background-repeat: repeat-x, no-repeat;
>    background-size: auto 100%, auto auto;

don't you need to add a extra field for the background-attachment/repeat/size/position property ?

::: toolkit/components/extensions/ext-theme.js:128
(Diff revision 4)
>        if (Array.isArray(val)) {
>          cssColor = "rgb" + (val.length > 3 ? "a" : "") + "(" + val.join(",") + ")";
>        }
>  
>        switch (color) {
> +        case "selected_tab":

combine this with the other tab_loading/tab_text/... case below.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:7
(Diff revision 4)
> +
> +// This test checks whether applied WebExtension themes that attempt to change
> +// the background color of selected tab are applied correctly.
> +
> +add_task(async function test_tab_background_color_property() {
> +  const TAB_BACKGROUND_COLOR = '#9400ff';

nit: double quotes

Comment 18

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review226374

> don't you need to add a extra field for the background-attachment/repeat/size/position property ?

Is there a way to easily examine the CSS in this case to see how changes in the code affect the tab style? I've added an extra field for the mentioned properties, but I've just duplicated the properties that were set for linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)), so I've put scroll/x-repeat/auto100%/00 respectively. Using --jsdebugger I was unable to see any difference.

Comment 19

[Tim Nguyen :ntim]

If you go to Customize, then select an image theme like "Pastel Gradients" in the Themes menu, you should see where those are used.

Comment 20

[u602518]

Ok. I'm using browser toolbox to test the CSS in the selected tab. If I understand correctly, the selected tab will have the color defined in the theme manifest. If it doesn't have a color defined, it will be set to transparent and the selected tab will have the color defined by '--toolbar-bgcolor' instead.

So, does it make sense to define those properties like this?

>#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background[selected=true]:-moz-lwtheme {
>  background-attachment: scroll, scroll, fixed;
>  background-color: transparent;
>  background-image: linear-gradient(var(--lwt-selected-tab-background-color, transparent), var(--lwt-selected-tab-background->color, transparent)),
>                    linear-gradient(var(--toolbar-bgcolor), var(--toolbar-bgcolor)),
>                    var(--lwt-header-image);
>  background-position: 0 0, 0 0, right top;
>  background-repeat: repeat-x, repeat-x, no-repeat;
>  background-size: auto 100%, auto 100%, auto auto;
>}

Comment 21

[Tim Nguyen :ntim]

Looks good to me!

Comment 22

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/4-5/

Comment 23

[u602518]

(In reply to Tim Nguyen :ntim from comment #21)
> Looks good to me!

Ok! Just added the changes and pushed for review again

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review227786

::: toolkit/components/extensions/ext-theme.js:155
(Diff revision 5)
> +          this.lwtStyles[color] = cssColor;
> +          break;

nit: remove these 2 lines, since they're already below in the case statement

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:37
(Diff revision 5)
> +
> +  let gradient = window.getComputedStyle(tabBackground, null).getPropertyValue("background-image");
> +  let rgbRegex = /rgb\((\d{1,3}), (\d{1,3}), (\d{1,3})\)/g;
> +  let colors = gradient.match(rgbRegex);
> +
> +  debugger;

nit: this seems like a leftover

Comment 25

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/5-6/

Comment 26

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

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review229630

This looks really good! I'm very sorry for not doing the review sooner. The last week was not normal for me with traveling but I am back home now and promise to be faster on my reviews.

With the two changes below I think this will be ready to check in to mozilla-central :)

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:33
(Diff revision 6)
> +  info("Checking selected tab color");
> +
> +  let selectedTab = document.querySelector(".tabbrowser-tab[visuallyselected=true]");
> +  let tabBackground = document.getAnonymousElementByAttribute(selectedTab, "class", "tab-background");
> +
> +  let gradient = window.getComputedStyle(tabBackground, null).getPropertyValue("background-image");

You can remove the `, null` since that second argument is optional.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tabbackground_color.js:37
(Diff revision 6)
> +  Assert.equal(colors[0], "rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
> +         "Selected tab background color should be set.");

Can you also open a second tab and make sure that the selected tab background color is _not_ applied to the background tab?

Comment 27

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review229630

> Can you also open a second tab and make sure that the selected tab background color is _not_ applied to the background tab?

Hm... I've restructured this part of the code. I'm opening a new tab with BrowserTestUtils.openNewForegroundTab and this new tab gets the set background color. The assert.equal part is the same as before (just changed the variable names). Also, I check if 'background-image' is 'none' for the non selected tab, as to make sure that the selected tab background color is not applied.

```javascript
  let openTab = document.querySelector(".tabbrowser-tab[visuallyselected=true]");
  let openTabBackground = document.getAnonymousElementByAttribute(openTab, "class", "tab-background");
  
  let selectedTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
  let selectedTabBackground = document.getAnonymousElementByAttribute(selectedTab, "class", "tab-background");

  let openTabGradient = window.getComputedStyle(openTab).getPropertyValue("background-image");
  let selectedTabGradient = window.getComputedStyle(selectedTabBackground).getPropertyValue("background-image");

  let rgbRegex = /rgb\((\d{1,3}), (\d{1,3}), (\d{1,3})\)/g;
  let selectedTabColors = selectedTabGradient.match(rgbRegex);

  Assert.equal(selectedTabColors[0], "rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
         "Selected tab background color should be set.");
  Assert.equal(openTabGradient, "none");
```

Comment 28

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

  let openTabGradient = window.getComputedStyle(openTab).getPropertyValue("background-image");

should be

  let openTabGradient = window.getComputedStyle(openTabBackground).getPropertyValue("background-image");

Comment 29

[Tim Nguyen :ntim]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review230486

::: toolkit/components/extensions/ext-theme.js:154
(Diff revision 6)
>          case "icons_attention":
>            this.lwtStyles.icon_attention_color = cssColor;
>            break;
>          case "tab_loading":
>          case "tab_text":
> +        case "selected_tab":

We're renaming background_tab_text to tab_background_text.

For consistency, could you rename selected_tab to tab_selected ? (sorry for making you rename so much) :(

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review230488

::: toolkit/components/extensions/test/browser/browser.ini:23
(Diff revision 6)
>  [browser_ext_themes_tab_text.js]
>  [browser_ext_themes_toolbar_fields.js]
>  [browser_ext_themes_toolbars.js]
>  [browser_ext_themes_toolbarbutton_icons.js]
>  [browser_ext_themes_toolbarbutton_colors.js]
> +[browser_ext_themes_tabbackground_color.js]

could you rename the test to browser_ext_themes_tab_selected.js ?

Comment 31

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/6-7/

Comment 32

[u602518]

ok, no problems! I've made the changes and pushed for review again.

Comment 33

[Tim Nguyen :ntim]

Thanks for updating! The patch looks good, you might want to rebase it on top of the latest source code however.

Comment 34

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/7-8/

Comment 35

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

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review230804

Comment 36

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

Nice job! Can you click on the MozReview link, and go to each of the issues that have been logged and click on the "Fixed" button for each one?

Once you do that, please flag me for needinfo and I'll land your patch.

Comment 37

[Tim Nguyen :ntim]

As I said in comment 33, you'll need to rebase your patch on top of the latest changes. :)

Comment 38

[u602518]

Hm, sorry. How should I do that? Just 'hg rebase' ?

Comment 39

[Tim Nguyen :ntim]

I think it's 'hg pull --rebase'. Jared probably knows better than me though.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f5b5e79d2e
Allow changing the background color of the selected tab. r=jaws

Comment 41

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

I took care of the rebase and pushed it to inbound. Nice work!

Comment 42

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/65f5b5e79d2e

Comment 43

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

(In reply to Tim Nguyen :ntim from comment #39)
> I think it's 'hg pull --rebase'. Jared probably knows better than me though.

Yes, `hg pull --rebase` should have worked. But the move to ThemeVariablesMap.jsm wouldn't have been fixed automatically by the `--rebase` option.

Comment 44

[Code Review Bot [:reviewbot]]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review230908


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_selected.js:43
(Diff revision 8)
> +
> +  let rgbRegex = /rgb\((\d{1,3}), (\d{1,3}), (\d{1,3})\)/g;
> +  let selectedTabColors = selectedTabGradient.match(rgbRegex);
> +
> +  Assert.equal(selectedTabColors[0], "rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
> +         "Selected tab background color should be set.");

Error: Expected indentation of 15 spaces but found 9. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_selected.js:46
(Diff revision 8)
> +
> +  Assert.equal(selectedTabColors[0], "rgb(" + hexToRGB(TAB_BACKGROUND_COLOR).join(", ") + ")",
> +         "Selected tab background color should be set.");
> +  Assert.equal(openTabGradient, "none");
> +
> +  gBrowser.removeTab(selectedTab)

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_selected.js:48
(Diff revision 8)
> +         "Selected tab background color should be set.");
> +  Assert.equal(openTabGradient, "none");
> +
> +  gBrowser.removeTab(selectedTab)
> +  await extension.unload();
> +})

Error: Missing semicolon. [eslint: semi]

Comment 45

[u602518]

Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218986/diff/8-9/

Comment 46

[u602518]

I've just fixed the errors Code Review Bot complained about and pushed again for review. Not sure if it was really needed, but here it is :)

Comment 47

[Tim Nguyen :ntim]

The patch has already landed on Nightly, so there's nothing else for you to do here :)

Congratulations!