1417883 - Allow theming autocomplete popups

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement, P5)

Description

[Tim Nguyen :ntim]

The URLbar autocomplete popup should be themable.

Comment 1

[Tim Nguyen :ntim]

Attachment: Neutral colors on dark background

Comment 2

[Tim Nguyen :ntim]

Attachment: Neutral colors on light background

Comment 3

[Tim Nguyen :ntim]

Hi Stephen,

For this bug, we would like to use "neutral" colors that work both on light and dark backgrounds for the urlbar dropdown hover and active states.

At the moment, the colors are:

  --arrowpanel-dimmed: hsla(0,0%,80%,.3);
  --arrowpanel-dimmed-further: hsla(0,0%,80%,.45);
  --arrowpanel-dimmed-even-further: hsla(0,0%,80%,.8);

The problem with those colors is that the hover/active states aren't visible on a dark background.

My proposal is to change them to:

  --arrowpanel-dimmed: hsla(210,4%,50%,.1);
  --arrowpanel-dimmed-further: hsla(210,4%,50%,.18);
  --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25);

Screenshots of the proposed colors on a dark and light background have been attached.

What do you think about those colors?

Comment 4

[Tim Nguyen :ntim]

Attachment: Current colors on light background

Here is the current hover color for comparison.

Comment 5

[Tim Nguyen :ntim]

Attachment: Current colors on dark background

As you can see, with the dark background, the hover state is not very visible.

Comment 6

[Tim Nguyen :ntim]

There are 2 main parts to this bug: you'll need to add support for the properties (autocomplete, autocomplete_text, autocomplete_selected, autocomplete_selected_text), then you'll need to implement some logic ensuring minimum contrast for the blue urls, green action tips and the gray text that is shown in the popup.


For the first part, you can map the new properties to the CSS vars I've introduced in bug 1435019:
  autocomplete               -> --autocomplete-popup-background
  autocomplete_text          -> --autocomplete-popup-color
  autocomplete_selected      -> --autocomplete-popup-highlight-background
  autocomplete_selected_text -> --autocomplete-popup-highlight-color

You'll also need to do some minor tweaks in browser/themes/(linux/windows)/browser.css to ensure that --panel-separator-color is always hsla(210,4%,10%,.14); for :root:-moz-lwtheme.

For the second part, to solve the blue URLs (.ac-url) and the green action (.ac-action) tips, you'll need to set the [brighttext] attribute on the autocomplete popups depending on the color of autocomplete_text. See what we do here for toolbars: https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#8810-8832

Depending on the [brighttext] attribute of the popup, you'll need to set --urlbar-popup-url-color (blue url) and --urlbar-popup-action-color (green action text) to appropriate values so they are visible on every background.

As for the gray text part, you'll first need to introduce a new CSS var for all the instances of `GrayText` that affect the autocomplete popups: --autocomplete-popup-secondary-color. Most of these instances of GrayText are in browser/themes/shared/urlbar-autocomplete.inc.css and browser/themes/shared/searchbar.inc.css. GrayText will be the default value of that variable.

You'll then need to add some JS to set that variable to a faded version of `autocomplete_text`. Something similar to : https://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#3954-3967 but in JS.

Comment 7

[Matthew N. [:MattN]]

These new theme properties should be clear about whether they affect all autocomplete popups (e.g. search box suggestions, on <input> or form history/autofill, etc.) or only the address bar one. I would suggest a name other than "autocomplete" if it's only for the awesomebar.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

These should affect the awesomebar results and search box suggestions, but nothing within content such as form history/autofill or <input>. There are no properties that affect items within page content so I don't think we need to make that explicit in the naming.

Comment 9

[Matthew N. [:MattN]]

Autocomplete popups are also used in "chrome" such as about:preferences for homepage selection and in the bookmark panel for tag autocompletion. Btw. the form history/autofill dropdowns are rendered in the parent process. I would think users would expect them all to use the same styles.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

MattN and I chatted about this over IRC. I'm going to rescind comment 8, and say that we should instead push for the --autocomplete* styles to apply to all chrome-generated autocomplete popups, such as: form autofill, form history, autocomplete popup within the Bookmarks panel, and the URL bar and search results popup.

The main reasoning behind this is that user styles can alter webpage content, but have no ability to alter the styling of the autofill/form history popup. This will also simplify implementation as well. If we later find that the themed colors appearing in some places is odd, we can restrict those areas at that time.

Comment 11

[Mike de Boer [:mikedeboer]]

I wanted to take the time to list a bit of research/ Proof of Concept work I've done in the past, regarding color inter-/ extrapolation by 1) extending the ColorAnalyzer worker we have in-tree and 2) harnessing knowledge and prior art in color theory.

