Closed Bug 1423757 Opened 6 years ago Closed 5 years ago

Allow theming toolbar fields focused state

Categories

(WebExtensions :: Themes, enhancement, P5)

Product:
WebExtensions
Component:
Themes
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ntim, Assigned: lianzhen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1423757 - Allow theming toolbar fields focused state
59 bytes, text/x-review-board-request
jaws
: review+
ntim
: review+
Details 
It would be nice to be able to theme the hover/focused state of the toolbar fields: notably the background/border/text/box-shadow
Severity: normal → enhancement
Priority: -- → P5
Component: WebExtensions: Frontend → WebExtensions: Themes
Assignee: nobody → lianzhen
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review231758

::: browser/modules/ThemeVariableMap.jsm:29
(Diff revision 1)
>    ["--lwt-toolbarbutton-icon-fill-attention", "icon_attention_color"],
>    ["--lwt-toolbarbutton-hover-background", "button_background_hover"],
>    ["--lwt-toolbarbutton-active-background", "button_background_active"],
>    ["--lwt-selected-tab-background-color", "tab_selected"],
> +  ["--lwt-toolbar-field-hover", "toolbar_field_hover_background"],
> +  ["--lwt-toolbar-field-hover-text", "toolbar_field_hover_text_color"],

Please use --lwt-toolbar-field-hover-color instead of *-text since it is clearer that we expect a `color` value here.

::: toolkit/components/extensions/schemas/theme.json:188
(Diff revision 1)
> +              },
> +              "toolbar_field_hover_border": {
> +                "$ref": "ThemeColor",
> +                "optional": true
>                }
> +

Please remove this blank line.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:6
(Diff revision 1)
> +  const TOOLBAR_BACKGROUND = "#FFFFFF";
> +  const TOOLBAR_TEXT = "#000000";

