Closed
Bug 1417883
Opened 6 years ago
Closed 5 years ago
Allow theming autocomplete popups
Categories
(WebExtensions :: Themes, enhancement, P5)
WebExtensions
Themes
Tracking
(relnote-firefox 61+, firefox61 fixed)
RESOLVED
FIXED
mozilla61
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.
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Priority: -- → P5
Updated•5 years ago
|
Depends on: dark-theme-darkening
Updated•5 years ago
|
Blocks: dark-theme-darkening
No longer depends on: dark-theme-darkening
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Comment 2•5 years ago
|
||
Reporter | ||
Comment 3•5 years ago
|
||
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)
Reporter | ||
Comment 4•5 years ago
|
||
Here is the current hover color for comparison.
Reporter | ||
Comment 5•5 years ago
|
||
As you can see, with the dark background, the hover state is not very visible.
Updated•5 years ago
|
Assignee: nobody → stokesdy
Status: NEW → ASSIGNED
Reporter | ||
Updated•5 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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.
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(jaws)
Comment 10•5 years ago
|
||
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)
Updated•5 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment 11•5 years ago
|
||
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•5 years ago
|
||
*known base color
Reporter | ||
Comment 13•5 years ago
|
||
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)
Comment 14•5 years ago
|
||
(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•5 years ago
|
||
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)
Updated•5 years ago
|
Mentor: mconley, jaws, ntim.bugs
Comment 16•5 years ago
|
||
(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)
Comment 17•5 years ago
|
||
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•5 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 20•5 years ago
|
||
Current review is a work in progress.
Comment 21•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 23•5 years ago
|
||
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 hidden (mozreview-request) |
Comment 25•5 years ago
|
||
mozreview-review |
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-
Comment 26•5 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 30•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 32•5 years ago
|
||
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•5 years ago
|
||
mozreview-review |
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)
Reporter | ||
Comment 34•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8950080 -
Flags: review?(mconley)
Reporter | ||
Comment 36•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 38•5 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Updated•5 years ago
|
Attachment #8950080 -
Flags: review?(ntim.bugs)
Updated•5 years ago
|
Attachment #8950080 -
Flags: review?(ntim.bugs)
Attachment #8950080 -
Flags: review?(jaws)
Reporter | ||
Comment 40•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 42•5 years ago
|
||
mozreview-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+
Comment 43•5 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b03fd1004cac Allow theming autocomplete popups. r=jaws,ntim
Comment 44•5 years ago
|
||
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)
Comment 45•5 years ago
|
||
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 46•5 years ago
|
||
mozreview-review |
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•5 years ago
|
||
mozreview-review |
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•5 years ago
|
||
> > +: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 hidden (mozreview-request) |
Assignee | ||
Comment 50•5 years ago
|
||
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)
Comment 51•5 years ago
|
||
(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)
Assignee | ||
Comment 52•5 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 54•5 years ago
|
||
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•5 years ago
|
||
mozreview-review |
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•5 years ago
|
||
(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)
Reporter | ||
Comment 57•5 years ago
|
||
> 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 58•5 years ago
|
||
mozreview-review |
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)
Updated•5 years ago
|
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 61•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 63•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 65•5 years ago
|
||
mozreview-review |
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+
Comment 66•5 years ago
|
||
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
Comment 67•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2151edc10e9e https://hg.mozilla.org/mozilla-central/rev/a1fae6d6cca4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 68•5 years ago
|
||
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:
--- → ?
Comment 69•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 70•5 years ago
|
||
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)
Comment 71•5 years ago
|
||
Looks good! Thanks!!
Flags: needinfo?(jaws)
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•