406883 - New style for Preferences, Add-ons, and PageInfo header

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Michael Monreal [:micmon]]

The Preferences, Addons and Page Info windows all use a similar header. GNOME normally does not use such a header, so there is no real native app to compare. But nevertheless I have done a few changes that improve the look of the header and make it look less alien:

1.) added top margin

2.) added left/right margins (align with boxes below)

3.) Removed hover feedback: GTK apps normally don't have this and we don't want to use a semi-transparent state of the icon.

3.) Improved selection: use selection color from theme. To make this look more interesting, also use the fading trick that the Addons dialog uses for selections (transparent gradient)

Also fixed is the notification area that sometimes pops up in the Addons dialog.

Comment 1

[Michael Monreal [:micmon]]

Attachment: Screenshot

This shows changed CSS applied to all three dialogs

Comment 2

[Michael Monreal [:micmon]]

Attachment: CSS

You can use this in userChrome.css to review the new look. Also take a look at bug 406879 which should fix the look of the permissions list in the page info window to also align with the new header.

Comment 3

[Michael Monreal [:micmon]]

One small addition: the theme preview area also needs to be put a bit further to the left:

#themePreviewArea {
  margin-right: 4px !important;
}

Comment 4

[Ian Spence]

This is actually an important fix, especially for dark gtk themes.

My only complaint would be the hover being set to white. While I don't have a suggestion for a better native color, it does clash for dark themes.

Comment 5

[Michael Monreal [:micmon]]

Ah, the "White" was just meant as a hack o get rid of the current hover color. In the real patch, the hover color should just be removed. The most similar GTK widget is the one used in Evolution's preferences. It's basicly the same as this bar, only that it is vertical. It also does not use a any hover feedback. I will attach a screenshot of this. 

Comment 6

[Michael Monreal [:micmon]]

Attachment: Evolution Preferences

Comment 7

[Michael Monreal [:micmon]]

Attachment: CSS (r2)

I have updated the CSS to look good with the latest firefox nightly. It also contains the fix from bug 406879 as well as other alignment fixes.

It's probably not easy to make a proper patch out of this but I really hope someone will find the time nevertheless.

Comment 9

[Michael Monreal [:micmon]]

Attachment: Small bug

There's also a small bug in those headers: the info dialog header is a bit higher than the preferences and addons headers. Expanding those to the same height should improve the look a bit, as the icons would not touch the top line anymore.

Comment 10

[u294409]

I hope it would land in trunk quickly.

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Michael, could you write a patch instead of a userchrome-like CSS ? You could then ask review to UI people. I can provide some help, should you need it.

Comment 12

[Michael Monreal [:micmon]]

Thanks _FrnchFrgg_... I would love to, but I fear I'm quite lost when it comes to the mozilla codebase and my time is too limited these days to dive into this, sorry. I hope someone else can pick up the pieces I left scattered around and do proper patches :/

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

OK, I'll try to do it. No guarantees, though.

Comment 15

[u294409]

_FrnchFrgg_: do it as good as possible :) .

Comment 16

[Michael Ventnor]

Attachment: Patch

This is my attempt, so forgive me if it is not accurate, but IMO this CSS looks the best.

Comment 17

[Michael Monreal [:micmon]]

Nicely done patch, from what I can see. The only thing that should still be fixed is the spacing and appearance of some dialog elements below the header (mostly in Addons and Page info) but I can file separate bugs for those if needed after this one has landed.

Comment 18

[u294409]

When it can land in trunk?

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> When it can land in trunk?

After it's been reviewed and approved for landing. I recommend you read http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree for information on how a patch gets into the tree.

Comment 20

[Julien "_FrnchFrgg_" RIVAUD]

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

To begin with, thanks ventnor. I was probably going to resign on this bug because I lack time, so your work is timely...


>+ * Contributor(s):
>+ *   Ben Goodger <[e-mail]>

You could credit yourself and Michael Montreal...


>+prefpane {
>+  padding-top: 8px;
>+  padding-bottom: 10px;
>+  -moz-padding-start: 8px;
>+  -moz-padding-end: 10px;
>+}

