Allow theming arrow panels

RESOLVED FIXED in Firefox 60

Status

P5
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: ntim, Assigned: masinico, Mentored)

Tracking

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

unspecified
mozilla60
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Here are the properties that can be supported:

colors.arrowpanel
colors.arrowpanel_text
colors.arrowpanel_border
(Reporter)

Updated

a year ago
Blocks: 1348034
No longer blocks: 1330328
(Reporter)

Updated

a year ago
Depends on: 1408121

Updated

a year ago
Priority: -- → P5
(Reporter)

Updated

a year ago
Depends on: 1425868
Assignee: jaws → masinico
Mentor: mconley, jaws, ntim.bugs
(Reporter)

Updated

a year ago
Keywords: dev-doc-needed
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review223838

::: toolkit/components/extensions/ext-theme.js:160
(Diff revision 3)
> +        case "arrowpanel":
> +          this.lwtStyles.arrowpanel = cssColor;
> +          break;
> +        case "arrowpanel_text":
> +          this.lwtStyles.arrowpanel_text = cssColor;
> +          break;
> +        case "arrowpanel_border":
> +          this.lwtStyles.arrowpanel_border = cssColor;
> +          break;

The 3 cases are already covered by the 3 other lines you added before.

::: toolkit/components/extensions/test/browser/browser.ini:21
(Diff revision 3)
>  [browser_ext_themes_static_onUpdated.js]
>  [browser_ext_themes_tab_text.js]
>  [browser_ext_themes_toolbar_fields.js]
>  [browser_ext_themes_toolbars.js]
>  [browser_ext_themes_toolbarbutton_icons.js]
> +[browser_ext_themes_arrowpanels.js]

You forgot to add the test file. (hg add path/to/file
)
Attachment #8948605 - Flags: review?(ntim.bugs)
Comment hidden (mozreview-request)
Hi Connor, thanks for working on this! It'd be very cool if you attached a screenshot of your patch in action to this bug, so that people who are watching this can get a feel for what it looks like. Thanks!
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review224140

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:4
(Diff revision 4)
> +"use strict";
> +
> +
> +ignoreAllUncaughtExceptions();

