Closed
Bug 1434476
Opened 7 years ago
Closed 7 years ago
Allow changing the background color of the selected tab
Categories
(WebExtensions :: Themes, enhancement, P5)
WebExtensions
Themes
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ntim, Assigned: u602518, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
It would be nice to be able to change the background color of the selected tab Ă -la old developer edition theme.
Updated•7 years ago
|
Mentor: jaws, ntim.bugs
Updated•7 years ago
|
Assignee: nobody → viniciuscosta0197
Status: NEW → ASSIGNED
Comment 1•7 years ago
|
||
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•7 years ago
|
Priority: -- → P5
(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)
Comment 3•7 years ago
|
||
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)
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)
Comment 5•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
Ok. I think most problems with indentation are result of a badly configured vim. Just updated the patch.
Updated•7 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment 12•7 years 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/#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•7 years 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?
Comment 14•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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.
Reporter | ||
Comment 19•7 years ago
|
||
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 | ||
Comment 20•7 years 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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years 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•7 years 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 26•7 years 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/#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•7 years 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");
```
Comment 28•7 years ago
|
||
let openTabGradient = window.getComputedStyle(openTab).getPropertyValue("background-image");
should be
let openTabGradient = window.getComputedStyle(openTabBackground).getPropertyValue("background-image");
Flags: needinfo?(jaws)
Reporter | ||
Comment 29•7 years 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•7 years 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•7 years ago
|
||
ok, no problems! I've made the changes and pushed for review again.
Reporter | ||
Comment 33•7 years ago
|
||
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 35•7 years 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/#review230804
Attachment #8949632 -
Flags: review?(jaws) → review+
Comment 36•7 years ago
|
||
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)
Reporter | ||
Comment 37•7 years ago
|
||
As I said in comment 33, you'll need to rebase your patch on top of the latest changes. :)
Assignee | ||
Comment 38•7 years ago
|
||
Hm, sorry. How should I do that? Just 'hg rebase' ?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 39•7 years ago
|
||
I think it's 'hg pull --rebase'. Jared probably knows better than me though.
Flags: needinfo?(ntim.bugs)
Comment 40•7 years 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
Comment 41•7 years ago
|
||
I took care of the rebase and pushed it to inbound. Nice work!
Flags: needinfo?(jaws)
Comment 42•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 43•7 years ago
|
||
(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•7 years 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•7 years 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)
Reporter | ||
Comment 47•7 years ago
|
||
The patch has already landed on Nightly, so there's nothing else for you to do here :)
Congratulations!
Flags: needinfo?(jaws)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•