Why having a greater padding to the end ? I see the difference, and it hurts such a symetry maniac as me...
You may have a good reason that I don't see.

>+prefwindow[type="child"] {
>+  padding-top: 8px;
>+  padding-bottom: 10px;
>+  -moz-padding-start: 8px;
>+  -moz-padding-end: 10px;
>+}

idem

>+.prefWindow-dlgbuttons {
>+  padding-bottom: 10px;
>+  -moz-padding-start: 8px;
>+  -moz-padding-end: 10px;
>+}

idem

>+radio[pane] {
>+  -moz-appearance: none;
>+  min-width: 4.5em;
>+  margin: 0;
>+  padding: 2px;
>+}

Is 2px enough ? I'd go for a bit more space (especially vertical) around the icons, but it's not ugly as is.

>+.paneSelector {
>+  border: 1px solid ThreeDShadow;
>+  margin: 8px 8px 0 8px;
>+  padding: 0;
>+  background-color: -moz-Field;
>+  color: -moz-FieldText;
>+}

Why not using "-moz-appearance: listbox"; it would match better the role="listbox" attribute...

>+radio[pane]:hover {
>+  background-color: white;
>+  color: black;
>+}

Why is that here ? I thought that just removing it would be sufficient, since it then would use the non hover rule... Nevertheless it's wrong to use white and black, you should use -moz-Field and -moz-FieldText.


> /* View buttons */
> .viewSelector {
>-  border-bottom: 2px groove ThreeDFace;
>-  margin: 0px;
>-  -moz-padding-start: 10px;
>+  border: 1px solid ThreeDShadow;
>+  margin: 8px 8px 0 8px;
>+  padding: 0;
>   background-color: -moz-Field;
>   color: -moz-FieldText;
> }

As before, why not using -moz-appearance: listbox ?

> #viewGroup radio:hover {
>-  background-color: #E0E8F6;
>+  background-color: white;
>   color: black;
> }

Same thing as before on colors.

While you're at it, the pageinfo/permissions rich list (or what looks like a richlist) has not a -moz-appearance: listbox); could you change that ?

Comment 21

[Michael Ventnor]

Attachment: Patch 2

A lot of the prefwindow CSS rules were just copied and pasted from Winstripe, but I'll address those anyway.

Comment 22

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 298621 [details] [diff] [review]
Patch 2

Looks very good to me.

> /* Permissions Tab */
> #permList {
>+  -moz-appearance: listbox;
>   margin-top: .5em;
>   overflow: auto;
>   border: 2px solid;
>   -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
>   -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-left-colors: ThreeDShaédow ThreeDDarkShadow;
>   background-color: -moz-field;

You can then remove the borders and background colors (or I'll do it in bug 412307)

Perhaps I could set review+ and ask rflint for superreview ?

Comment 23

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 298621 [details] [diff] [review]
Patch 2

Looks very good to me.

> /* Permissions Tab */
> #permList {
>+  -moz-appearance: listbox;
>   margin-top: .5em;
>   overflow: auto;
>   border: 2px solid;
>   -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
>   -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-left-colors: ThreeDShaédow ThreeDDarkShadow;
>   background-color: -moz-field;

You can then remove the borders and background colors (or I'll do it in bug 412307)

Perhaps I could set review+ and ask rflint for superreview ?

Comment 24

[Julien "_FrnchFrgg_" RIVAUD]

(Sorry for the double post, but I think it's bugzilla's fault for this one)

Comment 25

[Michael Ventnor]

Bug 412307 is probably the best place since the two bugs have different goals.

Comment 26

[Julien "_FrnchFrgg_" RIVAUD]

Couldn't ryan have made a superreview ? I assumed he had the authority... Who should be asked ?

Comment 27

[Reed Loden [:reed]]

Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.29; previous revision: 1.28
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/preferences.css,v
done
Checking in toolkit/themes/gnomestripe/global/preferences.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/preferences.css,v  <--  preferences.css
initial revision: 1.1
done
Checking in toolkit/themes/gnomestripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.3; previous revision: 1.2
done
Checking in browser/themes/gnomestripe/browser/pageInfo.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/pageInfo.css,v  <--  pageInfo.css
new revision: 1.13; previous revision: 1.12
done

Comment 28

[u294409]

There's no text hover effect in preferences and addons manager.

Comment 29

[Daniel C]

I miss the hover effect in Preferences. I think it was quite useful feedback, and we should prefer usability over conformance to guidelines (which in this case are really rather loose anyway). The other changes are nice.

Comment 30

[u294409]

> The other changes are nice.

Yes, tangoified icons are very pretty :) .

Comment 31

[Michael Monreal [:micmon]]

(In reply to comment #29)
> I miss the hover effect in Preferences. I think it was quite useful feedback,
> and we should prefer usability over conformance to guidelines (which in this
> case are really rather loose anyway). The other changes are nice.
> 

We agreed not to use hover feedback here because of two things:

1.) As you pointed out, it's just uncommon for GTK/GNOME apps (as well as KDE3/4)to have such a state. The way we have it now, it works/looks the same as the selector used in many apps for preferences¹. 

2.) It would be hard to do a nice hover state. FF2 worked around this by drawing the normal state transparent, which means the icons actually look bad most of the time. Doing it properly would mean we would have to redo all of the icons.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=292235

