Allow changing the background color of the selected tab

RESOLVED FIXED in Firefox 60

Status

P5
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: ntim, Assigned: viniciuscosta0197, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

It would be nice to be able to change the background color of the selected tab à-la old developer edition theme.
Assignee: nobody → viniciuscosta0197
Status: NEW → ASSIGNED
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.

Updated

10 months ago
Priority: -- → P5
(Assignee)

Comment 2

10 months ago
(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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
(Assignee)

Comment 4

10 months ago
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.
Flags: needinfo?(jaws)
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/`
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Comment 7

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 9

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 11

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

Updated

10 months ago
Component: WebExtensions: Frontend → WebExtensions: Themes
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".
Attachment #8949632 - Flags: review?(jaws) → review-
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
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?
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

10 months ago
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.
(Reporter)

Comment 17

10 months ago
mozreview-review
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
(Assignee)

Comment 18

10 months ago
mozreview-review-reply
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.
(Assignee)

Updated

10 months ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
If you go to Customize, then select an image theme like "Pastel Gradients" in the Themes menu, you should see where those are used.
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
(Assignee)

Comment 20

10 months ago
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;
>}
Flags: needinfo?(ntim.bugs)
Looks good to me!
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

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

Ok! Just added the changes and pushed for review again
(Reporter)

Comment 24

10 months ago
mozreview-review
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 hidden (mozreview-request)
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?
Attachment #8949632 - Flags: review?(jaws) → review-
(Assignee)

Comment 27

10 months ago
mozreview-review-reply
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");
```
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
  let openTabGradient = window.getComputedStyle(openTab).getPropertyValue("background-image");

should be

  let openTabGradient = window.getComputedStyle(openTabBackground).getPropertyValue("background-image");
Flags: needinfo?(jaws)
(Reporter)

Comment 29

10 months ago
mozreview-review
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) :(
(Reporter)

Comment 30

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 32

10 months ago
ok, no problems! I've made the changes and pushed for review again.
Thanks for updating! The patch looks good, you might want to rebase it on top of the latest source code however.
Comment hidden (mozreview-request)
Comment on attachment 8949632 [details]
Bug 1434476 - Allow changing the background color of the selected tab.

https://reviewboard.mozilla.org/r/218986/#review230804
Attachment #8949632 - Flags: review?(jaws) → review+
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.
Flags: needinfo?(viniciuscosta0197)
(Assignee)

Updated

10 months ago
Flags: needinfo?(viniciuscosta0197)
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
As I said in comment 33, you'll need to rebase your patch on top of the latest changes. :)
(Assignee)

Comment 38

10 months ago
Hm, sorry. How should I do that? Just 'hg rebase' ?
Flags: needinfo?(ntim.bugs)
I think it's 'hg pull --rebase'. Jared probably knows better than me though.
Flags: needinfo?(ntim.bugs)

Comment 40

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f5b5e79d2e
Allow changing the background color of the selected tab. r=jaws
I took care of the rebase and pushed it to inbound. Nice work!
Flags: needinfo?(jaws)

Comment 42

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65f5b5e79d2e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(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

9 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 46

9 months ago
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 :)
Flags: needinfo?(jaws)
The patch has already landed on Nightly, so there's nothing else for you to do here :)

Congratulations!
Flags: needinfo?(jaws)
Depends on: 1452019

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.