Add support for theming the tab loading indicator

RESOLVED FIXED in Firefox 60

Status

enhancement
P5
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jaws, Assigned: lianzhen, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla60
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

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.

Updated

a year ago
Priority: -- → P5

Updated

a year ago
No longer blocks: 1347188
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.
Summary: Add support for theming the tab loading/waiting indicator → Add support for theming the tab loading indicator
Assignee: nobody → lianzhen
Status: NEW → ASSIGNED
Mentor: jaws, mconley
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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");
(Reporter)

Comment 5

a year ago
mozreview-review
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!
Attachment #8944977 - Flags: review?(jaws) → review-

Updated

a year ago
Blocks: themingapi-more-ui
No longer blocks: themingapi-chrome
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 hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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 = ...;

Updated

a year ago
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
(Reporter)

Comment 11

a year ago
mozreview-review
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.
Attachment #8944977 - Flags: review?(jaws) → review+

Updated

a year ago
Blocks: 1435122
(Assignee)

Comment 12

a year ago
mozreview-review
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?
Comment hidden (mozreview-request)
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)
Comment hidden (mozreview-request)

Comment 16

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

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/111e574ef42a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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!

Comment 19

a year ago
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.
Flags: needinfo?(lianzhen)
This landed with an automated test, so I think we're good here. Thanks!
Flags: needinfo?(lianzhen) → qe-verify-

Updated

a year ago
Component: WebExtensions: Frontend → WebExtensions: Themes
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
Flags: needinfo?(ntim.bugs)
Same as the other bug
Flags: needinfo?(ntim.bugs)

Updated

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