1037246 - Fix style of search engine dropdown in search field (toolbar) and about:newtab

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

The changes bring the search engine switcher style in New Tab into alignment with Firefox’s Australis styles (Project Chameleon) of in-content panels (in about:preferences) and list items that show additional items (such as the bookmarks toolbar item).

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Search Engine switcher in Australis style (Project Chameleon)

Comment 2

[Tim Nguyen :ntim]

Can we take the sidebars widget as reference ? With the items hoverand active states ?

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #2)
> Can we take the sidebars widget as reference ? With the items hoverand
> active states ?

Unfortunately, that widget is differently styled because it lives in the menu bar.  The small dropdown (http://cl.ly/image/0M1n1Y052e3Q) consistent with other menu widgets with long lists of items rather than just a few.  For in-content, rather than menu, widgets, about:preferences is a better guide.

Comment 4

[Tim Nguyen :ntim]

(In reply to Jennifer Morrow [:Boriss] (UX) from comment #3)
> (In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #2)
> > Can we take the sidebars widget as reference ? With the items hoverand
> > active states ?
> 
> Unfortunately, that widget is differently styled because it lives in the
> menu bar.  The small dropdown (http://cl.ly/image/0M1n1Y052e3Q) consistent
> with other menu widgets with long lists of items rather than just a few. 
> For in-content, rather than menu, widgets, about:preferences is a better
> guide.

I think the mixture of in-content and chrome looks quite strange to me. The whole reason the panels were made white is for them to fit on all platforms. With the blue dropdown items, it disrupts that goal. The blue dropdown items are rather appropriate for Windows 7/Vista or Mac OSX, but not at all for Windows. Gray is a more generic color that was chosen to fit on all platforms.
Also, the huge item size doesn't really fit into the UI, especially when everything else in the new tab page is small. (The doorhanger was designed as a small element (comparing to the chrome)).
Besides, Project Chameleon isn't done yet. Specs are likely to change soon.

Comment 5

[Matthew N. [:MattN]]

We should probably do the work to implement this behaviour and styling for <xul:menupopup>/<xul:panel> (e.g. with a class or in the case of xul:menupopup, maybe an element which inherits from xul:menupopup, or at least a drop-in stylesheet for a specific class) so we don't have to keep doing one-offs.

Note that UITour and Loop also implement this panel style too.

Comment 6

[Manish Goregaokar [:manishearth]]

I don't mind working on this once the design is decided. We'll need this design for bug 1044537 too.

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Patch

Would this work? Or should we try to unify the styles?

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8472849 [details] [diff] [review]
Patch

Review of attachment 8472849 [details] [diff] [review]:
-----------------------------------------------------------------

Note that the latest specs are here : bug 1040267 .

::: browser/base/content/newtab/newTab.css
@@ +365,5 @@
>    -moz-box-align: center;
> +  padding-top: 14px;
> +  padding-bottom: 14px;
> +  -moz-padding-start: 29px;
> +  -moz-padding-end: 29px;

Isn't it faster to use padding: 14px 29px; ?

@@ +377,5 @@
>  }
>  
> +#newtab-search-manage {
> +  background-color: #e8e9e9;
> +}

You should try to use the same CSS as the subview footers (with a top border, and hover and active states).
Here's the CSS for reference : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#763

Comment 10

[Sevaan Franks [:sevaan]]

Work has been done in Bug 1040267 to unify the search engine selector in about:newtab and the search box. This unified style is the one that should be implemented: https://bug1040267.bugzilla.mozilla.org/attachment.cgi?id=8470116

Comment 11

[Sevaan Franks [:sevaan]]

Attachment: Revised mockup of search engine selection dropdown.

Comment 12

[Madhava Enros [:madhava]]

OK.

This bug is now actually about reconciling the designs of the search selector in
- the chrome search field
- about:newtab

They should both use the design in Comment 11.

about:home does not currently have one of these, and should, but that can be another bug. Let's not delay in fixing the current UI inconsistency to add it to about:home.

Comment 13

[Manish Goregaokar [:manishearth]]

Attachment: WIP for newtab

Here's what I have till now. It applies to the search dropdown and customize dropdown on the newtab page (this would fix bug 1044537 too).

Looks like this: http://i.stack.imgur.com/jYnbv.png

Issues:
 - Need to add more space between the first entry and the top. Padding isn't working, might end up using first-child.
 - The images for some strange reason are 16x20 instead of 16x16. I confirmed this with the inspector, their height is 16px but the computed height is 20px.
 - I'm not sure how to reduce the font size of the manage text without it looking ugly. A different font may be needed.
 - The paddings and margins might be a bit off.

Thoughts and help? This is my first time implementing something from an image-spec, not completely sure how to go about it :)

Comment 14

[Sevaan Franks [:sevaan]]

Hi :manishearth,

That's a great start!

Regarding working off an image spec, your best bet for some quick wins here is to inspect the chrome and copy the various CSS stylings from there. Use the bookmarks or the download panel for inspiration.

A couple things to keep in mind/change (apart from the stuff you already mentioned):

- Is that font correct?
- The footer where it says "Manage Search Engines..." should look exactly like footer in the bookmarks/downloads panels
- Our doorhangers/panels have rounded corners and a dropshadow...inspecting the chrome should really help here.
- Is the background color of the dropdown body correct? Looks a little grey.
- The arrow should be the same colour as the dropdown body.

Here's an image to help make comparing doorhangers a little easier (I hope!). http://cl.ly/image/0q1U2L161l0Y

Comment 15

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

Hey manishearth - you might have an easier time if you inspect the chrome styling and either copy the rules, or find some way of generalizing them for use in the search dropdown as well.

Have you used DOM Inspector before to inspect panel styles?

Comment 16

[Manish Goregaokar [:manishearth]]

(In reply to Mike Conley (:mconley) from comment #15)
> Hey manishearth - you might have an easier time if you inspect the chrome
> styling and either copy the rules, or find some way of generalizing them for
> use in the search dropdown as well.

That's what I did, there's just some tweaks which I can't get right.

> Have you used DOM Inspector before to inspect panel styles?

DOM Inspector? The one with the shadow DOM? Not much, admittedly. I've just played a bit with it.

Comment 17

[Manish Goregaokar [:manishearth]]

(In reply to Sevaan Franks [:sevaan] from comment #14)
> Regarding working off an image spec, your best bet for some quick wins here
> is to inspect the chrome and copy the various CSS stylings from there. Use
> the bookmarks or the download panel for inspiration.
> 
> A couple things to keep in mind/change (apart from the stuff you already
> mentioned):
> 
> - Is that font correct?
I used `menu`, I'm not sure which I should use. (The bookmark dropdown uses the same)

I'll have a look at the rest when I get time. Thanks!

Comment 18

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

(In reply to Manish Goregaokar [:manishearth] from comment #16)
> (In reply to Mike Conley (:mconley) from comment #15)
> > Hey manishearth - you might have an easier time if you inspect the chrome
> > styling and either copy the rules, or find some way of generalizing them for
> > use in the search dropdown as well.
> 
> That's what I did, there's just some tweaks which I can't get right.
> 
> > Have you used DOM Inspector before to inspect panel styles?
> 
> DOM Inspector? The one with the shadow DOM? Not much, admittedly. I've just
> played a bit with it.

Yeah, that's the one. Here's what I suggest you do:

1) Install DOM Inspector: https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/

2) Restart your browser

2.1) Click on the downloads panel to preload it. Not sure if you still need to do this, but worth doing just in case.

3) Open DOM Inspector (Ctrl-Shift-I, or you can find it under Tools > Web Developer).

4) In the window that comes up, go to File > Inspect Chrome Document, and then choose the item with the title matching your browser window. It's probably the first item in that list.