The PoC work can be found at https://reviewboard.mozilla.org/r/92030/diff/3#index_header, which is a full listing of the code and downloadable as a patch file. This work was done to see if we could extract and generate a full palette from any given image, like a favicon.

Generating a palette from a know base color is something slightly more specialized palette class can do. These are the best examples I could find:
1. https://github.com/arcarson/palette-generator/blob/master/src/palette_generator.js
   Looks like a well-written, particularly small module, published under the ISC license (abbreviated MIT).
   You can visually inspect the effects of the various palette types at http://www.perbang.dk/color+scheme/. I think the complementary scheme yields the most useful results and the resulting palette looks like the most useful of the bunch in size.
2. https://github.com/dojo/dojox/blob/master/color/Palette.js
   This is the Dojo toolkit palette class, BSD license. This looks to be set up less neatly, reusing less code which also results in less documentation that might explain its inner workings a bit better. I'm adverse to 'magic' code that does something without attempting basic simplification, thus self-documenting code. It does seem better suited to take the bits we need and not take the whole module.
3. https://github.com/bgrins/TinyColor/blob/master/tinycolor.js
   From our very own :bgrins! Not tiny, as the name might suggest, but not massive either, when you consider the features it packs. Apart from being a good reference when you find yourself to do any color math, it also allows for generating a palette; it returns three colors for the complementary scheme, so we'll need to some additional work.

The resulting colors, generated using a base, may then be classified using the Palette code I mentioned above, using a `findDominantColors()` logic or similar.

Please feel free to ask if you need more info, explanation, theory... It's fun! ;-)

Comment 12

[Mike de Boer [:mikedeboer]]

*known base color

Comment 13

[Tim Nguyen :ntim]

Mike, I thought we had agreed on using two fixed sets of colors for the blue url and the green action text, then picking the right set using the [brighttext] attribute rather than generating the sets of colors.

The only generated color here would be the gray text color, but I don't think that requires a palette generator.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Nguyen :ntim from comment #13)
> Mike, I thought we had agreed on using two fixed sets of colors for the blue
> url and the green action text, then picking the right set using the
> [brighttext] attribute rather than generating the sets of colors.
> 
> The only generated color here would be the gray text color, but I don't
> think that requires a palette generator.

That is correct, no need for a palette generator on this first pass. We may decide to use a palette generator later but for this v1 it is not necessary.

Comment 15

[Mike de Boer [:mikedeboer]]

ah ok, that's fine - I was thinking a bit ahead and wanted to put a braindump somewhere so we may be able to reference it later, if useful & necessary.

Comment 16

[Stephen Horlander [:shorlander]]

(In reply to Tim Nguyen :ntim from comment #3)
> Hi Stephen,
> 
> For this bug, we would like to use "neutral" colors that work both on light
> and dark backgrounds for the urlbar dropdown hover and active states.
> 
> At the moment, the colors are:
> 
>   --arrowpanel-dimmed: hsla(0,0%,80%,.3);
>   --arrowpanel-dimmed-further: hsla(0,0%,80%,.45);
>   --arrowpanel-dimmed-even-further: hsla(0,0%,80%,.8);
> 
> The problem with those colors is that the hover/active states aren't visible
> on a dark background.
> 
> My proposal is to change them to:
> 
>   --arrowpanel-dimmed: hsla(210,4%,50%,.1);
>   --arrowpanel-dimmed-further: hsla(210,4%,50%,.18);
>   --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25);
> 
> Screenshots of the proposed colors on a dark and light background have been
> attached.
> 
> What do you think about those colors?

Hi Tim!

Is there a reason we are trying to use one set of colors that works on both instead of two sets of colors customized for light and dark?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

We'll use two colors, this question came about before we decided to go the route of [brighttext] that is used for the toolbars.

We will introduce two new attributes: [arrowpanel-brighttext] and [autocomplete-brighttext]. We can determine and set these attributes in LightweightThemeConsumer.jsm when it gets the colors from the extension loading code. If possible, we should try to just set these attributes on #mainPopupSet instead of :root, unless there are other popups that are defined outside of #mainPopupSet.

