Closed Bug 1431189 Opened 6 years ago Closed 6 years ago

Add Google Chrome toolbar button color properties

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: vivek3zero, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Bug 1431189 - Add google chrome toolbar button color properties:
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Split off from bug 1347184.

This bug should implement the `colors.*` properties described in bug 1347184.
Summary: Add Google Chrome toolbar button tint properties → Add Google Chrome toolbar button color properties
Priority: -- → P5
Comment on attachment 8946977 [details]
Bug 1431189 - Add google chrome toolbar button color properties:

https://reviewboard.mozilla.org/r/216818/#review222758

::: toolkit/components/extensions/ext-theme.js:150
(Diff revision 2)
>            this.lwtStyles[color] = cssColor;
>            break;
> +        case "button_background":
> +          this.lwtStyles.button_background = cssColor;
> +          break;
> +        case "button_hover_background":
> +          this.lwtStyles.button_hover_background = cssColor;
> +          break;
> +        case "button_active_background":
> +          this.lwtStyles.button_active_background = cssColor;
> +          break;

Combine these 3 cases with case above.

::: toolkit/components/extensions/schemas/theme.json:76
(Diff revision 2)
> +              "button_background": {
> +                "$ref": "ThemeColor",
> +                "optional": true
> +              },
> +              "button_hover_background": {
> +                "$ref": "ThemeColor",
> +                "optional": true
> +              },
> +              "button_active_background": {
> +                "$ref": "ThemeColor",
> +                "optional": true
> +              },

I prefer using button_background_hover/active as opposed to button_hover/active_background

Also, please move these properties at the end of the block (after "toolbar_vertical_separator").

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:3
(Diff revision 2)
> +// This test checks whether applied WebExtension themes that attempt to change
> +// the background color properties are applied correctly.

Can you change "the background color properties" to "the button background color properties" ?

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:34
(Diff revision 2)
> +  let toolbarbutton = document.querySelector("#home-button");
> +
> +  Assert.equal(

This is just testing the variable value, not testing whether the background is actually set.

Try this:
  Components.utils.importGlobalProperties(["InspectorUtils"]);

  let toolbarButton = document.querySelector("#home-button");
  let toolbarButtonIcon = document.getAnonymousElementByAttribute(toolbarButton, "class", "toolbarbutton-icon");

  Assert.equal(
    window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"),
    `rgb(${hexToRgb(BUTTON_BACKGROUND).join(", ")})`,
    "Toolbar button background is set."
  );
  
  InspectorUtils.addPseudoClassLock(toolbarButton, ":hover");
  
  Assert.equal(
    window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"),
    `rgb(${hexToRgb(BUTTON_BACKGROUND_HOVER).join(", ")})`,
    "Toolbar button hover background is set."
  );
  
  InspectorUtils.addPseudoClassLock(toolbarButton, ":active");

  Assert.equal(
    window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"),
    `rgb(${hexToRgb(BUTTON_BACKGROUND_ACTIVE).join(", ")})`,
    "Toolbar button active background is set!"
  );
  
  InspectorUtils.clearPseudoClassLocks(toolbarButton);
Comment on attachment 8946977 [details]
Bug 1431189 - Add google chrome toolbar button color properties:

https://reviewboard.mozilla.org/r/216818/#review222992

ntim's review here is sufficient.
Attachment #8946977 - Flags: review?(jaws) → review-
Comment on attachment 8946977 [details]
Bug 1431189 - Add google chrome toolbar button color properties:

https://reviewboard.mozilla.org/r/216818/#review223372

r- for the lack of passing test. Please fix the test and make sure it passes before submitting for review.

::: toolkit/components/extensions/ext-theme.js:149
(Diff revision 3)
>          case "toolbar_vertical_separator":
>            this.lwtStyles[color] = cssColor;
>            break;
> +        case "button_background":
> +          this.lwtStyles.button_background = cssColor;
> +          break;
> +        case "button_background_hover":
> +          this.lwtStyles.button_background_hover = cssColor;
> +          break;
> +        case "button_background_active":
> +          this.lwtStyles.button_background_active = cssColor;
> +          break;

Since these three strings ("button_background", "button_background_hover", and "button_background_active") are equal to their property names, we don't need to separately handle them here.

Instead we can add them to the list above (line 149) like so:

```js
...
case "toolbar_vertical_separator":
case "button_background":
case "button_background_hover":
case "button_background_active":
  this.lwtStyles[color] = cssColor;
  break;
```

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:40
(Diff revision 3)
> +  let toolbarButton = document.querySelector("#home-button");
> +  let toolbarButtonIcon = document.getAnonymousElementByAttribute(toolbarButton, "class", "toolbarbutton-icon");
> +
> +  Assert.equal(
> +    window.getComputedStyle(toolbarButtonIcon).getPropertyValue("background-color"),
> +    'rgb(${hexToRgb(BUTTON_BACKGROUND).join(", ")})',

Did you try running this test? This isn't actually going to build the string you think it will since you need to be using a grave character (`) instead of a single quote character (') to bound the string when you are using JavaScript template strings.

This is the error I got when I run your test locally:

TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js | Toolbar button background is set. - "rgb(222, 222, 222)" == "rgb(${hexToRgb(BUTTON_BACKGROUND).join(\", \")})" - JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js :: test_button_background_properties :: line 38
Stack trace:
chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:test_button_background_properties:38

Note how you wanted "rgb(222, 222, 222)" but instead you ended up with "rgb(${hexToRgb(BUTTON_BACKGROUND).join(\", \")})".
Attachment #8946977 - Flags: review?(jaws) → review-
Attachment #8946977 - Attachment is obsolete: true
Comment on attachment 8947996 [details]
Bug 1431189 - Add google chrome toolbar button color properties:

https://reviewboard.mozilla.org/r/217636/#review223424

::: browser/themes/shared/toolbarbuttons.inc.css:26
(Diff revision 2)
>  
>    /* This default value of --toolbarbutton-height is defined to prevent
>       CSS errors for an invalid variable. The value should not get used,
>       as a more specific value should be set when the value will be used. */
>    --toolbarbutton-height: 0;
> -}
> + }