5) Your DOM Inspector should look like this: https://www.dropbox.com/s/096b08bboilem03/Screenshot%202014-08-14%2015.10.25.png

6) In the "Document - DOM Nodes" section (bottom left pane), scroll to the bottom, and choose the "popupset", with no ID. It's probably third from the bottom. Unfold it. It should contain a single entry with the ID "downloadsPanel".

7) Select the downloadsPanel entry. Your DOM Inspector should look like this: https://www.dropbox.com/s/4rg6l9qbt2ko830/Screenshot%202014-08-14%2015.13.25.png

8) On the right side, under Attributes, right click on the bottom of the list where there's some whitespace, and choose "Insert" to add a new attribute.

9) Set Node Name to "noautohide" (no quotes) and Node Value to "true" (no quotes), and then press OK.

10) Now re-open the downloads panel. It should not close if you click elsewhere - it should stay open.

11) Back in DOM Inspector, above the Attributes list, there's a dropdown list. One of the items in that list is "CSS Rules". Choose it. https://www.dropbox.com/s/ku2t80ostus2q0m/Screenshot%202014-08-14%2015.15.23.png

12) You'll now be looking at the CSS rules for the panel. You can also go back to the node tree and unfold down into the guts of the panel, inspecting the style of each node. You can tweak the style as well to experiment.

