Hi, I've provided a spec to update the colouring of the search bar in Dark theme below. You can click on each element to get their values. Thanks! https://firefoxux.github.io/people/amlee/Dark-Theme-Search-Bar/

Hi, Just wanted to follow-up on this bug to see if this can be graduated from P3? I've had feedback asking if there is a bug to fix the contrast issues in Dark theme. Thanks!

Pushed changes to every part of the new spec _except_ for the background color of the autocomplete popup (#5 on the spec). Changing that background color is not possible unless the background color of all popups and doorhangers change, or Bug 1462456 is addressed.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review251570 ::: browser/themes/shared/compacttheme.inc.css:35 (Diff revision 2) > + --autocomplete-popup-background: #2A2A2E; > + --autocomplete-popup-highlight-background: #0060DF; > + --autocomplete-popup-color: var(--chrome-color); These have no effect, since they're overridden by the theme API (see LightweightThemeManager.addBuiltInTheme in nsBrowserGlue.js), which sets the popup variables. You can possibly add !important for those to apply ? ::: browser/themes/shared/searchbar.inc.css:27 (Diff revision 2) > +/* Dark theme search borders are darker than other > + panel separators, unlike light theme. */ > +:root[lwt-popup-brighttext] { > + --panel-separator-color: rgba(249,249,250,0.10); > +} Similar rules are already defined: https://searchfox.org/mozilla-central/source/browser/themes/linux/browser.css#53 https://searchfox.org/mozilla-central/source/browser/themes/osx/browser.css#44 https://searchfox.org/mozilla-central/source/browser/themes/windows/browser.css#75 Please replace those instead of defining a new rule. ::: browser/themes/shared/searchbar.inc.css:54 (Diff revision 2) > border-bottom: 1px solid var(--panel-separator-color); > } > > .search-panel-header { > font-weight: normal; > + font-size: 11px; Hardcoded font-sizes don't work well on multiple DPIs notably on Windows/Linux. I would personally just undo this change, as it's not part of the dark. ::: browser/themes/shared/searchbar.inc.css:62 (Diff revision 2) > +:root[lwt-popup-brighttext] .search-panel-header { > + color: var(--autocomplete-popup-highlight-color); > +} Hmm, That's not the intended use case of this variable. ::: browser/themes/shared/searchbar.inc.css:87 (Diff revision 2) > +:root[lwt-popup-brighttext] .search-panel-one-offs { > + border-top: 1px solid rgba(249,249,250,0.10); > +} Isn't this redundant with --panel-separator-color already being set to this value.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. Please address ntim's feedback and re-request review. Thanks!

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review251110 ::: browser/themes/shared/compacttheme.inc.css:35 (Diff revision 2) > + --autocomplete-popup-background: #2A2A2E; > + --autocomplete-popup-highlight-background: #0060DF; > + --autocomplete-popup-color: var(--chrome-color); I did a bit of testing, and it appears only --autocomplete-popup-background is overridden? The other two work without an !important. Wouldn't adding an !important to those ones override any custom theme the users installs? ::: browser/themes/shared/searchbar.inc.css:27 (Diff revision 2) > +/* Dark theme search borders are darker than other > + panel separators, unlike light theme. */ > +:root[lwt-popup-brighttext] { > + --panel-separator-color: rgba(249,249,250,0.10); > +} The intention here is to override those colours specifically in the searchbar context. Panel separators should remain the old colour in doorhangers and other popups, but need this new colour for the searchbar. Rather than setting :root[lwt-popup-brighttext]-specific rules for every searchbar element that makes use of --panel-separator-color, I'm overriding for the entire file here.

Comment on attachment 8979661 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245800/#review252078 This appears to be on top of the previous patch. Please fold these changes into one patch.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review252474 ::: browser/themes/shared/identity-block/identity-block.inc.css:117 (Diff revision 3) > list-style-image: url(chrome://browser/skin/search-glass.svg); > fill-opacity: .4; > } > > +:root[lwt-popup-brighttext] #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon { > + fill-opacity: .6; Why use [lwt-popup-brighttext] here? The identity icon isn't in a popup. ::: browser/themes/shared/searchbar.inc.css:31 (Diff revision 3) > > +/* Dark theme search borders are darker than other > + panel separators, unlike light theme. */ > +:root[lwt-popup-brighttext] { > + --panel-separator-color: rgba(249,249,250,0.10); > +} This will affect more than just the search bar, so let's not put this in this file. browser.inc.css is probably the right place for this. ::: browser/themes/shared/searchbar.inc.css:90 (Diff revision 3) > /* Bug 1108841: prevent font-size from affecting the layout */ > line-height: 0; > } > > +:root[lwt-popup-brighttext] .search-panel-one-offs { > + background-color: #4A4A4F; Seems like this will make the one off buttons inconsistent with other panels. Why are we not using --arrowpanel-dimmed here? ::: browser/themes/shared/searchbar.inc.css:264 (Diff revision 3) > -moz-context-properties: fill; > fill: currentColor; > } > + > +:root[lwt-popup-brighttext] .search-setting-button-compact > .button-box > .button-icon { > + fill: rgba(249,249,250,0.60); This seems like a rather arbitrary deviation from light themes. Can we just keep using currentColor here?

> The intention here is to override those colours specifically in the searchbar context. Panel separators should remain the old colour in doorhangers and other popups, but need this new colour for the searchbar. Rather than setting :root[lwt-popup-brighttext]-specific rules for every searchbar element that makes use of --panel-separator-color, I'm overriding for the entire file here. I think it would be fine to change the separator colors in the other panels too. The current colors in other panels never followed a particular UX spec.

(In reply to Dão Gottwald [::dao] from comment #11) > ::: browser/themes/shared/searchbar.inc.css:31 > (Diff revision 3) > > > > +/* Dark theme search borders are darker than other > > + panel separators, unlike light theme. */ > > +:root[lwt-popup-brighttext] { > > + --panel-separator-color: rgba(249,249,250,0.10); > > +} > > This will affect more than just the search bar, so let's not put this in > this file. browser.inc.css is probably the right place for this. Actually there are already rules for this in browser/themes/*/browser.css.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review252520 ::: browser/themes/shared/searchbar.inc.css:31 (Diff revision 3) > > +/* Dark theme search borders are darker than other > + panel separators, unlike light theme. */ > +:root[lwt-popup-brighttext] { > + --panel-separator-color: rgba(249,249,250,0.10); > +} I spoke to :amylee about this one, and the panel-separators on the elements in the dark theme search bar are intentionally a different color than all of the other panel-separators, like those that separate elements in doorhangers. The entire search header section at the bottom of the search bar is specced differently than other similar elements, hence the different background-colors for it and the search panel one-offs.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review252562 (In reply to Harry Twyford [:harry] from comment #14) > ::: browser/themes/shared/searchbar.inc.css:31 > (Diff revision 3) > > > > +/* Dark theme search borders are darker than other > > + panel separators, unlike light theme. */ > > +:root[lwt-popup-brighttext] { > > + --panel-separator-color: rgba(249,249,250,0.10); > > +} > > I spoke to :amylee about this one, and the panel-separators on the elements > in the dark theme search bar are intentionally a different color than all of > the other panel-separators, like those that separate elements in > doorhangers. The entire search header section at the bottom of the search > bar is specced differently than other similar elements, hence the different > background-colors for it and the search panel one-offs. The --panel-separator-color value we currently set for :root[lwt-popup-brighttext] is supposed to work on different dark backgrounds, it's not supposed to cater for any particular dark background specifically. If this doesn't work as expected, we should probably fix this by tweaking that value rather than overriding it here. Even if we really did want to use a different separator color here, the way you're doing it is wrong, because searchbar.css isn't scoped. It will affect every panel using --panel-separator-color in browser.xul.

Latest revision should make everything consistent. I spoke with :amylee about how changing the highlight color of autocomplete elements should also be reflected in other elements using --arrowpanel-dimmed. She recommended that they all be made darker, so this revision has that. The borders on the search one offs are also no longer overridden but are rather updated globally on dark theme, also recommnded by Amy. This will be reflected in borders on doorhangers and other popups. I've attached a screenshot of the new look of the search bar dropdown. It's close to the original spec, except the search panel one-offs use --arrowpanel-dimmed, on :dao's recommendation. I'm needinfo'ing Amy to get her thoughts. This revision makes sure all dark theme --arrowpanel elements are consistent (as they are in light theme) but have been updated to suit the new, darker, UX spec.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review252930 ::: browser/themes/linux/browser.css:28 (Diff revision 5) > --toolbarbutton-icon-fill-opacity: .85; > > --panel-separator-color: ThreeDShadow; > - --arrowpanel-dimmed: hsla(0,0%,80%,.3); > - --arrowpanel-dimmed-further: hsla(0,0%,80%,.45); > - --arrowpanel-dimmed-even-further: hsla(0,0%,80%,.8); > + --arrowpanel-dimmed: rgba(249,249,250,0.10); > + --arrowpanel-dimmed-further: rgba(249,249,250,0.15); > + --arrowpanel-dimmed-even-further: rgba(249,249,250,0.20); Changing these colors unconditionally like this breaks them on light backgrounds. ::: browser/themes/linux/browser.css:53 (Diff revision 5) > > --panel-separator-color: hsla(210,4%,10%,.14); > } > > :root[lwt-popup-brighttext] { > - --panel-separator-color: hsla(0,0%,80%,.25); > + --panel-separator-color: rgba(249,249,250,0.2); nit: drop the leading 0 from the alpha value for consistency ::: browser/themes/shared/compacttheme.inc.css:35 (Diff revision 5) > --lwt-toolbar-field-background-color: rgb(71, 71, 73); > + --lwt-toolbar-field-border-color: rgba(249, 249, 250, 0.2); > --lwt-toolbar-field-color: var(--chrome-color); > --urlbar-separator-color: #5F6670; > + > + /* !important to override LightweightThemeManager.addBuiltInTheme in nit: trailing space ::: browser/themes/shared/compacttheme.inc.css (Diff revision 5) > + --autocomplete-popup-color: var(--chrome-color); > } > > :root:-moz-lwtheme-darktext { > --lwt-toolbar-field-background-color: #fff; > - Why are you removing this line? ::: browser/themes/shared/identity-block/identity-block.inc.css:116 (Diff revision 5) > #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon { > list-style-image: url(chrome://browser/skin/search-glass.svg); > fill-opacity: .4; > } > > +:root:-moz-lwtheme-brighttext #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon { #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon:-moz-lwtheme-brighttext

(In reply to Harry Twyford [:harry] from comment #18) > Created attachment 8980639 [details] > New dark theme search bar - 25 May 2018 > > Latest revision should make everything consistent. > > I spoke with :amylee about how changing the highlight color of autocomplete > elements should also be reflected in other elements using > --arrowpanel-dimmed. She recommended that they all be made darker, so this > revision has that. The borders on the search one offs are also no longer > overridden but are rather updated globally on dark theme, also recommnded by > Amy. This will be reflected in borders on doorhangers and other popups. > > I've attached a screenshot of the new look of the search bar dropdown. It's > close to the original spec, except the search panel one-offs use > --arrowpanel-dimmed, on :dao's recommendation. I'm needinfo'ing Amy to get > her thoughts. > > This revision makes sure all dark theme --arrowpanel elements are consistent > (as they are in light theme) but have been updated to suit the new, darker, > UX spec. If this is consistent with the light theme version I'm good with it. Thanks!

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review253052 ::: browser/themes/shared/compacttheme.inc.css:38 (Diff revision 6) > --urlbar-separator-color: #5F6670; > + > + /* !important to override LightweightThemeManager.addBuiltInTheme in > + nsBrowserGlue.js */ > + --autocomplete-popup-background: #2A2A2E !important; > + --autocomplete-popup-highlight-background: #0060DF; nit: keep only one whitespace before #0060DF. ::: browser/themes/shared/identity-block/identity-block.inc.css:116 (Diff revision 6) > #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon { > list-style-image: url(chrome://browser/skin/search-glass.svg); > fill-opacity: .4; > } > > +#urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon:-moz-lwtheme-brighttext { :root[lwt-toolbar-field-brighttext] #identity-box > #identity-icon So it doesn't affect themes that have a dark overall UI, but a light UI for the urlbar. ::: browser/themes/shared/searchbar.inc.css:56 (Diff revision 6) > padding: 3px 6px; > color: var(--autocomplete-popup-secondary-color); > } > > +:root[lwt-popup-brighttext] .search-panel-header { > + color: var(--autocomplete-popup-color); I'm surprised this isn't already the default color for .search-panel-header.

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review253476 Can you please rebase this patch? Thanks. applying https://reviewboard-hg.mozilla.org/gecko/rev/4031eafc3ac68f4a95d8dd970abb6e5a9f07b0b6 patching file browser/themes/shared/searchbar.inc.css Hunk #1 FAILED at 46 1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/searchbar.inc.css.rej patching file browser/themes/shared/urlbar-autocomplete.inc.css Hunk #1 FAILED at 16 1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/urlbar-autocomplete.inc.css.rej abort: patch failed to apply

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review254788 ::: browser/themes/shared/compacttheme.inc.css:31 (Diff revision 7) > --chrome-selection-color: #fff; > --chrome-selection-background-color: #5675B9; > > /* Url and search bars */ > --lwt-toolbar-field-background-color: rgb(71, 71, 73); > + --lwt-toolbar-field-border-color: rgba(249, 249, 250, 0.2); How about setting toolbar_field_border in the addBuiltInTheme call instead? ::: browser/themes/shared/compacttheme.inc.css:39 (Diff revision 7) > + > + /* !important to override LightweightThemeManager.addBuiltInTheme in > + nsBrowserGlue.js */ > + --autocomplete-popup-background: #2A2A2E !important; > + --autocomplete-popup-highlight-background: #0060DF; > + --autocomplete-popup-color: var(--chrome-color); Aren't we using this color already? https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/browser/components/nsBrowserGlue.js#759 ::: browser/themes/shared/identity-block/identity-block.inc.css:117 (Diff revision 7) > list-style-image: url(chrome://browser/skin/search-glass.svg); > fill-opacity: .4; > } > > +:root[lwt-toolbar-field-brighttext] #urlbar[pageproxystate="invalid"] > #identity-box > #identity-icon { > + fill-opacity: .6; Actually, I'm not sure anymore this makes sense at all. Why would we want a different opacity here for all dark themes? Is this something specific to our own dark theme? Can you drop this change for now and maybe file a followup bug on figuring out what exactly we want here?

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. https://reviewboard.mozilla.org/r/245128/#review254804 Thanks!

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8885e98d44d4 Improvements to Dark Theme search bar. r=dao

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. Approval Request Comment [Feature/Bug causing the regression]: dark theme darkening [User impact if declined]: less polished dark theme search panels [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: see specification [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: no [Why is the change risky/not risky?]: css only [String changes made/needed]:

Comment on attachment 8977000 [details] Bug 1453722 - Improvements to Dark Theme search bar. Low-risk polish fix for the dark theme work shipping in Fx61. Approved for 61.0b12.

I managed to reproduce the issue on Windows 10x64, Ubuntu 16.04 x64 and Mac OS X 10.12 using Firefox Nightly 62.0a1 (2018-05-25). Verified fixed on Windows 10x64, Ubuntu 16.04 x64 and Mac OS X 10.12 using the latest Nightly 62.0a1(2018-06-06).

Verified fixed on Windows 10x64, Ubuntu 17.04 x64 and Mac OS X 10.13 using the latest Firefox Beta 61.0b12.