Closed Bug 1426686 Opened 6 years ago Closed 6 years ago

Add support for theming the tab loading indicator

Categories

(WebExtensions :: Themes, enhancement, P5)

Product:
WebExtensions
Component:
Themes
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jaws, Assigned: lianzhen, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1426686 - Add support for theming the tab loading indicator.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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.
Priority: -- → P5
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 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!
Attachment #8944977 - Flags: review?(jaws) → review-
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 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 = ...;
Keywords: dev-doc-needed
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+
Blocks: 1435122
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
https://hg.mozilla.org/mozilla-central/rev/111e574ef42a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
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!
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-
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)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: