Spun off from bug 1347188. > * COLOR_TAB_THROBBER_SPINNING -> --tab-loading-fill > * COLOR_TAB_THROBBER_WAITING -> can be implemented "spinning" is better termed as "loading" Let's go with: #1 tab_loading - ThemeColor - Can use pre-existing --tab-loading-fill. Will set the color of the tab throbber while the page is loading, as well as set the color of the tab throbber "burst". #2 tab_waiting - ThemeColor - Will need to introduce new CSS custom property for this.

mconley, dao, and myself looked further at this bug and have limited the scope of it to: Changing only the tab_loading color. The tab_waiting color already uses the currentColor of the tab text and as such doesn't need a separate value to style it. It should be noted as a known issue that due to bug 1406414 the loading color will switch back to the default blue if the machine is under heavy load. We don't plan to support changing the color in this state.

Comment on attachment 8944977 [details] Bug 1426686 - Add support for theming the tab loading indicator. https://reviewboard.mozilla.org/r/215132/#review220786 Great work :) Here are some comments: ::: browser/themes/shared/tabs.inc.css:213 (Diff revision 2) > } > > #TabsToolbar[brighttext] .tab-throbber[progress]:not([selected=true])::before { > /* Don't change the --tab-loading-fill variable because this should only affect > tabs that are not visually selected. */ > - fill: #84c1ff; > + fill: #84c1ff nit: please undo this change :) ::: toolkit/components/extensions/schemas/theme.json:88 (Diff revision 2) > }, > "background_tab_text": { > "$ref": "ThemeColor", > "optional": true > }, > + "tab_loading_fill":{ :jaws said to use "tab_loading" (which I prefer as well) in comment 1. ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:24 (Diff revision 2) > + "accentcolor": '#000', > + "toolbar" : '#124455', We like using double quotes :) ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:39 (Diff revision 2) > + }); > + > + await extension.startup(); > + > + info("Checking selected tab loading indicator colors"); > + let tab_to_test = document.querySelector("#main-window"); We prefer using camelCase :) ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:40 (Diff revision 2) > + Assert.equal( > + window.getComputedStyle(tab_to_test).getPropertyValue("--tab-loading-fill"), > + TAB_LOADING_COLOR, > + "tab loading indicator color set" > + ); In addition to checking the variable value, Can you also check that the tab loading indicator actually getting filled ? Something among those lines: let selectedTab = document.querySelector(".tabbrowser-tab[visuallyselected=true]"); // Force loading throbber to appear selectedTab.setAttribute("busy", "true"); selectedTab.setAttribute("progress", "true"); let throbber = selectedTab.querySelector(".tab-throbber"); Assert.equal(window.getComputedStyle(throbber).fill, hexToRgb(TAB_LOADING_COLOR), "Throbber is filled with theme color"); selectedTab.removeAttribute("busy"); selectedTab.removeAttribute("progress");

Comment on attachment 8944977 [details] Bug 1426686 - Add support for theming the tab loading indicator. https://reviewboard.mozilla.org/r/215132/#review221130 ntim's review comments here are sufficient. Nice work!

Correction on comment 4: replace: let throbber = document.getAnonymousElementByAttribute(tab, "class", "tab-throbber"); with: let throbber = selectedTab.querySelector(".tab-throbber");