nit: undo this whitespace change.
Comment on attachment 8947996 [details]
Bug 1431189 - Add google chrome toolbar button color properties:

https://reviewboard.mozilla.org/r/217636/#review223430

Looks good! r+ from me with the one remaining change below to make.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:47
(Diff revision 3)
> +  );
> +
> +  InspectorUtils.addPseudoClassLock(toolbarButton, ":hover");
> +
> +  Assert.equal(
> +    window.getComputedStyle(toolbarButtonIcon).getPropertyValue("background-color"),

The return value from getComputedStyle() is a "live" value, meaning that it will update when the pseudo-classes are added to the element.

This means that you don't have to re-call getComputedStyle each time.

You can cache the result of the first call, and then call .getPropertyValue("background-color") each time that you add a pseudo-class.
Attachment #8947996 - Flags: review?(jaws) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1687250b0dd9
Add google chrome toolbar button color properties: r=jaws
Flags: needinfo?(vivek3zero)
Vivek, did your latest changes fix the failure (./mach eslint path/to/file) ? If not, does adding /* global InspectorUtils */ on top of the file fix it ?
I tested locally with his latest changes, it looks fine. Going to land this. Thanks Vivek!
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/749425381bba
Add google chrome toolbar button color properties: r=jaws
https://hg.mozilla.org/mozilla-central/rev/749425381bba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Woohoo! Thanks, Vivek. 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.
Woohoooo! This is super exciting and yes, I would like to make an account on mozillians.org
I went ahead and signed in using my google account and this is the link where you should be able to vouch on:

https://mozillians.org/en-US/u/vivek3zero/

Thanks again!
Component: WebExtensions: Frontend → WebExtensions: Themes
(In reply to Vivek from comment #24)
> Woohoooo! This is super exciting and yes, I would like to make an account on
> mozillians.org
> I went ahead and signed in using my google account and this is the link
> where you should be able to vouch on:
> 
> https://mozillians.org/en-US/u/vivek3zero/
> 
> Thanks again!

\o/ You're vouched! Welcome onboard -- I hope to see you around!
Depends on: 1436749
I think https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme is updated with the properties added in this bug (minus the one that was later removed:

button_background_active
button_background_hover

But let me know if we need anything else.
Flags: needinfo?(ntim.bugs)
Looks good, I would just document how the properties are applied using a screenshot.
Flags: needinfo?(ntim.bugs)
ntim: I decided in the end that we should have a separate screenshot for each of the `colors` items: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#colors

Let me know if this works for you.
Flags: needinfo?(ntim.bugs)
(In reply to Will Bamberg [:wbamberg] from comment #28)
> ntim: I decided in the end that we should have a separate screenshot for
> each of the `colors` items:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> theme#colors
> 
> Let me know if this works for you.

This is a big improvement, thanks!

One thing though, right now, you have to read the code sample to know exactly what the property is doing, I think those two suggestions can improve this:
* It would be nice to use the same base example all the time, and only change the relevant property.
* it would be nice if the described item would always be in red in the examples (so the brain can connect the red color with the item). It's currently the case for most items, but some of them use a different color.

The styled item should just be brought out visually without having to spend time reading the code sample :)

Also, some very minor nits:
* now that `headerURL` is optional, it would be nice to avoid putting it in the examples when it isn't relevant.
* the `toolbar_top_separator` seems to screenshot a bug (the red separator being visible under the selected tab, because of the alpha channel).
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen :ntim from comment #29)
> (In reply to Will Bamberg [:wbamberg] from comment #28)
> * It would be nice to use the same base example all the time, and only
> change the relevant property.

I don't think this is totally practical, as in some cases it would make the screenshots hard to decipher. But, I've tried to keep to a fairly consistent and simple set of properties.

> * it would be nice if the described item would always be in red in the
> examples (so the brain can connect the red color with the item). It's
> currently the case for most items, but some of them use a different color.

That's a good plan, and I've done this.

> Also, some very minor nits:
> * now that `headerURL` is optional, it would be nice to avoid putting it in
> the examples when it isn't relevant.

I've done this, except for `tab_selected`, which didn't seem to work without it.

> * the `toolbar_top_separator` seems to screenshot a bug (the red separator
> being visible under the selected tab, because of the alpha channel).

This seems to have gone away in the new screenshot.
ntim, please let me know if you think this is good enough now, so I can mark this and the related bugs dev-doc-complete. Thanks!
Flags: needinfo?(ntim.bugs)
(In reply to Will Bamberg [:wbamberg] from comment #31)
> ntim, please let me know if you think this is good enough now, so I can mark
> this and the related bugs dev-doc-complete. Thanks!

Looks good, thanks!

Just a small note, the tab_text property can be used independently from tab_selected.
Flags: needinfo?(ntim.bugs)
> Just a small note, the tab_text property can be used independently from tab_selected.

Sure. The only reason I used both in the example is just that otherwise the tab text is red on black->unreadable. Thanks for the reviews ntim!
Keywords: dev-doc-needed → dev-doc-complete
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: