Last Comment Bug 415810 - Respect the user's settings of icons in menus
: Respect the user's settings of icons in menus
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.9beta4
Assigned To: Michael Ventnor
:
Mentors:
Depends on: 564357
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-05 12:34 PST by Michael Ventnor
Modified: 2010-11-13 14:57 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.20 KB, patch)
2008-02-05 12:34 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 2 (8.03 KB, patch)
2008-02-06 15:27 PST, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Review
Patch 3 (8.02 KB, patch)
2008-02-06 15:51 PST, Michael Ventnor
dsicore: approval1.9+
Details | Diff | Review
Patch 4 (13.48 KB, patch)
2008-02-10 01:07 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 5 (8.52 KB, patch)
2008-02-10 13:28 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 6 (8.29 KB, patch)
2008-02-11 15:54 PST, Michael Ventnor
enndeakin: review+
mtschrep: approval1.9+
Details | Diff | Review

Description Michael Ventnor 2008-02-05 12:34:12 PST
Created attachment 301558 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2008-02-06 15:05:40 PST
How about getting this metric via gtk2drawing.c? Then we don't have to create any new widgets.
Comment 2 Michael Ventnor 2008-02-06 15:27:37 PST
Created attachment 301764 [details] [diff] [review]
Patch 2
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-06 15:46:00 PST
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 2008-02-06 15:51:35 PST
Created attachment 301769 [details] [diff] [review]
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) 2008-02-07 13:42:30 PST
Comment on attachment 301769 [details] [diff] [review]
Patch 3

a1.9+=damons
Comment 6 Dão Gottwald [:dao] 2008-02-07 16:43:56 PST
|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 2008-02-07 17:36:51 PST
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] 2008-02-07 17:59:31 PST
(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 2008-02-07 19:00:54 PST
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] 2008-02-08 01:03:56 PST
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] 2008-02-08 10:51:14 PST
Also, if you put it in xul.css with the appropriate ifdef, it would work for non-default themes.
Comment 12 Reed Loden [:reed] (use needinfo?) 2008-02-09 23:35:45 PST
Should I not land this due to Dão's comments?
Comment 13 Michael Ventnor 2008-02-09 23:38:28 PST
Land it now so users can get the benefit now, I'll file a followup bug for perf.
Comment 14 Reed Loden [:reed] (use needinfo?) 2008-02-09 23:57:30 PST
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 2008-02-10 00:00:05 PST
(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 2008-02-10 01:07:02 PST
Created attachment 302385 [details] [diff] [review]
Patch 4

I dunno, this approach seems a little messy in xul.css, unless I misunderstood what everyone wanted...
Comment 17 Neil Deakin 2008-02-10 05:29:12 PST
(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] 2008-02-10 07:49:44 PST
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] 2008-02-10 07:54:46 PST
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) (Exited; email my personal email if necessary) 2008-02-10 12:54:07 PST
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] 2008-02-10 12:59:47 PST
(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 2008-02-10 13:07:05 PST
Actually, IIRC display:none doesn't load its content. I think I now know of a rule to use thos.
Comment 23 Michael Ventnor 2008-02-10 13:16:15 PST
Hmm, turns out the images are also used for alignment due to their defined sizes, so that didn't work.
Comment 24 Michael Ventnor 2008-02-10 13:28:25 PST
Created attachment 302472 [details] [diff] [review]
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 2008-02-11 15:54:29 PST
Created attachment 302697 [details] [diff] [review]
Patch 6

I think this is the approach gavin wanted.
Comment 26 Michael Ventnor 2008-02-14 15:23:54 PST
Comment on attachment 302697 [details] [diff] [review]
Patch 6

Neil, can you take a look at this?
Comment 27 neil@parkwaycc.co.uk 2008-02-17 03:18:56 PST
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 2008-02-17 04:12:50 PST
(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] 2008-02-17 05:33:17 PST
(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 neil@parkwaycc.co.uk 2008-02-17 12:05:52 PST
Never mind me. Modern also respects OS scrollbar styles.
Comment 31 neil@parkwaycc.co.uk 2008-02-17 12:07:55 PST
Sorry for spamming, but what about menulists, e.g. in the helper app prefs?
Comment 32 Reed Loden [:reed] (use needinfo?) 2008-02-20 02:12:52 PST
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

Note You need to log in before you can comment on or make changes to this bug.