These can be removed, we don't need to test what the initial colors are.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:10
(Diff revision 1)
> +add_task(async function test_toolbar_field_hover() {
> +  const TOOLBAR_BACKGROUND = "#FFFFFF";
> +  const TOOLBAR_TEXT = "#000000";
> +  const TOOLBAR_HOVER_BACKGROUND = "#FF0000";
> +  const TOOLBAR_HOVER_TEXT = "#9400FF";
> +  const TOOLBAR_HOVER_BORDER = "#FFFFFF";

You should have a test for TOOLBAR_HOVER_BORDER

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:15
(Diff revision 1)
> +        "images": {
> +          "headerURL": "image1.png",
> +        },

Images are not required for themes anymore (bug 1404688) so this can be removed as well as the `files` section below.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:19
(Diff revision 1)
> +      "theme": {
> +        "images": {
> +          "headerURL": "image1.png",
> +        },
> +        "colors": {
> +          "accentcolor": "#000000",

Your test was failing becuase you didn't specify a `textcolor` here. Therefore the theme wasn't valid and wasn't being applied.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:20
(Diff revision 1)
> +          "toolbar": TOOLBAR_BACKGROUND,
> +          "toolbar_text": TOOLBAR_TEXT,

These two should be removed.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:34
(Diff revision 1)
> +  // Remove the `remotecontrol` attribute since it interferes with the urlbar styling.
> +  document.documentElement.removeAttribute("remotecontrol");

I see that this part was copied from another test, so it's not your fault, but we should be using registerCleanupFunction after we remove this attribute to guarantee that it gets re-added back in case an exception gets thrown before line 63 is reached.

See https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/browser/components/extensions/test/browser/browser_ext_tabs_create.js#14 for example usage.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:39
(Diff revision 1)
> +  let urlBarQuery = document.querySelector("#urlbar");
> +  let urlBar = document.getElementById("urlbar");

These two variables will be referencing the same element.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:41
(Diff revision 1)
> +
> +  info("Checking toolbar field's hover color");
> +
> +  let urlBarQuery = document.querySelector("#urlbar");
> +  let urlBar = document.getElementById("urlbar");
> +  Assert.equal(window.getComputedStyle(urlBar, ":hover").backgroundColor,

The second argument to getComputedStyle is for pseudo-elements, such as ::before and ::after. Using :hover here has no effect, and can be removed.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:41
(Diff revision 1)
> +  Assert.equal(window.getComputedStyle(urlBar, ":hover").backgroundColor,
> +               `rgb(${hexToRGB(TOOLBAR_BACKGROUND).join(", ")})`,
> +               "Initial background color is not changed");
> +  Assert.equal(window.getComputedStyle(urlBar).color,

You can keep these assertions, but change them to Assert.notEqual and compare against their respective hover colors.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:48
(Diff revision 1)
> +               "Initial background color is not changed");
> +  Assert.equal(window.getComputedStyle(urlBar).color,
> +               `rgb(${hexToRGB(TOOLBAR_TEXT).join(", ")})`,
> +               "Initial text color is not changed");
> +
> +  urlBarQuery.setAttribute("focused", "true");

You don't need to set the focused="true" attribute. The background color is set when either focused="true" attribute is present or the :hover pseudo-class is applied.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:59
(Diff revision 1)
> +               "Hover Background Color is changed");
> +  Assert.equal(window.getComputedStyle(urlBar).color,
> +               `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`,
> +               "Hover Text Color is changed");
> +
> +               

nit, this extra line with trailing whitespace should be deleted.
Attachment #8956736 - Flags: review?(jaws) → review+
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review231768

Sorry, meant to mark r-
Attachment #8956736 - Flags: review+ → review-
Can we just allow theming the focus state for now ? It is the use-case theme developers are requesting, and is more straightforward to implement than the hover state.

colors.toolbar_field_focus
colors.toolbar_field_text_focus
colors.toolbar_field_border_focus

and potentially:

properties.toolbar_field_shadow_focus
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review232414

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:37
(Diff revision 3)
> +  Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor,
> +               `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,
> +               "Hover Background Color is not equal to initial");

Sorry, I may not have been clear enough on my previous review.

Now that you've got these changes made, it doesn't make much sense to have an assertion for "not equal" followed by an assertion for "equal". Since the two are mutually exclusive, the "equal" case will already cover the "not equal" case.

You should move "not equal" assertions to above the call to addPseudoClassLock, and change them from "not equal" to "equal" since the urlBar *should* have those styles before :hover is applied.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:52
(Diff revision 3)
> +  // Restore the `remotecontrol` attribute at the end of the test.
> +  registerCleanupFunction(() => {
> +    SpecialPowers.clearUserPref("remotecontrol");
> +  });

This should be moved up to where the attribute is removed, and secondly, this is now incorrect in calling clearUserPref. Your previous code added the attribute back, this is now clearing some preference that doesn't exist.
Attachment #8956736 - Flags: review?(jaws) → review-
Status: NEW → ASSIGNED
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review233740

Looks good, basically done after you make the following changes. r+ from me with the below changes made.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:48
(Diff revision 4)
> +  Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`,

This should be checking Assert.equal() with TOOLBAR_FIELD_BACKGROUND

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:50
(Diff revision 4)
> +
> +  urlBar.removeAttribute("focused");
> +
> +  Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`,
> +                  "Hover Background Color is set back to initial");

remove the "Hover" word here

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:51
(Diff revision 4)
> +  Assert.notEqual(window.getComputedStyle(urlBar).color,
> +                  `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`,

This shoudl be checking Assert.equal() with TOOLBAR_FIELD_COLOR

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:53
(Diff revision 4)
> +  Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`,
> +                  "Hover Background Color is set back to initial");
> +  Assert.notEqual(window.getComputedStyle(urlBar).color,
> +                  `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`,
> +                  "Hover Text Color is set back to initial");

remove the "Hover" here

::: toolkit/modules/LightweightThemeConsumer.jsm:85
(Diff revision 4)
>          break;
>      }
>    },
>  
>    _update(aData) {
> +    console.log(aData);

remove the console.log() here and below

::: toolkit/modules/LightweightThemeConsumer.jsm:186
(Diff revision 4)
>  
>  function _setProperties(root, active, vars) {
> +  console.log(vars);
>    for (let map of [toolkitVariableMap, ThemeVariableMap]) {
>      for (let [cssVarName, varsKey, optionalElementID] of map) {
> +      // console.log(cssVarName, varsKey, optionalElementID, vars[varsKey]);

Remove this comment
Attachment #8956736 - Flags: review?(jaws) → review+
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review233346

In addition to :jaws comments, could you rename all `HOVER` vars in your test to use `FOCUS` ?

::: browser/themes/shared/urlbar-searchbar.inc.css:68
(Diff revision 4)
> -  background-color: var(--url-and-searchbar-background-color, white);
> +  background-color: var(--lwt-toolbar-field-focus, white);
> +  color: var(--lwt-toolbar-field-focus-color, black);
> +  border-color: var(--lwt-toolbar-field-focus-border-color, white);

Those 3 properties should first fallback to the non-focus properties if the focus properties are no specified, then fallback to the default values.

Here's how it would be done for background-color:

background-color: var(--lwt-toolbar-field-focus, var(--url-and-searchbar-background-color, white));

This basically says if the focus variable is not specified, use the non-focus variable, and if that's not specified, use white.
Attachment #8956736 - Flags: review?(ntim.bugs)
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review234336

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:30
(Diff revision 6)
> +  // Remove the `remotecontrol` attribute since it interferes with the urlbar styling.
> +  document.documentElement.removeAttribute("remotecontrol");

I think what jaws meant here was:

```
document.documentElement.removeAttribute("remotecontrol");

registerCleanupFunction(() => {
  document.documentElement.setAttribute("remotecontrol", "true");
});
```

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:33
(Diff revision 6)
> +  await extension.startup();
> +
> +  // Remove the `remotecontrol` attribute since it interferes with the urlbar styling.
> +  document.documentElement.removeAttribute("remotecontrol");
> +
> +  info("Checking toolbar field's hover color");

s/hover/focus

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:55
(Diff revision 6)
> +  // Restore the `remotecontrol` attribute at the end of the test.
> +  document.documentElement.setAttribute("remotecontrol", true);
> +  registerCleanupFunction(() => {
> +    SpecialPowers.clearUserPref("remotecontrol");
> +  });

If you do what I wrote above, you should be able to remove this.
Attachment #8956736 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review234342

::: toolkit/components/extensions/test/browser/browser.ini:20
(Diff revision 6)
>  [browser_ext_themes_separators.js]
>  [browser_ext_themes_static_onUpdated.js]
>  [browser_ext_themes_tab_line.js]
>  [browser_ext_themes_tab_loading.js]
>  [browser_ext_themes_tab_text.js]
> +[browser_ext_themes_toolbar_fields_hover.js]

s/[browser_ext_themes_toolbar_fields_hover.js]/[browser_ext_themes_toolbar_fields_focus.js]
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review234472


Code analysis found 5 defects in this patch:
 - 5 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_toolbar_fields_focus.js:51
(Diff revision 7)
> +  testBorderColor(urlBar, TOOLBAR_FOCUS_BORDER);
> +
> +  urlBar.removeAttribute("focused");
> +
> +  Assert.equal(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,

Error: Expected indentation of 15 spaces but found 18. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:52
(Diff revision 7)
> +
> +  urlBar.removeAttribute("focused");
> +
> +  Assert.equal(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,
> +                  "Background Color is set back to initial");

Error: Expected indentation of 15 spaces but found 18. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:54
(Diff revision 7)
> +
> +  Assert.equal(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,
> +                  "Background Color is set back to initial");
> +  Assert.equal(window.getComputedStyle(urlBar).color,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`,

Error: Expected indentation of 15 spaces but found 18. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:55
(Diff revision 7)
> +  Assert.equal(window.getComputedStyle(urlBar).backgroundColor,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,
> +                  "Background Color is set back to initial");
> +  Assert.equal(window.getComputedStyle(urlBar).color,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`,
> +                  "Text Color is set back to initial");

Error: Expected indentation of 15 spaces but found 18. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:56
(Diff revision 7)
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`,
> +                  "Background Color is set back to initial");
> +  Assert.equal(window.getComputedStyle(urlBar).color,
> +                  `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`,
> +                  "Text Color is set back to initial");
> +                  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Summary: Allow theming toolbar fields hover & focused state → Allow theming toolbar fields focused state
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fbbb00841e4
Allow theming toolbar fields hover & focused state r=jaws,ntim
Backed out changeset 6fbbb00841e4 (bug 1423757) for failing browser_ext_themes_toolbar_fields_focus.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/03a0851d631012ed637e28092385642871fd05fd

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6fbbb00841e489a639cbe766ad5f02c798c21ac7

Failure log link: https://treeherder.mozilla.org/logviewer.html#?job_id=169988257&repo=autoland&lineNumber=1983

Log snippet:
[task 2018-03-23T22:43:04.996Z] 22:43:04     INFO - TEST-START | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js
[task 2018-03-23T22:43:04.997Z] 22:43:04     INFO - TEST-INFO | started process screentopng
[task 2018-03-23T22:43:06.029Z] 22:43:06     INFO - TEST-INFO | screentopng: exit 0
[task 2018-03-23T22:43:06.030Z] 22:43:06     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js | Exception thrown - [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.importGlobalProperties]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js :: <TOP_LEVEL> :: line 3"  data: no]
[task 2018-03-23T22:43:06.033Z] 22:43:06     INFO - GECKO(1054) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2018-03-23T22:43:06.033Z] 22:43:06     INFO - GECKO(1054) | MEMORY STAT | vsize 2008MB | residentFast 253MB | heapAllocated 99MB
[task 2018-03-23T22:43:06.033Z] 22:43:06     INFO - TEST-OK | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js | took 49ms
[task 2018-03-23T22:43:06.034Z] 22:43:06     INFO - checking window state
[task 2018-03-23T22:43:06.035Z] 22:43:06     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-03-23T22:43:06.679Z] 22:43:06     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-03-23T22:43:07.482Z] 22:43:07     INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1167
[task 2018-03-23T22:43:07.490Z] 22:43:07     INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1191
[task 2018-03-23T22:43:07.608Z] 22:43:07     INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1109
[task 2018-03-23T22:43:07.838Z] 22:43:07     INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1054
Flags: needinfo?(lianzhen)
Comment on attachment 8956736 [details]
Bug 1423757 - Allow theming toolbar fields focused state

https://reviewboard.mozilla.org/r/225696/#review236324

::: commit-message-bccdc:1
(Diff revision 8)
> +Bug 1423757 - Allow theming toolbar fields hover & focused state r?ntim,jaws

nit: remove the "hover" part from the commit message

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:3
(Diff revision 8)
> +"use strict";
> +
> +Cu.importGlobalProperties(["InspectorUtils"]);

Looks like you don't need this, and it seems to be causing the problem in your test too.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e811edda21f3
Allow theming toolbar fields focused state r=jaws,ntim
Flags: needinfo?(lianzhen)
https://hg.mozilla.org/mozilla-central/rev/e811edda21f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is it planned to uplift this bug to beta? Then ESR will have the same theme feature set.
Flags: needinfo?(jaws)
No, we're not planning on uplifting theme features to beta for ESR support. It may be nice to not have the difference between release and ESR, but there are a number of new API features for theming that are landing in 61 and uplifting all of them would make fixing any bugs that are found during QA much riskier. We'll only uplift theme API changes if it is to fix a regression or compatibility problem, generally.
Flags: needinfo?(jaws)
Depends on: 1449765
Product: Toolkit → WebExtensions
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)
Flags: needinfo?(lianzhen) → qe-verify-
You need to log in before you can comment on or make changes to this bug.