13) You're probably interested in the downloadsFooter: https://www.dropbox.com/s/mw45fs63m76njdb/Screenshot%202014-08-14%2015.17.17.png - but explore around for more context.

That should give you some insight on how we're doing this stuff. Ping me in IRC if you have any questions.

Comment 19

[Sevaan Franks [:sevaan]]

This is super helpful. Thanks, :mconley!

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8473149 [details] [diff] [review]
WIP for newtab

Review of attachment 8473149 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a good start :)
I'll investigate the issues you mentioned later today :)

::: browser/base/content/newtab/newTab.css
@@ +355,5 @@
>  #newtab-search-panel .panel-arrowcontent {
>    -moz-padding-start: 0;
>    -moz-padding-end: 0;
>    padding-top: 0;
>    padding-bottom: 0;

Can this set of padding be combined ?

@@ +363,5 @@
> +#newtab-customize-panel,
> +#newtab-search-panel {
> +  padding-top: 10px;
> +  padding-bottom: 3px;
> +}

Did you want to apply the styling on the panel itself ?
If so, you should change the previous properties (that apply on the .panel-arrowcontent child)

@@ +368,3 @@
>  .newtab-customize-panel-item,
>  .newtab-search-panel-engine {
> +  padding: 2px 6px 2px 29px;

Not sure why you're putting a 29px left padding here ?
This will break in rtl. (the previous patch had the same padding on both sides, which is why you could combine them).

@@ +399,5 @@
> +}
> +
> +#newtab-search-manage:hover {
> +  background-color: hsla(210,4%,10%,.15);
> +  border-top: 1px solid hsla(210,4%,10%,.14);

For the hover state, I think you can just override the border-color, rather than re-mentioning the width and the style.

Also see here for the active state : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#777

@@ +406,5 @@
>  .newtab-customize-panel-item > label,
>  .newtab-search-panel-engine > label {
>    -moz-padding-start: 0;
>    -moz-margin-start: 0;
> +  color: rgb(0, 0, 0);

You don't need to override the colors here. The right colors are already defined in : toolkit/themes/(windows|osx)/global/popup.css
(I'll file a bug for updating colors on Linux).

@@ +423,1 @@
>  

I think there should be an active state too.
See : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#752

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8473149 [details] [diff] [review]
WIP for newtab

Review of attachment 8473149 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/newTab.css
@@ +356,5 @@
>    -moz-padding-start: 0;
>    -moz-padding-end: 0;
>    padding-top: 0;
>    padding-bottom: 0;
>    background: rgb(248, 250, 251);

This background can be removed too. It's already defined at toolkit level (with the correct colors).

Comment 23

[:Gavin Sharp [email: [e-mail]]]

This is related to bug 1029889 (at least one of the implementation approaches here probably conflicts with this patch).

Comment 24

[Thomas Stache]

This bug's definition is affected by the new Flare search implementation, maybe even obsolete.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Yes, I think we've stepped over this with the new search UI - sorry about that, but thanks for the contributions!