(In reply to Tim Nguyen :ntim from comment #6) > Correction on comment 4: > > replace: > > let throbber = document.getAnonymousElementByAttribute(tab, "class", > "tab-throbber"); > > with: > > let throbber = selectedTab.querySelector(".tab-throbber"); meant the other way round obviously :)

Comment on attachment 8944977 [details] Bug 1426686 - Add support for theming the tab loading indicator. https://reviewboard.mozilla.org/r/215132/#review222614 ::: browser/themes/shared/tabs.inc.css:213 (Diff revision 3) > } > > #TabsToolbar[brighttext] .tab-throbber[progress]:not([selected=true])::before { > /* Don't change the --tab-loading-fill variable because this should only affect > tabs that are not visually selected. */ > - fill: #84c1ff; > + fill: #84c1ff Please restore the `;` that was removed. ::: toolkit/components/extensions/ext-theme.js:144 (Diff revision 3) > case "toolbar_text": > case "bookmark_text": > this.lwtStyles.toolbar_text = cssColor; > break; > + case "tab_loading": > + this.lwtStyles.loadingFill = cssColor; Can you just use `tab_loading` instead of `loadingFill` here and in LightweightThemeConsumer.jsm ? ::: toolkit/components/extensions/schemas/theme.json:88 (Diff revision 3) > }, > "background_tab_text": { > "$ref": "ThemeColor", > "optional": true > }, > + "tab_loading":{ Please add a space after `:` ::: toolkit/components/extensions/test/browser/browser.ini:12 (Diff revision 3) > [browser_ext_themes_chromeparity.js] > [browser_ext_themes_dynamic_getCurrent.js] > [browser_ext_themes_dynamic_onUpdated.js] > [browser_ext_themes_dynamic_updates.js] > [browser_ext_themes_getCurrent_differentExt.js] > +[browser_ext_themes_loading_filling.js] Can you rename the test to "browser_ext_themes_tab_loading.js" ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:3 (Diff revision 3) > +// This test checks whether applied theme that attempt to change the loading > +// indicator filling of tab are applied properly > +// Will add visible testing instead of compare hex value This comment can probably removed :) The test is already explicit enough by itself ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:7 (Diff revision 3) > +add_task(async function setup() { > + await SpecialPowers.pushPrefEnv({ > + set: [["extensions.webextensions.themes.enabled", true]], > + }); > +}); You don't need the setup part. ::: toolkit/components/extensions/test/browser/browser_ext_themes_loading_filling.js:40 (Diff revision 3) > + let selectedTab = document.querySelector(".tabbrowser-tab[visuallyselected=true]"); > + > + selectedTab.setAttribute("busy", "true"); > + selectedTab.setAttribute("progress", "true"); > + let throbber = document.getAnonymousElementByAttribute(selectedTab, "class", "tab-throbber"); Little detail about line spacing: let selectedTab = ...; selectedTab.setAttribute(...); selectedTab.setAttribute(...); let throbber = ...;

Comment on attachment 8944977 [details] Bug 1426686 - Add support for theming the tab loading indicator. https://reviewboard.mozilla.org/r/215132/#review222988 Looks good! Please make the below changes and re-upload and we can land this. ::: toolkit/components/extensions/ext-theme.js:143 (Diff revision 4) > + case "tab_loading": > + this.lwtStyles.tab_loading = cssColor; > + break; > case "tab_text": nit, this can be combined with the below block. ::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_loading.js:37 (Diff revision 4) > + > + selectedTab.setAttribute("busy", "true"); > + selectedTab.setAttribute("progress", "true"); > + > + let throbber = document.getAnonymousElementByAttribute(selectedTab, "class", "tab-throbber"); > + Assert.equal(window.getComputedStyle(throbber, ":before").fill, `rgb(${hexToRGB(TAB_LOADING_COLOR).join(", ")})`, nit, this should technically be "::before" instead of ":before". ":before" only works for backwards-compat. From https://developer.mozilla.org/en-US/docs/Web/CSS/::before, "CSS3 introduced the ::before notation (with two colons) to distinguish pseudo-classes from pseudo-elements. Browsers also accept :before, introduced in CSS2." ::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_loading.js:42 (Diff revision 4) > + > + > + await extension.unload(); > + nit, please remove these blank lines.

Comment on attachment 8944977 [details] Bug 1426686 - Add support for theming the tab loading indicator. https://reviewboard.mozilla.org/r/215132/#review223348 ::: toolkit/components/extensions/ext-theme.js:143 (Diff revision 4) > + case "tab_loading": > + this.lwtStyles.tab_loading = cssColor; > + break; > case "tab_text": Which can be combined?

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s e51c5ac9c85890c58d2a7cbf9256acc6a5ff3777 -d 90f493006a7d: rebasing 445569:e51c5ac9c858 "Bug 1426686 - Add support for theming the tab loading indicator. r=jaws" (tip) merging toolkit/components/extensions/ext-theme.js merging toolkit/components/extensions/schemas/theme.json merging toolkit/components/extensions/test/browser/browser.ini merging toolkit/modules/LightweightThemeConsumer.jsm warning: conflicts while merging toolkit/components/extensions/ext-theme.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/111e574ef42a Add support for theming the tab loading indicator. r=jaws

Woohoo! Thanks, Zhengyi. This has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#February_2018 Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

This landed with an automated test, so I think we're good here. Thanks!

I think the docs for this are updated, but please let me know if we need anything else: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme

Same as the other bug