Why is this here ?

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:14
(Diff revision 4)
> +add_task(async function test_arrowpanel_styling(browser, accDoc) {
> +  const ARROWPANEL_BACKGROUND_COLOR = "#FF0000";
> +  const ARROWPANEL_TEXT_COLOR = "#008000";
> +  const ARROWPANEL_BORDER_COLOR = "#0000FF";
> +
> +  //Create an extension with the added properties

add a space after // here and in the rest of the test

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:66
(Diff revision 4)
> +    Assert.equal(
> +      window.getComputedStyle(arrowpanel).getPropertyValue("--arrowpanel-border-color"),
> +      ARROWPANEL_BORDER_COLOR,
> +      "Arrowpanel background color"
> +    );

I wrote this function to test border colors: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js#13-26

You can move it into head.js, then use it in your test.
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review224454

::: toolkit/components/extensions/test/browser/head.js:57
(Diff revision 5)
>  function imageBufferFromDataURI(encodedImageData) {
>    let decodedImageData = atob(encodedImageData);
>    return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer;
>  }
> +
> +function testBorderColor(element, expected) {

Now that it's in head.js, can you remove it from browser_ext_themes_toolbar_fields.js ?

::: toolkit/components/extensions/test/browser/head.js:60
(Diff revision 5)
> +               "Field left border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderRightColor,
> +               hexToCSS(expected),
> +               "Field right border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderTopColor,
> +               hexToCSS(expected),
> +               "Field top border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderBottomColor,
> +               hexToCSS(expected),
> +               "Field bottom border color should be set.");

Should be "Element" instead of "Field"

Comment 10

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review224466


Code analysis found 1 defect in this patch:
 - 1 defect 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/head.js:57
(Diff revision 5)
>  function imageBufferFromDataURI(encodedImageData) {
>    let decodedImageData = atob(encodedImageData);
>    return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer;
>  }
> +
> +function testBorderColor(element, expected) {

Error: 'testbordercolor' is defined but never used. allowed unused vars must match /^(cc|ci|cr|cu|exported_symbols)$/. [eslint: no-unused-vars]
(Reporter)

Comment 11

a year ago
(In reply to Code Review Bot [:reviewbot] from comment #10)
> 
> ::: toolkit/components/extensions/test/browser/head.js:57
> (Diff revision 5)
> >  function imageBufferFromDataURI(encodedImageData) {
> >    let decodedImageData = atob(encodedImageData);
> >    return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer;
> >  }
> > +
> > +function testBorderColor(element, expected) {
> 
> Error: 'testbordercolor' is defined but never used. allowed unused vars must
> match /^(cc|ci|cr|cu|exported_symbols)$/. [eslint: no-unused-vars]

Ah yes, you need to add "testBorderColor" on top of the head.js file in the /* exported ... */ comment

Comment 12

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review224514

Looks good, Connor! I have a few suggestions below. Thanks for the patch!

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:33
(Diff revision 5)
> +  await BrowserTestUtils.withNewTab({ gBrowser, url: "about:home" }, async function(browser) {
> +    await extension.startup();
> +
> +    // Load a webpage to access the information arrow panel
> +    let promise = BrowserTestUtils.browserLoaded(browser);
> +    browser.loadURI("https://example.com");
> +    await promise;

If you're sending the tab to example.com anyways, might as well just load that instead of about:home on line 33.

Then you can get rid of lines 36-39.

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:42
(Diff revision 5)
> +    let identityBox = document.querySelector("#identity-box");
> +    identityBox.setAttribute("open", "true");
> +    identityBox.click();

Usually, you have to wait for the popupshown event to fire to be sure that the panel is actually open. There are also some variables stashed around that you can use instead of re-querying for ID's.

Here's an example: https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/browser/base/content/test/permissions/browser_permissions.js#9-19

I've also included a function that'll close the popup, which you'll need to do at the end up the test.

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:46
(Diff revision 5)
> +    // Open the information arrow panel by "clicking" the info icon
> +    let identityBox = document.querySelector("#identity-box");
> +    identityBox.setAttribute("open", "true");
> +    identityBox.click();
> +
> +    let arrowpanel = document.getElementsByClassName("panel-arrowcontent")[0];

Instead of querying like this and relying on DOM structure, I think you can use:

```js
let arrowContent = document.getAnonymousElementByAttribute(gIdentityHandler._identityPopup, "class", "panel-arrowcontent");
```

which should be less brittle.

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:50
(Diff revision 5)
> +
> +    let arrowpanel = document.getElementsByClassName("panel-arrowcontent")[0];
> +
> +    // Ensure arrowpanel background color was set properly
> +    Assert.equal(
> +      window.getComputedStyle(arrowpanel).getPropertyValue("background-color"),

You can probably do this getComputedStyle once before these assertions, and check the return value instead (see a similar suggestion I made for `testBorderColor()` if you're not sure what I mean)

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:52
(Diff revision 5)
> +
> +    // Ensure arrowpanel background color was set properly
> +    Assert.equal(
> +      window.getComputedStyle(arrowpanel).getPropertyValue("background-color"),
> +      `rgb(${hexToRGB(ARROWPANEL_BACKGROUND_COLOR).join(", ")})`,
> +      "Arrowpanel background color"

"Arrow panel background color should have been themed"

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:59
(Diff revision 5)
> +
> +    // Ensure arrowpanel text color was set properly
> +    Assert.equal(
> +      window.getComputedStyle(arrowpanel).getPropertyValue("color"),
> +      `rgb(${hexToRGB(ARROWPANEL_TEXT_COLOR).join(", ")})`,
> +      "Arrowpanel text color"

"Arrow panel text color should have been themed"

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:64
(Diff revision 5)
> +      "Arrowpanel text color"
> +     );
> +
> +    // Ensure arrowpanel border color was set properly
> +    testBorderColor(arrowpanel, ARROWPANEL_BORDER_COLOR)
> +

Here's where we need to close the panel, and wait for it to fire popuphidden.

::: toolkit/components/extensions/test/browser/head.js:58
(Diff revision 5)
> +  Assert.equal(window.getComputedStyle(element).borderLeftColor,
> +               hexToCSS(expected),
> +               "Field left border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderRightColor,
> +               hexToCSS(expected),
> +               "Field right border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderTopColor,
> +               hexToCSS(expected),
> +               "Field top border color should be set.");
> +  Assert.equal(window.getComputedStyle(element).borderBottomColor,

While we're moving this, I think we can improve it:

1. We should be able to run getComputedStyle once and use the return value repeatedly
2. "Field X border color should be set" is kind of a misnomer now because we're using it for a panel too. Perhaps let's call it an element instead.

What I'm suggesting is something like this:

```js
let styles = window.getComputedStyles(element);
Assert.equal(styles.borderLeftColor, hextToCSS(expected),
             "Left border color should be set on element");
Assert.equal(styles.borderRightColor, hextToCSS(expected),
             "Right border color should be set on element"););
Assert.equal(styles.borderTopColor, hextToCSS(expected),
             "Top border color should be set on element");
Assert.equal(styles.borderBottomColor, hextToCSS(expected),
             "Bottom border color should be set on element");
```
Attachment #8948605 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a year ago
(Reporter)

Comment 17

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review224726

Looks close from landing!

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:23
(Diff revision 7)
> +add_task(async function test_arrowpanel_styling(browser, accDoc) {
> +  const ARROWPANEL_BACKGROUND_COLOR = "#FF0000";
> +  const ARROWPANEL_TEXT_COLOR = "#008000";
> +  const ARROWPANEL_BORDER_COLOR = "#0000FF";
> +
> +  // Create an extension with the added properties

nit: you probably don't need this comment.

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:49
(Diff revision 7)
> +
> +  await BrowserTestUtils.withNewTab({ gBrowser, url: "https://example.com" }, async function(browser) {
> +    await extension.startup();
> +
> +    // Open the information arrow panel
> +    openIdentityPopup();

Should be `await openIdentityPopup();` since you want to wait for the promise to resolve.

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:70
(Diff revision 7)
> +     );
> +
> +    // Ensure arrowpanel border color was set properly
> +    testBorderColor(arrowContent, ARROWPANEL_BORDER_COLOR)
> +
> +    closeIdentityPopup();

Same here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I suspect "arrowpanel" is too specific. Themes will probably want to style popups consistently, so we should pick a more generic name such as "menu" or "popup" or "panel", even though we may support it only for arrow panels initially. Bug 1417883 would be the first candidate to use the same variables in another context.
(Reporter)

Comment 21

a year ago
(In reply to Dão Gottwald [::dao] from comment #20)
> I suspect "arrowpanel" is too specific. Themes will probably want to style
> popups consistently, so we should pick a more generic name such as "menu" or
> "popup" or "panel", even though we may support it only for arrow panels
> initially. Bug 1417883 would be the first candidate to use the same
> variables in another context.

I agree "arrowpanel" is probably a bad naming choice, since the fact that we use arrows is just an UX decision, and it might change in the future.

However, I don't think we should use the same property for both the autocomplete dropdowns and the arrow panels. Those two differ UX-wise in Firefox, so themes might also want to make them differ too, we should give them the flexibility of doing so. Also, it doesn't add much maintenance cost to have separate properties for those two, since the variables already exist internally.
Arrow panels don't have a consistent UX to begin with. Some look like menus, others have entirely random content, so I don't see how the address bar and search bar popups wouldn't fit in there.

Maintainability isn't the concern here. The concern is bad API design, i.e. providing APIs that are more confusing than useful. If we can make hacking together an internally consistent theme simpler at the cost of not supporting fringe use cases, that's a win in my book.
(Reporter)

Comment 24

a year ago
(In reply to Dão Gottwald [::dao] from comment #23)
> Arrow panels don't have a consistent UX to begin with. Some look like menus,
> others have entirely random content, so I don't see how the address bar and
> search bar popups wouldn't fit in there.

How you interact with autocomplete popups is entirely different, autocomplete popups are keyboard oriented, while arrow panels are mouse oriented. This also has consequences on how they look: autocomplete popups have a special selected state (blue), while arrow panels essentially don't. If anything, unifying those two conceptually different APIs seems confusing to me.
Keyboard access for arrow panels is planned as part of bug 1418973. The selected state can be supported as something like panel_highlight_background and panel_highlight_text, which doesn't imply that every panel must use these colors.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review225056

r+ with the one change below

::: browser/themes/linux/customizableui/panelUI.css
(Diff revision 13)
>   * Linux works around this issue. This bug is on file as 1394575.
>   */
>  #pageActionFeedback > .panel-arrowcontainer > .panel-arrowcontent {
>    box-shadow: none;
>  }
> -

Please undo this change.
Attachment #8948605 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Attachment #8948605 - Flags: review?(mconley)
Looks like jaws beat me to this one.

Comment 33

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e81474b90dc
Allow theming arrow panels(popups). r=jaws
Comment hidden (mozreview-request)
(Reporter)

Comment 37

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review225042

::: browser/themes/shared/customizableui/panelUI.inc.css:959
(Diff revision 15)
>    min-height: unset;
> +  border: 1px solid #e1e1e1;
> +  border-radius: 10000px;
> +  padding: 1px 8px;
>  }
>  

Thanks for combining the rules.

Can you also change the border color to be var(--panel-separator-color) ?
Comment hidden (mozreview-request)
(Reporter)

Comment 39

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review225076

::: browser/themes/shared/customizableui/panelUI.inc.css:960
(Diff revisions 15 - 16)
>    border: 1px solid #e1e1e1;
>    border-radius: 10000px;
>    padding: 1px 8px;
> +  background-color: var(--arrowpanel-background);
> +}
> +
> +#appMenu-zoomReset-button@buttonStateHover@ {
> +  background-color: var(--arrowpanel-dimmed);
> +}
> +
> +#appMenu-zoomReset-button@buttonStateActive@ {
> +  background-color: var(--arrowpanel-dimmed-further);
>  }

You probably don't want to change the styling too much so:

border: 1px solid var(--panel-separator-color);
background-color: var(--arrowpanel-dimmed);

For hover:

background-color: var(--arrowpanel-dimmed-further);


For active:

background-color: var(--arrowpanel-dimmed-even-further);
(Assignee)

Comment 40

a year ago
The 
>    border-radius: 10000px;
>    padding: 1px 8px;
are needed to make the zoom reset button the rectangle with curved edges shape it currently is instead of a regular boxy rectangle so I left them in.  I did change the colors for the various states though.
Flags: needinfo?(masinico)
Comment hidden (mozreview-request)
(Reporter)

Comment 42

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review225482
Attachment #8948605 - Flags: review+

Comment 43

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/719333bb05af
Allow theming arrow panels(popups). r=jaws
(Reporter)

Comment 45

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review225754

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:67
(Diff revision 17)
> +      `rgb(${hexToRGB(POPUP_TEXT_COLOR).join(", ")})`,
> +      "Popup text color should have been themed"
> +    );
> +
> +    // Ensure popup border color was set properly
> +    testBorderColor(arrowContent, POPUP_BORDER_COLOR);

On macOS, the border is applied as a box-shadow, so you'll probably want something like:

if (AppConstants.platform == "macosx") {
  Assert.ok(
    arrowContentComputedStyle.getPropertyValue("box-shadow").includes(`rgb(${hexToRGB(POPUP_TEXT_COLOR).join(", ")})`),
    "Popup border color should be set"
  );
} else {
  testBorderColor(...);
}

Comment 46

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review226090

Clearing review request - this already landed (and bounced). ntim has good feedback here on how to get the test fixed.
Attachment #8948605 - Flags: review?(mconley)
Comment hidden (mozreview-request)
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 b0e38fb1dc4807a742ccfdaf56774cd7bf44fbc6 -d 9688823d23fd: rebasing 447624:b0e38fb1dc48 "Bug 1417880: Allow theming arrow panels(popups). r=jaws,ntim" (tip)
merging browser/themes/shared/customizableui/panelUI.inc.css
merging toolkit/components/extensions/ext-theme.js
merging toolkit/components/extensions/schemas/theme.json
merging toolkit/components/extensions/test/browser/browser.ini
merging toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js
merging toolkit/components/extensions/test/browser/head.js
merging toolkit/modules/LightweightThemeConsumer.jsm
warning: conflicts while merging toolkit/modules/LightweightThemeConsumer.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
(Reporter)

Comment 51

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review226688

::: toolkit/modules/LightweightThemeConsumer.jsm:18
(Diff revision 20)
> +  ["--arrowpanel-background", "popup"],
> +  ["--arrowpanel-color", "popup_text"],
> +  ["--arrowpanel-border-color", "popup_border"],

Remove this, and add this in ThemeVariableMap.jsm (it's in the browser/ directory).
Attachment #8948605 - Flags: review+
(In reply to Tim Nguyen :ntim from comment #51)
> Comment on attachment 8948605 [details]
> Bug 1417880: Allow theming arrow panels(popups).
> 
> https://reviewboard.mozilla.org/r/218022/#review226688
> 
> ::: toolkit/modules/LightweightThemeConsumer.jsm:18
> (Diff revision 20)
> > +  ["--arrowpanel-background", "popup"],
> > +  ["--arrowpanel-color", "popup_text"],
> > +  ["--arrowpanel-border-color", "popup_border"],
> 
> Remove this, and add this in ThemeVariableMap.jsm (it's in the browser/
> directory).

Why? --arrowpanel-* are toolkit variables.
The ThemeVariableMap.jsm are app specific now. Some variables use IDs which are FX specific and TB don't have. See bug 1436100.
Yet --arrowpanel-* are toolkit variables. Why not set them in toolkit/?
(Reporter)

Comment 55

a year ago
(In reply to Dão Gottwald [::dao] from comment #54)
> Yet --arrowpanel-* are toolkit variables. Why not set them in toolkit/?

ThemeVariableMap.jsm is just a map from existing CSS variables to the property names exposed in each product.
(Reporter)

Comment 56

a year ago
There's also no ThemeVariableMap.jsm inside toolkit atm.
(In reply to Tim Nguyen :ntim from comment #56)
> There's also no ThemeVariableMap.jsm inside toolkit atm.

Right. LightweightThemeConsumer.jsm is already in toolkit so it wouldn't need another module for toolkit variables.

Comment 58

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29569f02015d
Allow theming arrow panels. r=jaws, ntim
Why was this landed without the question where toolkit variables should go sorted out?
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 60

a year ago
(In reply to Dão Gottwald [::dao] from comment #59)
> Why was this landed without the question where toolkit variables should go
> sorted out?

Bug 1436100 introduced the change, so I don't see why this bug specifically should fix its fallouts.
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/29569f02015d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
We seem to keep talking past each other. Bug 1436100 moved browser variable mappings to browser/, which makes a lot of sense. This bug doing the same with toolkit variables isn't bug 1436100's fallout. Now, I'm not saying that putting these variables in browser/ is a showstopper, but cutting the discussion short by just landing the patch after a review peer raised a concern isn't the way to go.

Turns out there's also a pending review request for mconley.

I've backed this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the one in browser extends whatever is defined by the toolkit one?
(Reporter)

Comment 65

a year ago
(In reply to Dão Gottwald [::dao] from comment #62)
> cutting the discussion short by just landing the patch after a review peer raised a concern isn't the way to go.

That wasn't my intent. Since I didn't see any direct reply relating to comment 55 (which was my answer to the question), I assumed you were ok with it. Sorry for the misunderstanding. Feel free to needinfo? me next time if whatever answer I provide is not sufficient.


To expand on comment 55, having toolkit CSS variables in the product-specific mapping doesn't really break the meaning of "product-specific mapping", since toolkit/ is technically part of the product. Also, I think introducing a new map for toolkit unnecessarily introduces complexity without actually giving us much benefit. The only benefit I possibly see is reducing the duplicated items between comm-central and mozilla-central, but I don't really see what it would bring otherwise...
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #64)
> Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the
> one in browser extends whatever is defined by the toolkit one?

As I tried to say in comment 57 already, why not just let LightweightThemeConsumer.jsm keep its own array besides the app-specific ThemeVariableMap.jsm?
fwiw, I think a review from jaws and ntim should be fine, and shouldn't require yet another review from me in order to land. That's definitely not Connor's fault here - more of a process thing; I imagine whoever gets to the patch first can review it, and clear the other reviewers unless they don't feel confident being the sole reviewer. If that was the case here, I can still review - just let me know.

In any event, Connor, sorry you're kinda caught here - a discussion about where variables need to live is happening, and that's what's blocking your patch. Not your fault at all. We'll get this sorted out soon.

ni?ing ntim for dao's question in comment 66.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 69

a year ago
(In reply to Dão Gottwald [::dao] from comment #66)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #64)
> > Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the
> > one in browser extends whatever is defined by the toolkit one?
> 
> As I tried to say in comment 57 already, why not just let
> LightweightThemeConsumer.jsm keep its own array besides the app-specific
> ThemeVariableMap.jsm?

We could do something similar to `ThemeVariableMap = ThemeVariableMap.concat(<toolkit vars>)`.

I honestly still don't understand where's the benefit though. The current mapping reads "CSS variable X maps to WebExtension property Y on Firefox", it honestly does not make a difference that X is from toolkit, this is just an implementation detail... What matters the most in the mapping is the WebExtension property, not the CSS variable. Scattering the mapping over two places seems counterproductive and makes the mapping harder to read IMHO.

I'm not convinced this should be done unless there is some real benefit into doing this.
Flags: needinfo?(ntim.bugs) → needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #68)
> fwiw, I think a review from jaws and ntim should be fine, and shouldn't
> require yet another review from me in order to land. That's definitely not
> Connor's fault here - more of a process thing; I imagine whoever gets to the
> patch first can review it, and clear the other reviewers unless they don't
> feel confident being the sole reviewer. If that was the case here, I can
> still review - just let me know.
> 
> In any event, Connor, sorry you're kinda caught here - a discussion about
> where variables need to live is happening, and that's what's blocking your
> patch. Not your fault at all. We'll get this sorted out soon.

The patch changed after jaws r+'d it and Connor didn't even seem to be involved with that, so definitely not his fault.

(In reply to Tim Nguyen :ntim from comment #69)
> I honestly still don't understand where's the benefit though. The current
> mapping reads "CSS variable X maps to WebExtension property Y on Firefox",
> it honestly does not make a difference that X is from toolkit, this is just
> an implementation detail...

Sure, it is an implementation detail. We're discussing the implementation here. What's your point?

> What matters the most in the mapping is the
> WebExtension property, not the CSS variable. Scattering the mapping over two
> places seems counterproductive and makes the mapping harder to read IMHO.

Harder to read for whom? I hope we don't point people to ThemeVariableMap.jsm as some kind of webextension theme documentation?

> I'm not convinced this should be done unless there is some real benefit into
> doing this.

The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 71

a year ago
> Harder to read for whom?

For future people looking at this code ? It doesn't allow you to have a concise overview of how properties are mapped for each product.

> The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff.

The main point bug 1436100 moved the mapping is because comm-central wanted more flexibility over the mapping... and because toolkit changes broke TB. It might be counterproductive for TB to have toolkit mappings forced on them: they might want to implement the "popup" property differently, simply because they don't have any arrow panels or urlbar autocomplete... There are probably more than one example on this, since TB and Firefox are quite different products.

It's really about how each product chooses how to implement the WebExtension property, if they choose to use the shared toolkit CSS variable, they may do it by adding a single line in the mapping, if they don't choose to use the shared variable or choose to extend it for their product, they can choose to include a different variable... 

I don't understand why the CSS variable location matters so much here, this is really about how each product chooses to implement the WebExtension property, not about where each CSS variable is located.
(In reply to Tim Nguyen :ntim from comment #71)
> > Harder to read for whom?
> 
> For future people looking at this code ? It doesn't allow you to have a
> concise overview of how properties are mapped for each product.

I assume by people you mean engineers. The only reference to ThemeVariableMap.jsm is in LightweightThemeConsumer.jsm where also the array for toolkit variables would reside. Seems straightforward enough.

> > The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff.
> 
> The main point bug 1436100 moved the mapping is because comm-central wanted
> more flexibility over the mapping... and because toolkit changes broke TB.
> It might be counterproductive for TB to have toolkit mappings forced on
> them: they might want to implement the "popup" property differently, simply
> because they don't have any arrow panels or urlbar autocomplete... There are
> probably more than one example on this, since TB and Firefox are quite
> different products.

Unless I'm missing something, ThemeVariableMap.jsm can map properties to app-specific variables despite LightweightThemeConsumer.jsm already mapping the same properties to toolkit theme variables.
(Reporter)

Comment 73

a year ago
> Unless I'm missing something, ThemeVariableMap.jsm can map properties to app-specific variables despite LightweightThemeConsumer.jsm already mapping the same properties to toolkit theme variables.

In the current setup, that would cause both the item from ThemeVariableMap.jsm and from LightweightThemeConsumer.jsm to be used. One wouldn't override the other.
Yes, that's how I'd expect it to work. We want to be able to map theme properties to multiple variables.
(Reporter)

Comment 75

a year ago
(In reply to Dão Gottwald [::dao] from comment #74)
> Yes, that's how I'd expect it to work. We want to be able to map theme
> properties to multiple variables.

What if TB doesn't want the mapping from toolkit ?
(In reply to Tim Nguyen :ntim from comment #75)
> (In reply to Dão Gottwald [::dao] from comment #74)
> > Yes, that's how I'd expect it to work. We want to be able to map theme
> > properties to multiple variables.
> 
> What if TB doesn't want the mapping from toolkit ?

You might as well ask "what if Thunderbird doesn't want rule X from toolkit stylesheet Y?" Thunderbird may override it, and is also free to not using the toolkit theme at all. But when it does, it seems reasonable to me that this is by default part of the package.

Comment 77

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review228620

::: toolkit/modules/LightweightThemeConsumer.jsm:20
(Diff revision 20)
>  ChromeUtils.defineModuleGetter(this, "LightweightThemeImageOptimizer",
>    "resource://gre/modules/addons/LightweightThemeImageOptimizer.jsm");
>  
> +  ["--arrowpanel-background", "popup"],
> +  ["--arrowpanel-color", "popup_text"],
> +  ["--arrowpanel-border-color", "popup_border"],

Please put these in an array before the ThemeVariableMap.jsm import.

::: toolkit/modules/LightweightThemeConsumer.jsm:201
(Diff revision 20)
>      elem.style.removeProperty(variableName);
>    }
>  }
>  
>  function _setProperties(root, active, vars) {
>    for (let [cssVarName, varsKey, optionalElementID] of ThemeVariableMap) {

... and walk both arrays here like so:

for (let map of [toolkitVariableMap, ThemeVariableMap]) {
  for (let [cssVarName, varsKey, optionalElementID] of map) {

Thanks and sorry for the hassle!
Attachment #8948605 - Flags: review?(mconley)
Comment hidden (mozreview-request)

Comment 79

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review228700

Clearing review - there are some open-issues that need to be addressed before this can land.
Attachment #8948605 - Flags: review?(mconley)
Oh! I apologize - that latest patch addresses the issues. Connor, would you mind please marking them "Fixed"?
Flags: needinfo?(masinico)
(Assignee)

Comment 81

a year ago
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #80)
> Oh! I apologize - that latest patch addresses the issues. Connor, would you
> mind please marking them "Fixed"?

All issues have been dealt with. Sorry I forgot about that!
Flags: needinfo?(masinico)

Comment 82

a year ago
mozreview-review
Comment on attachment 8948605 [details]
Bug 1417880: Allow theming arrow panels(popups).

https://reviewboard.mozilla.org/r/218022/#review228704

Just two cosmetic issues, but otherwise, this looks great! Thanks Connor!

::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:42
(Diff revision 21)
> +    },
> +    files: {
> +      "image1.png": BACKGROUND,
> +    },
> +  });
> +

Nit - please remove this extra newline.

::: toolkit/modules/LightweightThemeConsumer.jsm:14
(Diff revision 21)
> +["--arrowpanel-background", "popup"],
> +["--arrowpanel-color", "popup_text"],
> +["--arrowpanel-border-color", "popup_border"],

Please indent these 2 spaces.
Attachment #8948605 - Flags: review+
Comment hidden (mozreview-request)
Thanks, Connor! Pushed to Try for you. If it comes back green, we can go ahead and autoland this.
Comment hidden (mozreview-request)

Comment 86

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc6f924977b4
Allow theming arrow panels(popups). r=jaws,mconley,ntim

Comment 87

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc6f924977b4
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
:ntim, I think you have updated the docs page and data for this, so if you are happy with the screenshots we can mark it dev-doc-complete.
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

a year ago
Flags: needinfo?(ntim.bugs)
Keywords: dev-doc-needed → dev-doc-complete

Updated

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