We will have to convert the colors that come from the extension to RGBA colors to do the brighttext computation. Something like the following could work:
> processCSSColor(property, cssColor) {
>   let processedColor = ...
>   switch (property) {
>     case "arrowpanel_text":
>       // set the brighttext attribute
>      break;
>    case "accentcolor":
>      // remove alpha channel
>      break;
> }

Comment 18

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #16)
> Hi Tim!
> 
> Is there a reason we are trying to use one set of colors that works on both
> instead of two sets of colors customized for light and dark?

We're supporting more than just lightweight themes here, i.e. various OS themes. If we wanted to use two sets of colors depending on whether the OS theme is dark or light, we'd have to detect this too (and LightweightThemeConsumer.jsm wouldn't be the right place for this btw).

Comment 19

[Dylan Stokes [:dyl]]

Attachment: Bug 1417883 - Allow theming autocomplete popups.

Review commit: https://reviewboard.mozilla.org/r/219346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219346/

Comment 20

[Dylan Stokes [:dyl]]

Current review is a work in progress.

Comment 21

[Code Review Bot [:reviewbot]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review225060


Code analysis found 3 defects in this patch:
 - 3 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/modules/LightweightThemeConsumer.jsm:230
(Diff revision 1)
>  }
> +
> +function _processColor(aRoot, aActive, aProperty, aColor) {
> +  let [r, g, b] = _parseRGB(aColor);
> +  let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +  switch(aProperty) {

Error: Expected space(s) after "switch". [eslint: keyword-spacing]

::: toolkit/modules/LightweightThemeConsumer.jsm:232
(Diff revision 1)
> +function _processColor(aRoot, aActive, aProperty, aColor) {
> +  let [r, g, b] = _parseRGB(aColor);
> +  let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +  switch(aProperty) {
> +    case "autocomplete_text":
> +      if(luminance <= 110)

Error: Expected space(s) after "if". [eslint: keyword-spacing]

::: toolkit/modules/LightweightThemeConsumer.jsm:236
(Diff revision 1)
> +    case "autocomplete_text":
> +      if(luminance <= 110)
> +        aRoot.removeAttribute("autocomplete-brighttext");
> +      else
> +        aRoot.setAttribute("autocomplete-brighttext", "true");
> +        //_setProperty(aRoot, aActive, "--autocomplete-popup-secondary-color", `rgba(`+ r +`,`+ g +`,`+ b +`,.4)`);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 22

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/1-2/

Comment 23

[Dylan Stokes [:dyl]]

In current review made changes on top of Bug 1417880. Want to make sure we are on the right track. Also wrote some simple tests in the _popup.js test file. Will need to write a more advance test.

Comment 24

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/2-3/

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review225888

::: browser/themes/shared/urlbar-autocomplete.inc.css:56
(Diff revision 3)
>  .ac-separator:not([selected=true]) {
> -  color: GrayText;
> +  color: var(--autocomplete-popup-secondary-color);
> +}
> +
> +#mainPopupSet[autocomplete-brighttext] .ac-url {
> +  color: #ff00ff;

I like your taste but we should find some better colors than these :)

https://firefoxux.github.io/photon/visuals/color.html might be a good place to start.

I looked at the Dark theme and I think for blue text we should use the selection-background-color, which is #5675B9.

::: browser/themes/shared/urlbar-autocomplete.inc.css:60
(Diff revision 3)
> +#mainPopupSet[autocomplete-brighttext] .ac-url {
> +  color: #ff00ff;
> +}
> +
> +#mainPopupSet[autocomplete-brighttext] .ac-action {
> +  color: #00ffff;

I looked at the Dark theme and think that we could try using the same green color that is used in the location bar for the HTTPS EV label, #30e60b.

::: toolkit/components/extensions/test/browser/browser_ext_themes_popup.js:46
(Diff revision 3)
> +
> +  await extension.startup();
> +
> +  let urlbar = document.getElementById("urlbar");
> +
> +  Assert.equal(window.getComputedStyle(urlbar).getPropertyValue("--autocomplete-popup-background"),

This should be checking the acutal computed background-color of the element, not just the value of the variable.

First, you can store the computed style in a local variable (`urlbarCS` for example) so you don't need to duplicate the `window.getComputedStyle(urlbar)` each time.

Then, you should be comparing `urlbarCS.backgroundColor` with POPUP_COLOR.

Please make that change here and below.

::: toolkit/components/extensions/test/browser/browser_ext_themes_popup.js:48
(Diff revision 3)
> +
> +  let urlbar = document.getElementById("urlbar");
> +
> +  Assert.equal(window.getComputedStyle(urlbar).getPropertyValue("--autocomplete-popup-background"),
> +               POPUP_COLOR,
> +               "popup color is not set properly.");

These strings appear whether it passed or failed. So it should be written in the affirmative, or even better it should describe what is expected.

For example, `Popup backgorund color should be set to ${POPUP_COLOR}`

::: toolkit/components/extensions/test/browser/browser_ext_themes_popup.js:68
(Diff revision 3)
> +               "urlbar popup action color is not set properly.");
> +
> +  await extension.unload();

This should check that there is no "autocomplete-brighttext" attribute set on the mainPopupSet.

::: toolkit/modules/LightweightThemeConsumer.jsm:230
(Diff revision 3)
>    var rgb = aColorString.match(/^rgba?\((\d+), (\d+), (\d+)/);
>    rgb.shift();
>    return rgb.map(x => parseInt(x));
>  }
> +
> +function _processColor(aRoot, aActive, aProperty, aColor) {

This function name should say something about brighttext since that is what it does. Maybe call it _inferPopupColorsFromText.

When you call this function you pass in the #mainPopupSet element, but inside of this function you refer to it as the root element. You should just rename this to aElement.

::: toolkit/modules/LightweightThemeConsumer.jsm:233
(Diff revision 3)
> +  switch (aProperty) {
> +    case "popup_text":

What other properties are you planning to add here? Right now you should just change the switch to an if-statement since it has only one case.

Comment 26

[Dão Gottwald [:dao]]

LightweightThemeConsumer.jsm shouldn't assume the "mainPopupSet" id, as this an entirely arbitrary browser.xul choice. It probably shouldn't even assume that all relevant popups are in the same popupset.

Comment 27

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/3-4/

Comment 28

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/4-5/

Comment 29

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/5-6/

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review228778

::: browser/themes/shared/urlbar-autocomplete.inc.css:55
(Diff revision 6)
> +#mainPopupSet[autocomplete-brighttext] .ac-url {
> +  color: #5675B9;
> +}
> +
> +#mainPopupSet[autocomplete-brighttext] .ac-action {
> +  color: #30e60b;
>  }

* Could you please use photon blue-50 for the url ? https://design.firefox.com/photon/visuals/color.html#blue

* Can you rename autocomplete-brighttext to popup-brighttext ? (now that the property name is popup) Also, as dao said, it's better not to use #mainPopupSet, I'd prefer using the root element.


* I'd prefer that you specify on top of the file (after :root):

:root[popup-brighttext] {
  --urlbar-popup-url-color: #0a84ff;
  --urlbar-popup-action-color: #30e60b;
}

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:57
(Diff revision 6)
> +add_task(async function setup() {
> +  await SpecialPowers.pushPrefEnv({
> +    set: [["extensions.webextensions.themes.enabled", true]],
> +  });
> +});
> +

You don't need this function, the pref is enabled by default

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:123
(Diff revision 6)
> +    Services.prefs.clearUserPref(ONEOFF_URLBAR_PREF);
> +    await BrowserTestUtils.removeTab(tab);
> +  });
> +
> +  let visits = [];
> +  repeat(maxResults, i => {

the repeat function is just used once, can you remove it and just use a simple loop?

Comment 31

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/6-7/

Comment 32

[Tim Nguyen :ntim]

A few things I spotted:
* this doesn't seem to remove the popup-brighttext attribute or the secondary color variable when a theme unloads
* autocomplete-brighttext should be replaced with popup-brighttext in the test too

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review230084

Looks like ntim beat me to this - please see his review feedback. Thanks!

Comment 34

[Tim Nguyen :ntim]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review230090

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:217
(Diff revision 7)
> +  mainPopupSet = document.getElementById("mainPopupSet");
> +  Assert.equal(mainPopupSet.getAttribute("autocomplete-brighttext"),
> +               "true",
> +               "brighttext should be set to true!");
> +
> +  await extension.unload();

After unloading the theme, can you check if the secondary color variable and the popup-brighttext attribute are unset ?

Comment 35

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/7-8/

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review230756

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:11
(Diff revision 8)
> +const POPUP_URL_COLOR_DARK = `#1c78d4`;
> +const POPUP_ACTION_COLOR_DARK = `#008f8a`;
> +const POPUP_URL_COLOR_BRIGHT = `#0a84ff`;
> +const POPUP_ACTION_COLOR_BRIGHT = `#30e60b`;

Please use normal double quoted strings, since there's no templating being used.

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:16
(Diff revision 8)
> +const POPUP_URL_COLOR_DARK = `#1c78d4`;
> +const POPUP_ACTION_COLOR_DARK = `#008f8a`;
> +const POPUP_URL_COLOR_BRIGHT = `#0a84ff`;
> +const POPUP_ACTION_COLOR_BRIGHT = `#30e60b`;
> +
> +const GRAY_TEXT = "#6d6d6d";

GrayText changes depending on the platform, so this will probably fail on other platforms.

Here's how you can get the computed color for GrayText:

let span = document.createElement("span");
span.style.color = "GrayText";
return window.getComputedStyle(span).color;

This should give you the rgb form of GrayText.

::: toolkit/modules/LightweightThemeConsumer.jsm:234
(Diff revision 8)
> +      if (luminance <= 110) {
> +        aElement.removeAttribute("popup-brighttext");
> +        _setProperty(aElement, aActive, "--autocomplete-popup-secondary-color", "GrayText");
> +      } else {
> +        aElement.setAttribute("popup-brighttext", "true");
> +        _setProperty(aElement, aActive, "--autocomplete-popup-secondary-color", `rgba(` + r + `,` + g + `,` + b + `, 0.4)`);
> +      }

We want to use a dimmed version of popup_text in all cases when popup_text is set, and GrayText only when popup_text is not set.

You can detect if popup_text is set by checking if aColor is an empty string.

You also don't need the aProperty parameter here.

function _inferPopupColorsFromText(element, active, color) {
  if (!color) {
    aElement.removeAttribute("popup-brighttext");
    _setProperty(element, active, "--autocomplete-popup-secondary-color");
    return;
  }
  
  let [r, g, b] = ...
  let luminance = ...
  
  if (luminance <= 110) {
    element.removeAttribute("popup-brighttext");
  } else {
    element.setAttribute("popup-brighttext", "true");
  }
  
  _setProperty(element, active, "--autocomplete-popup-secondary-color", `rgba(${r}, ${g}, ${b}, 0.4)`);
}

Comment 37

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/8-9/

Comment 38

[Tim Nguyen :ntim]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231040

::: toolkit/modules/LightweightThemeConsumer.jsm:233
(Diff revision 9)
>  }
> +
> +function _inferPopupColorsFromText(element, active, color) {
> +  if (!color) {
> +    element.removeAttribute("popup-brighttext");
> +    _setProperty(element, active, "--autocomplete-popup-secondayr-color");

There's a typo, it should be secondary-color

Comment 39

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/9-10/

Comment 40

[Tim Nguyen :ntim]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231146

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:207
(Diff revision 10)
> +  let searator = document.getAnonymousElementByAttribute(results[1], "anonid", "separator");
> +  Assert.equal(window.getComputedStyle(searator).color,

nit: seperator not searator

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:229
(Diff revision 10)
> +  searator = document.getAnonymousElementByAttribute(results[1], "anonid", "separator");
> +  Assert.equal(window.getComputedStyle(searator).color,
> +               GRAY_TEXT,
> +               `Urlbar popup seperator color should be set to ${GRAY_TEXT}`);

same here.

::: toolkit/modules/LightweightThemeConsumer.jsm:17
(Diff revision 10)
> +  ["--autocomplete-popup-background", "popup"],
> +  ["--autocomplete-popup-color", "popup_text"],
> +  ["--autocomplete-popup-highlight-background", "popup_highlight"],
> +  ["--autocomplete-popup-highlight-color", "popup_highlight_text"],

nit: move these items to browser/modules/ThemeVariableMap.jsm

Comment 41

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/10-11/

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231332

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b03fd1004cac
Allow theming autocomplete popups. r=jaws,ntim

Comment 44

[Natalia Csoregi [:nataliaCs]]

Backed out for failing browser_ext_themes_autocomplete_popup.js

backout: https://hg.mozilla.org/integration/autoland/rev/afe65b05e3e9e3c111d97856472d7f892840ed3f

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b03fd1004cacad2e79713f85ab055099ae4b0da2

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166253511&repo=autoland&lineNumber=6085

[task 2018-03-06T17:23:25.280Z] 17:23:25     INFO - TEST-START | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js
[task 2018-03-06T17:23:26.299Z] 17:23:26     INFO - TEST-INFO | started process screentopng
[task 2018-03-06T17:23:26.760Z] 17:23:26     INFO - TEST-INFO | screentopng: exit 0
[task 2018-03-06T17:23:26.761Z] 17:23:26     INFO - Buffered messages logged at 17:23:25
[task 2018-03-06T17:23:26.761Z] 17:23:26     INFO - Entering test bound setup
[task 2018-03-06T17:23:26.762Z] 17:23:26     INFO - Leaving test bound setup
[task 2018-03-06T17:23:26.763Z] 17:23:26     INFO - Entering test bound test_popup_url
[task 2018-03-06T17:23:26.763Z] 17:23:26     INFO - Extension loaded
[task 2018-03-06T17:23:26.764Z] 17:23:26     INFO - Buffered messages logged at 17:23:26
[task 2018-03-06T17:23:26.765Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Should get maxResults=10 results - 
[task 2018-03-06T17:23:26.765Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Popup background color should be set to #85A400 - "rgb(133, 164, 0)" == "rgb(133, 164, 0)" - 
[task 2018-03-06T17:23:26.766Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Popup color should be set to #000000 - "rgb(0, 0, 0)" == "rgb(0, 0, 0)" - 
[task 2018-03-06T17:23:26.767Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Popup highlight background color should be set to #9400ff - "rgb(148, 0, 255)" == "rgb(148, 0, 255)" - 
[task 2018-03-06T17:23:26.767Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Popup highlight color should be set to #09b9a6 - "rgb(9, 185, 166)" == "rgb(9, 185, 166)" - 
[task 2018-03-06T17:23:26.767Z] 17:23:26     INFO - Buffered messages finished
[task 2018-03-06T17:23:26.771Z] 17:23:26     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Urlbar popup url color should be set to #1c78d4 - "rgb(240, 119, 70)" == "rgb(28, 120, 212)" - JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js :: test_popup_url :: line 150
[task 2018-03-06T17:23:26.772Z] 17:23:26     INFO - Stack trace:
[task 2018-03-06T17:23:26.772Z] 17:23:26     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:test_popup_url:150
[task 2018-03-06T17:23:26.772Z] 17:23:26     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-03-06T17:23:26.773Z] 17:23:26     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | Urlbar popup action color should be set to #008f8a - "rgb(240, 119, 70)" == "rgb(0, 143, 138)" - JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js :: test_popup_url :: line 155
[task 2018-03-06T17:23:26.774Z] 17:23:26     INFO - Stack trace:
[task 2018-03-06T17:23:26.774Z] 17:23:26     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:test_popup_url:155
[task 2018-03-06T17:23:26.775Z] 17:23:26     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js | brighttext should not be set! - "" == "" -

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

Looks like it's only failing on Linux. Makes sense since I only ran the test locally on Windows before pushing.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231386

::: toolkit/modules/LightweightThemeConsumer.jsm:202
(Diff revision 11)
>  }
> +
> +function _inferPopupColorsFromText(element, active, color) {
> +  if (!color) {
> +    element.removeAttribute("popup-brighttext");
> +    _setProperty(element, active, "--autocomplete-popup-secondary-color");

We should probably make up an internal theme property for this and let ThemeVariableMap specify the variable.

::: toolkit/modules/LightweightThemeConsumer.jsm:215
(Diff revision 11)
> +    element.removeAttribute("popup-brighttext");
> +  } else {
> +    element.setAttribute("popup-brighttext", "true");
> +  }
> +
> +  _setProperty(element, active, "--autocomplete-popup-secondary-color", `rgba(${r}, ${g}, ${b}, 0.4)`);

How did we arrive at 0.4? For accessibility I think we'd want 0.5 at least, maybe more.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231390

::: browser/themes/shared/urlbar-autocomplete.inc.css:15
(Diff revision 11)
>    --autocomplete-popup-highlight-background: Highlight;
>    --autocomplete-popup-highlight-color: HighlightText;
> +  --autocomplete-popup-secondary-color: GrayText;
> +}
> +
> +:root[popup-brighttext] {

Can we rename this to lwt-popup-brighttext to make it clear that this isn't available otherwise?

::: browser/themes/shared/urlbar-autocomplete.inc.css:18
(Diff revision 11)
> +}
> +
> +:root[popup-brighttext] {
> +  --urlbar-popup-url-color: #0a84ff;
> +  --urlbar-popup-action-color: #30e60b;
>  }

This seems to assume that -moz-fieldtext is always dark and will work on non-popup-brighttext themes, which isn't the case.

Comment 48

[Dão Gottwald [:dao]]

> > +:root[popup-brighttext] {
> > +  --urlbar-popup-url-color: #0a84ff;
> > +  --urlbar-popup-action-color: #30e60b;
> >  }
> 
> This seems to assume that -moz-fieldtext is always dark and will work on
> non-popup-brighttext themes, which isn't the case.

Err, I meant -moz-nativehyperlinktext, the --urlbar-popup-url-color default.

Comment 49

[Dylan Stokes [:dyl]]

Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219346/diff/11-12/

Comment 50

[Dylan Stokes [:dyl]]

Continuing our conversation on IRC, I should be adding an Internal theme variable _popup_secondary_text_generated to map to --autocomplete-popup-secondary-color in the ThemeVariableMap. I would then need to add a check in `ext-theme.js` to check for the _ prefix in a theme and ignore it so it can not be set by a theme. I am a bit confused on why we would want this variable in the first place?

Comment 51

[Dão Gottwald [:dao]]

(In reply to Dylan Stokes [:dyl] from comment #50)
> Continuing our conversation on IRC, I should be adding an Internal theme
> variable _popup_secondary_text_generated to map to
> --autocomplete-popup-secondary-color in the ThemeVariableMap. I would then
> need to add a check in `ext-theme.js` to check for the _ prefix in a theme
> and ignore it so it can not be set by a theme. I am a bit confused on why we
> would want this variable in the first place?

To keep things neat and separated, i.e. let ThemeVariableMap.jsm keep control over browser variables rather than baking part of that into an obscure private LightweightThemeConsumer.jsm function.

Comment 52

[Dylan Stokes [:dyl]]

(In reply to Dão Gottwald [::dao] from comment #51)
> (In reply to Dylan Stokes [:dyl] from comment #50)
> > Continuing our conversation on IRC, I should be adding an Internal theme
> > variable _popup_secondary_text_generated to map to
> > --autocomplete-popup-secondary-color in the ThemeVariableMap. I would then
> > need to add a check in `ext-theme.js` to check for the _ prefix in a theme
> > and ignore it so it can not be set by a theme. I am a bit confused on why we
> > would want this variable in the first place?
> 
> To keep things neat and separated, i.e. let ThemeVariableMap.jsm keep
> control over browser variables rather than baking part of that into an
> obscure private LightweightThemeConsumer.jsm function.

That totally makes sense and I agree. I am confused on where I would be adding the check to see if lwt-popup-brighttext is set and if so, use the new internal variable and set the secondary color to the popup color with alpha if I were to remove it from _inferPopupColorsFromText. I think I am not understanding how the ThemeVariableMap is working. Could you describe the process to me on what changes need to be added and how it works?

Comment 53

[Tim Nguyen :ntim]

Attachment: Bug 1417883 - Refactor theme code to have processing code in the variable map.

Review commit: https://reviewboard.mozilla.org/r/228532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228532/

Comment 54

[Tim Nguyen :ntim]

Dão, what do you think of this approach ? It doesn't introduce an internal map item, but it clearly separates toolkit code from browser code.

Comment 55

[Code Review Bot [:reviewbot]]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

https://reviewboard.mozilla.org/r/228532/#review234370


Code analysis found 2 defects in this patch:
 - 2 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


::: browser/modules/ThemeVariableMap.jsm:97
(Diff revision 1)
> +      } else {
> +        root.setAttribute("lwt-popup-brighttext", "true");
> +      }
> +
> +      root.style.setProperty(secondaryVariable, `rgba(${r}, ${g}, ${b}, 0.5)`);
> +      return color;

Error: Method 'processcolor' expected no return value. [eslint: consistent-return]

::: toolkit/modules/LightweightThemeConsumer.jsm:35
(Diff revision 1)
> +      }
> +      const [r, g, b] = parsedColor;
> +      const luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +      root.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");
> +      root.setAttribute("lwtheme", "true");
> +      return color || "black";

Error: Method 'processcolor' expected no return value. [eslint: consistent-return]

Comment 56

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #54)
> Dão, what do you think of this approach ? It doesn't introduce an internal
> map item, but it clearly separates toolkit code from browser code.

Interesting idea, but might be overkill? Something like _popup_secondary_text_generated could be computed in LightweightThemeConsumer, and then ThemeVariableMap could map that to --autocomplete-popup-secondary-color. Doing that all in ThemeVariableMap does allow for more app-specific flexibility going forward, but I'm not sure we'll need that.

Comment 57

[Tim Nguyen :ntim]

> Doing that all in ThemeVariableMap does allow for more app-specific flexibility going forward, but I'm not sure we'll need that.

I don't think it is overkill. This also prepares the work for bug 1418602, where we'll allow specifying "contexts" where a certain variable should be applied (in this case the sidebar frame), and potentially some more brighttext logic if needed. All of this being browser/ specific.

Dão, what do you think ?

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

https://reviewboard.mozilla.org/r/228532/#review236814

::: browser/modules/ThemeVariableMap.jsm:84
(Diff revision 1)
> +      const secondaryVariable = "--autocomplete-popup-secondary-color";
> +
> +      if (!color) {
> +        root.removeAttribute("lwt-popup-brighttext");
> +        root.style.removeProperty(secondaryVariable);
> +        return;

need to return a value here

::: toolkit/modules/LightweightThemeConsumer.jsm:29
(Diff revision 1)
> +    lwtProperty: "textcolor",
> +    processColor(root, color, parsedColor) {
> +      if (!color) {
> +        root.removeAttribute("lwthemetextcolor");
> +        root.removeAttribute("lwtheme");
> +        return;

need to return a value here

::: toolkit/modules/LightweightThemeConsumer.jsm:201
(Diff revision 1)
> -function _inferPopupColorsFromText(element, active, color) {
> -  if (!color) {
> -    element.removeAttribute("lwt-popup-brighttext");
> -    _setProperty(element, active, "--autocomplete-popup-secondary-color");
> -    return;
> +      let colorUsed = active && vars[lwtProperty];
> +      let color = colorUsed ? _sanitizeCSSColor(root.ownerDocument, vars[lwtProperty])
> +                            : null;
> +      if (processColor) {
> +        let parsedColor = colorUsed ? _parseRGB(color) : null;
> +        color = processColor(root, color, parsedColor);

It looks like the color argument is only used for null-checks, and the RGB array can be used for that just as well.

::: toolkit/modules/LightweightThemeConsumer.jsm:205
(Diff revision 1)
> -    return;
> +        let parsedColor = colorUsed ? _parseRGB(color) : null;
> +        color = processColor(root, color, parsedColor);
> -  }
> +      }
>  
> -  let [r, g, b] = _parseRGB(color);
> -  let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +      _setProperty(elem, active, cssVarName, color);
> +    }

This is a bit confusing to read. I'd suggest:

let val = vars[lwtProperty];
if (isColor) {
  ...
  val = ...;
}
_setProperty(elem, active, cssVarName, val);

Comment 59

[Tim Nguyen :ntim]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228532/diff/1-2/

Comment 60

[Tim Nguyen :ntim]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228532/diff/2-3/

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

https://reviewboard.mozilla.org/r/228532/#review236966

::: browser/modules/ThemeVariableMap.jsm:78
(Diff revision 3)
> +  ["--autocomplete-popup-background", {
> +    lwtProperty: "popup"
> +  }],
> +  ["--autocomplete-popup-color", {
> +    lwtProperty: "popup_text",
> +    processColor(root, parsedColor) {

Lets change the signature to:

    processColor(rgbaChannels, element)

whereas element is that from optionalElementID or the root element.

::: toolkit/modules/LightweightThemeConsumer.jsm:198
(Diff revision 3)
> +        if (active && val) {
> +          val = _sanitizeCSSColor(root.ownerDocument, val);
> -    }
> +        }
> +        if (processColor) {
> +          let parsedColor = _parseRGBA(val);
> +          val = processColor(root, parsedColor);

nit: get rid of the parsedColor variable

::: toolkit/modules/LightweightThemeConsumer.jsm:224
(Diff revision 3)
> -  _setProperty(element, active, "--autocomplete-popup-secondary-color", `rgba(${r}, ${g}, ${b}, 0.5)`);
> +function _parseRGBA(aColorString) {
> +  if (!aColorString) {
> +    return null;
> +  }
> +  var rgba = aColorString.replace(/(rgba?\()|(\)$)/g, "").split(",");
> +  return rgba.map(x => parseFloat(x));

Can you make this an {r, g, b, a} object instead of an array?

Comment 62

[Tim Nguyen :ntim]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228532/diff/3-4/

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

https://reviewboard.mozilla.org/r/228532/#review237008

::: toolkit/modules/LightweightThemeConsumer.jsm:179
(Diff revision 4)
>    } else {
>      elem.style.removeProperty(variableName);
>    }
>  }
>  
>  function _setProperties(root, active, vars) {

While you're at it, can you rename vars to themeData, themeProperties or something like that? The overloading of the variable term is confusing here.

::: toolkit/modules/LightweightThemeConsumer.jsm:194
(Diff revision 4)
>                                     : root;
> -      _setProperty(elem, active, cssVarName, vars[varsKey]);
> +
> +      let val = vars[lwtProperty];
> +      if (isColor) {
> +        if (active && val) {
> +          val = _sanitizeCSSColor(root.ownerDocument, val);

It looks like _sanitizeCSSColor should be called unconditionally so that _parseRGBA and processColor can't be confused by e.g. rgba(0,0,0,0). Either that or don't even pass the color to processColor in the first place if !active.

Comment 64

[Tim Nguyen :ntim]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228532/diff/4-5/

Comment 65

[Dão Gottwald [:dao]]

Comment on attachment 8959707 [details]
Bug 1417883 - Refactor theme code to have processing code in the variable map.

https://reviewboard.mozilla.org/r/228532/#review237022

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2151edc10e9e
Allow theming autocomplete popups. r=ntim, jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1fae6d6cca4
Refactor theme code to have processing code in the variable map. r=dao

Comment 67

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2151edc10e9e
https://hg.mozilla.org/mozilla-central/rev/a1fae6d6cca4

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Release Note Request (optional, but appreciated)
[Why is this notable]:

For users with Dark Themes, this is a pretty big change in some primary UI.

[Affects Firefox for Android]:

No.

[Suggested wording]:

Dark Themes now affect the URL bar and search dropdowns.

[Links (documentation, blog post, etc)]:

https://twitter.com/mike_conley/status/978991881337081856
MSU student website: http://www.capstone.cse.msu.edu/2018-01/projects/mozilla/
https://mail.mozilla.org/pipermail/firefox-dev/2018-March/006245.html

Comment 69

[Pascal Chevrel:pascalc]

Added to Nigh(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #68)
> Release Note Request (optional, but appreciated)

Added to Nightly release notes. Release Managers will decide on the potential inclusion to release notes for the final version for end-users so no change on the flag from me.

Comment 70

[Will Bamberg [:wbamberg]]

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#colors includes the new theme properties introduced here I think, with code samples and screenshots. But please let me know if you need anything else.

Usually I ask :ntim to check docs updates, but since he's apparently busy at the moment I'll ask you, but please let me know if you're not the right target for this :).

Comment 71

[Jared Wein [:jaws] (please needinfo? me)]

Looks good! Thanks!!