Closed Bug 1297788 Opened 8 years ago Closed 7 years ago

Move general autocomplete styles to autocomplete.inc.css.

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fix-optional

People

(Reporter: rick3162, Assigned: ntim)

References

Details

Attachments

(4 files, 5 obsolete files)

Combined screenshots.jpg
242.85 KB, image/jpeg
Details
Dark Awesome Bar Results
222.32 KB, image/png
Details
Bug 1297788 - Move general autocomplete styles to autocomplete.inc.css.
59 bytes, text/x-review-board-request
adw
: review+
ntim
: checkin+
Details
bug1297788-contrast.png
88.44 KB, image/png
Details 
In Firefox Developer edition and Nightly,
if you switch to the 'Developer edition' theme (in about:addons|Appearance) and switch to the dark theme in the DevTools, then the awesomebar gets the 'dark' theme.
But, when you start typing into the awesomebar, and Firefox displays a drop-down list of suggested sites, well, this list still has the "light" theme: (see screenshot/upper part).

So, my suggestion is the dark theme to apply to the drop-down list of suggested sites in awesomebar, too.


For reference I have this userstyle (written with help from here [1])
-------------------------------------------------------------
@-moz-document url(chrome://browser/content/browser.xul) {
    .autocomplete-richlistbox {
        -moz-box-orient: horizontal;
        overflow: hidden;
        background-color: #171b1f !important;
    }
    .ac-title-text {
        color: #F5F7FA !important;
    }
}
-------------------------------------------------------------
which results in: (see screenshot/lower part).


[1] https://forum.userstyles.org/discussion/51121/firefox-awesomebar-rule-for-the-border-color-of-the-drop-down-list-of-suggested-sites?new=1

PS. Both screenshot are from Nightly 51 x64 in win 10 x64 using Stylish 2.0.7.
Assignee: nobody → rjesup
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: rjesup → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [fxsearch]
Component: Location Bar → Theme
Priority: P3 → --
Whiteboard: [fxsearch]
Blocks: 1090548
Summary: (For Nightly and Developer Edition) The dark theme in awesomebar to apply to the drop-down list of suggested sites, too → Dark compact theme should also apply to the location bar's drop-down list of suggested sites
The hamburger menu is another example of a UI element that the Compact Dark theme doesn't seem to apply to.
I currently use the following style:
(I improved the initial one using the 'Custom buttons' addon with 'Attributes Inspector' button, in order to find the relevant selectors).
I find it works great.

Notes:
- in the 1st rule there's also an alternative declaration, aimed for FF57+, where the background color of awesomebar is now grey, not dark grey/black anymore)
- the last rule is because I use it with this userstyle https://userstyles.org/styles/131235/firefox-autocomplete-prioritize-url (which moves the URL to be in front of the page title). 
It could also be used when bug #1280700 is fixed, i.e. when an extra option is implemented in FF, to offer displaying URL before page title.


----------------------------------------------------------------------------------------
@namespace url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

@-moz-document url(chrome://browser/content/browser.xul) {

    #PopupAutoCompleteRichResult > richlistbox {
    	 -moz-box-orient: horizontal;
    	overflow: hidden;
    	background-color: #171b1f !important;	 	/* dark grey-black */
    	/* background-color: #474749 !important;	/* grey, for FF 57+ */
    }

    #PopupAutoCompleteRichResult > richlistbox > richlistitem > hbox > description > description:not(.ac-url-text) {
        color: #4a99e8 !important;  			/* blue */
    }

    #PopupAutoCompleteRichResult > richlistbox > richlistitem[selected] {
        background-color: #0a335c !important;		/* dark blue */
    }

    /* For Prioritizing URL */
    .ac-url-text, .ac-action-text {
        color: #e6e6e6 !important;  			/* light grey/white */
    }


}
----------------------------------------------------------------------------------------
Assignee: nobody → rick3162
Mentor: jaws, ntim.bugs
Status: NEW → ASSIGNED
Attachment #8921631 - Attachment is obsolete: true
Comment on attachment 8921630 [details]
Bug 1297788 - Dark compact theme should also apply to the location bar's drop-down list of suggested sites

https://reviewboard.mozilla.org/r/192624/#review198276

When I tried using this with the Space Fantasy theme the selected item in the aweseombar dropdown is not visible (white on white).

This is because we only set these CSS variables for compact themes (dark and light). In /browser/base/content/browser.css we should probably default these values to something. @ntim, what do you think?
Attachment #8921630 - Flags: review-
For question in comment #9.
Flags: needinfo?(ntim.bugs)
> In /browser/base/content/browser.css we should probably default these values to something. @ntim, what do you think?

browser/base/content/browser.css doesn't seem like the right place to put this, the content files should only contain content/layout related CSS. browser/themes/shared/autocomplete.inc.css also seems wrong, because that file is used for the web content autocomplete. browser/themes/shared/urlbar-autocomplete.inc.css seems like the most appropriate place for this rule.