Comment 32

[Ian Spence]

Attachment: Screenshot showing preference dialog

It should be noted that this "breaks" the preferences dialog in Thunderbird. It currently has some CSS that tries to make these headers look native-ish for Vista. These conflict, thus making things break (a lot of themes have the listbox background color white, and the thunderbird css makes the text white... so invisible text)

Now I'm not positive, but we might magically fix this with the landing of bug 412092, since the -moz-appearance would override most of that.  That being said, I'm trying to figure out some perf issues with that bug (perf is fine everywhere except the new url autocomplete, but I think I know a fix for that), so it could be a week of unusable preference windows for Thunderbird users

Comment 33

[Reed Loden [:reed]]

(In reply to comment #32)
> Created an attachment (id=299636) [details]
> Screenshot showing preference dialog
> 
> It should be noted that this "breaks" the preferences dialog in Thunderbird.

File a bug on this, please?

Comment 34

[Ian Spence]

Filed bug 414290

Comment 35

[Samuel LB]

I miss the hover feedback too, and I disagree that "GTK apps normally don't have this". Most GTK apps don't have this kind of control, but for toolbars, scrollbars, buttons and file icons they DO give hover feedback (and notebook controls/tab bars in the upcoming Ubuntu version 8.04). I know Evolution doesn't have hover feedback for this specific control, but that application doesn't follow GTK/GNOME guidelines too well (for example it uses button-like controls for the fields header, even though GTK has it's own control for that).

Also, having hover feedback improves usability because it shows that the user can interact with a control. So I think this specific part of the patch should be reverted.

Comment 36

[u294409]

I agree with Samuel, but I want to add, that "opacity" should not be reverted for hover :> .

Maybe bg[SELECTED] lightened a bit, but not opacity ^^ .

And about Evolution: list headers are using workaround in Clearlooks (if I remember good), but second inconsistent part is "New" button. We can list also third inconsistent element: background of "INBOX" header in mail component...

Evolution is gift from Ximian... From old epoch.

Evolution needs facelift, big reorganisation of non-comfortable interface.

Comment 37

[Michael Monreal [:micmon]]

(In reply to comment #35)
> Most GTK apps don't have this kind of control, but for toolbars,
> scrollbars, buttons and file icons they DO give hover feedback (and notebook
> controls/tab bars in the upcoming Ubuntu version 8.04). 
All but "file icons" (if you are talking about nautilus) are engine/theme features. GTK does not support this out of the box and the engine implementations are more or less hacks (that's why the official engines in gtk-engines don't do this). If GTK apps show hover feedback and Firefox does not, then Firefox' GTK theme emulation needs to be extended to also provide this.

> I know Evolution
> doesn't have hover feedback for this specific control, but that application
> doesn't follow GTK/GNOME guidelines too well (for example it uses button-like
> controls for the fields header, even though GTK has it's own control 
> for that).
This is not correct. The 'problem' is that Evolution uses a custom list view widget, which is perfectly fine. But themes have no way of knowing that they should use the list view look for this widget. So the theme needs to include a hint for this to work. The official GNOME theme (Clearlooks) does this for example. I wouldn't call this a hack. The real problem is in GTK and the way custom widget theming works.

