Closed Bug 1417883 Opened 6 years ago Closed 6 years ago

Allow theming autocomplete popups

Categories

(WebExtensions :: Themes, enhancement, P5)

enhancement

Tracking

(relnote-firefox 61+, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
relnote-firefox --- 61+
firefox61 --- fixed

People

(Reporter: ntim, Assigned: dyl, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature)

Attachments

(6 files)

The URLbar autocomplete popup should be themable.
Blocks: themingapi-more-ui
No longer blocks: themingapi
Depends on: 1297788
Priority: -- → P5
Depends on: 1421497
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?
Flags: needinfo?(shorlander)
Here is the current hover color for comparison.
As you can see, with the dark background, the hover state is not very visible.
No longer depends on: 1419818
Assignee: nobody → stokesdy
Status: NEW → ASSIGNED
Depends on: 1435019
Keywords: dev-doc-needed
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.
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.
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.
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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Component: WebExtensions: Frontend → WebExtensions: Themes
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! ;-)
*known base color
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.
Flags: needinfo?(mdeboer)
(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.
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.
Flags: needinfo?(mdeboer)
Mentor: mconley, jaws, ntim.bugs
(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?
Flags: needinfo?(shorlander)
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;
> }
(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).
Current review is a work in progress.
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]
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 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.
Attachment #8950080 - Flags: review?(jaws) → review-
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 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?
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
Blocks: 1408121
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!
Attachment #8950080 - Flags: review?(mconley)
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 ?
Attachment #8950080 - Flags: review?(mconley)
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 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
Attachment #8950080 - Flags: review?(ntim.bugs)
Attachment #8950080 - Flags: review?(ntim.bugs)
Attachment #8950080 - Flags: review?(ntim.bugs)
Attachment #8950080 - Flags: review?(jaws)
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
Attachment #8950080 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8950080 [details]
Bug 1417883 - Allow theming autocomplete popups.

https://reviewboard.mozilla.org/r/219346/#review231332
Attachment #8950080 - Flags: review?(jaws) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b03fd1004cac
Allow theming autocomplete popups. r=jaws,ntim
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! - "" == "" -
Flags: needinfo?(stokesdy)
Flags: needinfo?(jaws)
Looks like it's only failing on Linux. Makes sense since I only ran the test locally on Windows before pushing.
Flags: needinfo?(jaws)
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 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.
> > +: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.
Blocks: 1441708
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?
Flags: needinfo?(stokesdy) → needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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?
Flags: needinfo?(dao+bmo)
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 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]
(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.
Flags: needinfo?(dao+bmo)
> 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 ?
Flags: needinfo?(dao+bmo)
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);
Attachment #8959707 - Flags: review?(dao+bmo)
Flags: needinfo?(dao+bmo)
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?
Attachment #8959707 - Flags: review?(dao+bmo)
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.
Attachment #8959707 - Flags: review?(dao+bmo)
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
Attachment #8959707 - Flags: review?(dao+bmo) → review+
Pushed by ntim.bugs@gmail.com:
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
Depends on: 1449323
Depends on: 1449324
Depends on: 1449327
https://hg.mozilla.org/mozilla-central/rev/2151edc10e9e
https://hg.mozilla.org/mozilla-central/rev/a1fae6d6cca4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
relnote-firefox: --- → ?
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.
No longer depends on: 1449933
Depends on: 1453035
Depends on: 1460287
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 :).
Flags: needinfo?(jaws)
Looks good! Thanks!!
Flags: needinfo?(jaws)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: