415810 - Respect the user's settings of icons in menus

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Michael Ventnor]

Attachment: Patch

Currently we display icons in menus no matter what. GTK does have a setting as to whether to display icons on menuitems or not, and we should honour that since "light" WM's like Fluxbox don't have icons in their menus.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about getting this metric via gtk2drawing.c? Then we don't have to create any new widgets.

Comment 2

[Michael Ventnor]

Attachment: Patch 2

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

+        aMetric = (moz_gtk_images_in_menus() ? 1 : 0);

Just return moz_gtk_images_in_menus() directly

Comment 4

[Michael Ventnor]

Attachment: Patch 3

This patch will allow further integration with the user's interface preferences without adding much risk at all.

Comment 5

[Damon Sicore (:damons)]

Comment on attachment 301769 [details] [diff] [review]
Patch 3

a1.9+=damons

Comment 6

[Dão Gottwald [:dao]]

|visibility: hidden| means that all images will still be loaded and processed. Instead, we shouldn't even use the menuitem-iconic binding in that case.

Comment 7

[Michael Ventnor]

Dao, I tried that but I think it still ended up using the iconic binding.

Either way, this method allows us to hide any and all icons on all menuitems and submenus, while still allowing proper margins.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Dao, I tried that but I think it still ended up using the iconic binding.

You think? :) Any chance to get that verified?
It's waste to load all icons for nothing.

> Either way, this method allows us to hide any and all icons on all menuitems
> and submenus, while still allowing proper margins.

The non-iconic binding should have proper margins, too. Otherwise that would be a bug.

Comment 9

[Michael Ventnor]

I'll probably file a follow-up bug on that, but this method ensures no icons get displayed on any menuitem. I can't seem to find a way from within CSS to remove every use of the iconic binding, just the ones I had set in Gnomestripe.

Comment 10

[Dão Gottwald [:dao]]

menuitem {
  -moz-binding: url(chrome://global/content/bindings/menu.xml#menuitem) !important;
}
menu.menu-iconic {
  -moz-binding: url(chrome://global/content/bindings/menu.xml#menu) !important;
}

Comment 11

[Dão Gottwald [:dao]]

Also, if you put it in xul.css with the appropriate ifdef, it would work for non-default themes.

Comment 12

[Reed Loden [:reed]]

Should I not land this due to Dão's comments?

Comment 13

[Michael Ventnor]

Land it now so users can get the benefit now, I'll file a followup bug for perf.

Comment 14

[Reed Loden [:reed]]

Sorry, but sayrer questions me on an almost daily basis about gtk/Linux perf issues, so if there's a known perf problem with this patch, I'd rather wait until there's a correct patch that fixes the problem and won't hurt perf. Don't want to be the bad guy here, but we don't need any more perf regressions.

Comment 15

[Robert Sayre]

(In reply to comment #13)
> Land it now so users can get the benefit now, I'll file a followup bug for
> perf.
> 

No...

Comment 16

[Michael Ventnor]

Attachment: Patch 4

I dunno, this approach seems a little messy in xul.css, unless I misunderstood what everyone wanted...

Comment 17

[Neil Deakin]

(In reply to comment #14)
> Sorry, but sayrer questions me on an almost daily basis about gtk/Linux perf
> issues,

This isn't a performance regression so a followup bug would be fine here. In fact, the newest patch could be one (although very negligible), as it adds more complexity to xul.css

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 302385 [details] [diff] [review]
Patch 4

If you use something along the lines of comment 10 in xul.css (with more intelligent selectors, of course), you shouldn't need to patch gnomestripe at all, and no third-party theme would need to be updated.

Comment 19

[Dão Gottwald [:dao]]

e.g.:

> %ifdef MOZ_WIDGET_GTK2
> menuitem:not([type]):not(:-moz-system-metric(images-in-menus)) {
>   -moz-binding: url(chrome://global/content/bindings/menu.xml#menuitem) !important;
> }
> menu.menu-iconic:not(:-moz-system-metric(images-in-menus)) {
>   -moz-binding: url(chrome://global/content/bindings/menu.xml#menu) !important;
> }
> %endif

This is untested and not necessarily complete.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should take patch 3. It's not going to hurt perf at all, in fact it will help perf a little bit for the small number of users it will affect. The performance "issue" is only that patch 3 might not help perf as much as it could ... but again, it's only going to affect a small minority of users so we shouldn't care much either way.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> It's not going to hurt perf at all, in fact it will
> help perf a little bit for the small number of users it will affect.

The perf win, if existing, would be near to nothing compared to what could be won if you just don't load the icons, wouldn't it?

Also, I don't see why this should be fixed within gnomestripe. This is filed as a Core bug to start with. Should every single theme explicitly support this?

Comment 22

[Michael Ventnor]

Actually, IIRC display:none doesn't load its content. I think I now know of a rule to use thos.

Comment 23

[Michael Ventnor]

Hmm, turns out the images are also used for alignment due to their defined sizes, so that didn't work.

Comment 24

[Michael Ventnor]

Attachment: Patch 5

The backend stuff has already gotten review from roc and it hasn't changed, so I'll just ask gavin to review the xul.css stuff.

Comment 25

[Michael Ventnor]

Attachment: Patch 6

I think this is the approach gavin wanted.

Comment 26

[Michael Ventnor]

Comment on attachment 302697 [details] [diff] [review]
Patch 6

Neil, can you take a look at this?

Comment 27

[[e-mail]]

Comment on attachment 302697 [details] [diff] [review]
Patch 6

>Index: toolkit/content/xul.css
What if I want to write a theme that has no intention of ever following OS settings? (OK, so Modern uses the OS font setting, but nothing else.)

Comment 28

[Michael Ventnor]

(In reply to comment #27)
> (From update of attachment 302697 [details] [diff] [review])
> >Index: toolkit/content/xul.css
> What if I want to write a theme that has no intention of ever following OS
> settings? (OK, so Modern uses the OS font setting, but nothing else.)
> 

(In reply to comment #18)
> (From update of attachment 302385 [details] [diff] [review])
> If you use something along the lines of comment 10 in xul.css (with more
> intelligent selectors, of course), you shouldn't need to patch gnomestripe at
> all, and no third-party theme would need to be updated.
> 

Something has to give...

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #27)
> (From update of attachment 302697 [details] [diff] [review])
> >Index: toolkit/content/xul.css
> What if I want to write a theme that has no intention of ever following OS
> settings?

You override these rules.
Respecting the user's settings should be the default, not the other way around.

Comment 30

[[e-mail]]

Never mind me. Modern also respects OS scrollbar styles.

Comment 31

[[e-mail]]

Sorry for spamming, but what about menulists, e.g. in the helper app prefs?

Comment 32

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.80; previous revision: 1.79
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.54; previous revision: 1.53
done
Checking in widget/src/gtk2/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--  nsILookAndFeel.h
new revision: 1.63; previous revision: 1.62
done
Checking in layout/style/nsCSSRuleProcessor.cpp;
/cvsroot/mozilla/layout/style/nsCSSRuleProcessor.cpp,v  <--  nsCSSRuleProcessor.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/content/xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.115; previous revision: 1.114
done