Comment 38

[u294409]

[[ GTK does not support this out of the box and the engine
implementations are more or less hacks (that's why the official engines in
gtk-engines don't do this). ]]

No? nVidia's closed driver users can increase "Digital vibrance" to see hover effect on toolbar icons. In every engine from gtk-engines.

Comment 39

[Michael Monreal [:micmon]]

(In reply to comment #38)
> No? nVidia's closed driver users can increase "Digital vibrance" to see hover
> effect on toolbar icons. In every engine from gtk-engines.
> 

I don't know what you are talking about. The setting you are referring to is not even remotely connected to GTK. Don't make this kind of claims.

Comment 40

[u294409]

You really do not understand?

You can increase a setting of video card to notice hover effect in GTK, if it is hard to notice normally.

Comment 41

[Ian Spence]

While I don't see this as an important bug at all, it should be noted that GTK apparently has the ability to add these hover options.

http://developer.gimp.org/api/2.0/gtk/GtkStyle.html#gtk-style-render-icon

That function supports the idea that gtk images can be painted with states.
Anyone with the gtk source can check gtkstyle.c for the function gtk_default_render_icon to see the kinds of things that happen

Comment 42

[Michael Monreal [:micmon]]

Jakub, your words: "...to see hover effect on toolbar icons."

Sure the icon's color temperature changes, but it changes the same on a hovered icon as on a non-hovered icon (for engines that don't implement a hover change, which includes all of the official gtk-engines!).

If you where talking about the BUTTONS, this is already supported in firefox.

Comment 43

[u294409]

No. I'm talking about toolbars, mostly.

Comment 44

[Michael Monreal [:micmon]]

Jakub: toolbars still use (toolbar)buttons.

(In reply to comment #41)
> http://developer.gimp.org/api/2.0/gtk/GtkStyle.html#gtk-style-render-icon

Didn't know about that. But it makes sense. So what can we do now?

a) Preferences headers: there is a clear indication of which one is the selected one. Absolutely no reason to do any hover etc effects.

b) Button icons: should be handled desktop wide, e.g. by the engine. Note that buttons already have the highlight/pressed effects. No need to change anything here, too, as it would only mean a different look&feel from other desktop apps.

c) Special icons, like those in the url entry (go, rss, search..): I think those are problematic because they do not have a button or changing background, so a hover state would make sense here. Ian, can we get support for this in Firefox using the GTK functions?

Comment 45

[Ian Spence]

It's quite possible to do this, but it'd require some ugliness.

We'd need to choose one of two options.

1) Force every icon through moz-icon:// stock.
2) Implement -moz-appearance: NS_THEME_IMAGE and have gtkdrawing.c render each image

Considering these two options, (1) seems like the better option. By using the built in GTK icon architecture, we can register all of our icons [using the freedesktop spec for forwards compatibility].  Then, we can start doing things like moz-icon://stock/new-tab and things of that nature. This actually allows us to handle icons that were added later in the line without breaking backwards compatibility [see the debian/gtk-select-all bug].  It is then trivial to modify the code in nsIconChannel.cpp (I think that's the file, don't have access to source atm) to handle painting states.

This would only give us one more issue.  How do we pass the state of the image to the nsIconChannel code?  Currently we do things like

button[disabled="true"] img {
    list-style-image url("moz-icon://stock/foo?disabled=true");
}

That's fine for handling whether something is disabled, but we get to the point where handling each case makes the css cumbersome.

I actually mislead you when I suggested that was the last issue.  The final issue is that we would no longer be able to handle "sheets" of icons.  That is, using moz-image-rect or whatever it is to store multiple images in the same file.  I'd worry about performance, although we don't really have too many "icon sheets" nowadays anyway, other than pref page headers.