Here's what I would suggest: at [0], you could add something like:

```css
:root {
  --urlbar-popup-background: -moz-field;
  --urlbar-popup-color: -moz-fieldtext;
  --urlbar-popup-highlight-background: Highlight;
  --urlbar-popup-highlight-color: HighlightText;
}
```
(the -moz-field and -moz-fieldtext values are from autocomplete.css in toolkit/themes)

In that same file, you can then add a rule that uses the popup-background and popup-color variables and edit the last rule in the file to use the highlight-background and highlight-color variables.

Once you've done that, you can override those variables in [1].

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/urlbar-autocomplete.inc.css
[1]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/compacttheme.inc.css#9
Flags: needinfo?(ntim.bugs)
Sorry Tim, but I don't quite understand what you mean in your review/suggestion.  

Ok, initially, I should add the following to the end of the urlbar-autocomplete.inc.css :

css
:root {
  --urlbar-popup-background: -moz-field;
  --urlbar-popup-color: -moz-fieldtext;
  --urlbar-popup-highlight-background: Highlight;
  --urlbar-popup-highlight-color: HighlightText;
}


But, then, what?

I mean, the 4 rules in the patch consisted of: 
- 2 rules with property `color` and 
- 2 rules with property `background-color`.

But, you speak of:
> add a rule that uses the popup-background and popup-color variables.
i.e. of a single (1) rule (containing both above two variables??),  
while the patch had 2+2 rules. How can this be done?


Then you say to:
> edit the last rule in the file to use the highlight-background and highlight-color variables.
For reference, the last rule in the file is:

.autocomplete-richlistitem[selected],
treechildren.searchbar-treebody::-moz-tree-row(selected) {
  background-color: Highlight;
  color: HighlightText;
}

So, ok, if I understand correctly, this must be converted into:

.autocomplete-richlistitem[selected],
treechildren.searchbar-treebody::-moz-tree-row(selected) {
  background-color: var(--highlight-background);
  color: var(--highlight-color);
}



Lastly you say that:
> you can override those variables in compacttheme.inc.css#9.

But, what is to override? 
in other words, which of the 4 rules (in the submitted patch) are to be put to the urlbar-autocomplete.inc.css,  
and which, afterwards, to override in compacttheme.inc.css#9 ?
Flags: needinfo?(ntim.bugs)
(In reply to Kostas from comment #12)
> Sorry Tim, but I don't quite understand what you mean in your
> review/suggestion.  
> 
> Ok, initially, I should add the following to the end of the
> urlbar-autocomplete.inc.css :
> 
> css
> :root {
>   --urlbar-popup-background: -moz-field;
>   --urlbar-popup-color: -moz-fieldtext;
>   --urlbar-popup-highlight-background: Highlight;
>   --urlbar-popup-highlight-color: HighlightText;
> }
> 
> 
> But, then, what?
> 
> I mean, the 4 rules in the patch consisted of: 
> - 2 rules with property `color` and 
> - 2 rules with property `background-color`.
> 
> But, you speak of:
> > add a rule that uses the popup-background and popup-color variables.
> i.e. of a single (1) rule (containing both above two variables??),  
> while the patch had 2+2 rules. How can this be done?

I'm not sure why 4 rules are needed, as adding this CSS should work fine:

.autocomplete-richlistbox {
  background-color: var(--urlbar-popup-background);
  color: var(--urlbar-popup-color);
}

> Then you say to:
> > edit the last rule in the file to use the highlight-background and highlight-color variables.
> For reference, the last rule in the file is:
> 
> .autocomplete-richlistitem[selected],
> treechildren.searchbar-treebody::-moz-tree-row(selected) {
>   background-color: Highlight;
>   color: HighlightText;
> }
> 
> So, ok, if I understand correctly, this must be converted into:
> 
> .autocomplete-richlistitem[selected],
> treechildren.searchbar-treebody::-moz-tree-row(selected) {
>   background-color: var(--highlight-background);
>   color: var(--highlight-color);
> }
Yes, but with --urlbar-popup-highlight instead of --highlight.


> and which, afterwards, to override in compacttheme.inc.css#9 ?

Only the variables need to be overriden in compacttheme.inc.css, so something like this:

--urlbar-popup-background: var(--url-and-searchbar-background-color);
--urlbar-popup-color: var(--chrome-color);
--urlbar-popup-highlight-background: var(--chrome-selection-background-color);
--urlbar-popup-highlight-color: var(--chrome-selection-color);


inside the :root:-moz-lwtheme rule in compacttheme.inc.css#9


Let me know if you need extra information :)
Flags: needinfo?(ntim.bugs)
The 4 variable overrides in compacttheme.inc.css correspond to your 4 original rules :)
Ok, thank you.

So, I tested your complete suggestion, 
i.e. replaced the last rule of urlbar-autocomplete.inc.css with:

:root {
  --urlbar-popup-background: -moz-field;
  --urlbar-popup-color: -moz-fieldtext;
  --urlbar-popup-highlight-background: Highlight;
  --urlbar-popup-highlight-color: HighlightText;
}

.autocomplete-richlistitem[selected],
treechildren.searchbar-treebody::-moz-tree-row(selected) {
  background-color: var(--urlbar-popup-highlight-background);
  color: var(--urlbar-popup-highlight-color);
}

.autocomplete-richlistbox {
  background-color: var(--urlbar-popup-background);
  color: var(--urlbar-popup-color);
}



and added the following inside the :root:-moz-lwtheme rule in compacttheme.inc.css#9 :

  --urlbar-popup-background: var(--url-and-searchbar-background-color);
  --urlbar-popup-color: var(--chrome-color);
  --urlbar-popup-highlight-background: var(--chrome-selection-background-color);
  --urlbar-popup-highlight-color: var(--chrome-selection-color);



but there are two problems now (for reference, these didn't occur with the 4 rules in the patch) :

- the title entries (all except the selected one) now have black text color (with the patch it was white, i.e. as expected),
- the gray background color now also applies to the saved password dropdown that appears when you doubleclick on a login form field, that you have saved your credentials for.

I also installed the themes "Space Fantasy by MaDonna" and "Starry Space Fantasy" (I don't know exactly which theme Jared referred to, above), and didn't notice any problem now.
I was referring to the Space Fantasy theme that is listed as a pre-installed recommended theme in the menu found at the bottom of Customize mode.
Alright, Jared. (I installed it ok via the Customize mode, but for your reference, it's not in AMO anymore, its homepage no longer exist: https://addons.mozilla.org/en-US/firefox/addon/space-fantasy/ )

Anyway, just tested it too, and no problem with that, as well.
(In reply to Kostas from comment #15)
> - the title entries (all except the selected one) now have black text color
> (with the patch it was white, i.e. as expected),
> - the gray background color now also applies to the saved password dropdown
> that appears when you doubleclick on a login form field, that you have saved
> your credentials for.
> 
> I also installed the themes "Space Fantasy by MaDonna" and "Starry Space
> Fantasy" (I don't know exactly which theme Jared referred to, above), and
> didn't notice any problem now.


Can you please share your new patch so I can check what's going on ?
Thanks!

I see what's going one, the rules in urlbar-autocomplete.inc.css incorrectly apply to all autocomplete popups (even before this patch).

What makes sense to do here is to clean up urlbar-autocomplete.inc.css to exclusively contain urlbar autocomplete rules, and move the other rules to autocomplete.inc.css. Kostas, are you comfortable of doing this ? I can finish the patch (with your name as commit author) if you want.
> Kostas, are you comfortable of doing this ? I can finish the patch (with your name as commit author) if you want.

I wouldn't say I really am.
So, please, yes, I'd prefer if you finished the patch. Thanks a lot!
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Stephen, can you test out this build with a dark theme? The awesomebar popup
> will now use the dark theme colors.

Looking good! Noticed a few issues:

1) With a new Profile the Search hint isn't dark
2) The Search engine footer is not dark
3) Looks like some contrast issues for links. We should use Blue-50 http://design.firefox.com/photon/visuals/color.html#blue

Thanks!
Flags: needinfo?(shorlander)
Tim, have you been able to come back to this?
Flags: needinfo?(ntim.bugs)
Attachment #8921630 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8929081 - Flags: review?(jaws) → review?(adw)
Comment on attachment 8929082 [details]
Bug 1297788 - Make urlbar autocomplete style match photon light/dark themes.

https://reviewboard.mozilla.org/r/200374/#review205548
Attachment #8929082 - Flags: review?(jaws) → review+
Comment on attachment 8929083 [details]
Bug 1297788 - Use photon colors for autocomplete popup.

https://reviewboard.mozilla.org/r/200376/#review205550

We should get UX sign-off on these color tweaks.
Attachment #8929083 - Flags: review?(jaws) → review+
Stephen, can you take a look at the colors of the awesomebar popup with the Dark theme enabled. They're very slightly tweaked to use the current Photon colors that are used elsewhere.

OSX Build: https://queue.taskcluster.net/v1/task/ZiY9eJ0LTf-fvXv0L3XvYg/runs/0/artifacts/public/build/target.dmg
Forgot to add needinfo for comment #29 ^
Flags: needinfo?(shorlander)
Blocks: 1417883
Comment on attachment 8929081 [details]
Bug 1297788 - Move general autocomplete styles to autocomplete.inc.css.

https://reviewboard.mozilla.org/r/200372/#review206158

LGTM.  I think it does make sense to move the .autocomplete-richlistitem rules from urlbar-autocomplete.inc.css to autocomplete.inc.css since they aren't specific to the urlbar.
Attachment #8929081 - Flags: review?(adw) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29)
> Stephen, can you take a look at the colors of the awesomebar popup with the
> Dark theme enabled. They're very slightly tweaked to use the current Photon
> colors that are used elsewhere.
> 
> OSX Build:
> https://queue.taskcluster.net/v1/task/ZiY9eJ0LTf-fvXv0L3XvYg/runs/0/
> artifacts/public/build/target.dmg

This looks great thank you!

The only thing I noticed is that the link to preferences on the Search Hint bar has bad contrast: https://cl.ly/1j2k010i0C0J
Flags: needinfo?(shorlander)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/83a3d32d81e0
Move general autocomplete styles to autocomplete.inc.css. r=adw
https://hg.mozilla.org/integration/autoland/rev/c8d3ad173dbc
Make urlbar autocomplete style match photon light/dark themes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/5a9be669bd5c
Use photon colors for autocomplete popup. r=jaws
https://hg.mozilla.org/mozilla-central/rev/83a3d32d81e0
https://hg.mozilla.org/mozilla-central/rev/c8d3ad173dbc
https://hg.mozilla.org/mozilla-central/rev/5a9be669bd5c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/05a913c5cf3f
Backed out 2 changesets for fixing the commit message r=backout a=backout
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0e9d8fb67dd8
Make urlbar autocomplete style match photon light/dark themes "Original patch by Kostas (rick3162@gmail.com)". r=jaws
https://hg.mozilla.org/mozilla-central/rev/4affa6e0a8c6
Use photon colors for autocomplete popup "DONTBUILD". r=jaws a=reland
Attached image bug1297788-contrast.png — 
I have observed the patch in action today in Nightly Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20171122103138 and in general I like the idea having all UI dark (hopefully including the sidebar?). But at least on Linux the contrast is definitely not great at all. I have highlighted on a screenshot (sorry it's not as professional as from :shorlander).
Btw. also the search engine icons might be affected, as probably noone picked them with regard to the possibility of dark theme. This is something that the l10n communities should verify for themselves, I guess?
Fix the commit message
https://hg.mozilla.org/mozilla-central/rev/05a913c5cf3f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ][:mstanke] (use needinfo) from comment #40)
> Created attachment 8930999 [details]
> bug1297788-contrast.png

I filed bug 1419888 for this.
Depends on: 1419913
Depends on: 1419915
Depends on: 1419920
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Depends on: 1419818
See Also: 1419818 → 
This landed with quite a few aesthetical quirks, usability issues and inconsistencies. At this point I'm not sure we can expect all these issues to be resolved before the next uplift, so in order to keep up quality I'd like to back this out now rather than discovering at the end of the 59 cycle that a backout has become more complicated because other patches landed on top of this.

For those interested in working on this, this shouldn't make a big difference. You can work on these issues and then re-land the whole thing when ready. Jared, does this make sense or am I missing something?
Flags: needinfo?(jaws)
I have the light theme. After this patch, "Switch to tab" are shiny green and urls are shiny blue. These colors are so shiny different that both attract my attention so it's distracting because I can't focus on the one I want.
I've backed out the two last parts of this patch since I'm unlikely going to get to fixing all the regressions soon.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c841dbfc685301fe4d6163e841ec06f8139d8b94

Kostas, feel free to pick this up!
Flags: needinfo?(jaws)
Status: RESOLVED → REOPENED
status-firefox59: fixed → fix-optional
Resolution: FIXED → ---
Attachment #8929081 - Flags: checkin+
> Kostas, feel free to pick this up!
Tim, unfortunately, I don't feel confident doing this.
I initially wanted to do it, expecting that it would be just a simple addition (a variation of the userstyle I posted in comment #4) to a single CSS file.

But, based on the Dão Gottwald said, this has multiple issues and regressions that need to be fixed:
> This landed with quite a few aesthetical quirks, usability issues and inconsistencies.
i.e. fixing this requires multiple CSS files to be modified, in addition to the backed out patches.

But, unfortunately I'm not familiar with the Firefox's CSS files structure, and my CSS knowledge is limited. 


So, please assign this to some more experienced than me. Sorry.
Since the first patch in the set has stayed in mozilla-central we should rename this bug and close it as fixed. Tracking multiple landings in one bug can get very confusing, especially when they land many days or potentially weeks apart. We can use bug 1417883 to finish implementing this bug.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Summary: Dark compact theme should also apply to the location bar's drop-down list of suggested sites → Move general autocomplete styles to autocomplete.inc.css.
Assignee: rick3162 → ntim.bugs
Mentor: jaws, ntim.bugs
Attachment #8929082 - Attachment is obsolete: true
Attachment #8929083 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: