Respect the user's settings of icons in menus

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Widget: Gtk
RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: Michael Ventnor, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9beta4
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Attachment #301558 - Flags: superreview?(roc)
Attachment #301558 - Flags: review?(roc)
How about getting this metric via gtk2drawing.c? Then we don't have to create any new widgets.
(Assignee)

Comment 2

9 years ago
Created attachment 301764 [details] [diff] [review]
Patch 2
Assignee: nobody → ventnor.bugzilla
Attachment #301558 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #301764 - Flags: superreview?(roc)
Attachment #301764 - Flags: review?(roc)
Attachment #301558 - Flags: superreview?(roc)
Attachment #301558 - Flags: review?(roc)
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
Attachment #301764 - Flags: superreview?(roc)
Attachment #301764 - Flags: superreview+
Attachment #301764 - Flags: review?(roc)
Attachment #301764 - Flags: review+
(Assignee)

Comment 4

9 years ago
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.
Attachment #301764 - Attachment is obsolete: true
Attachment #301769 - Flags: approval1.9?
Version: unspecified → Trunk
Comment on attachment 301769 [details] [diff] [review]
Patch 3

a1.9+=damons
Attachment #301769 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
|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.
(Assignee)

Comment 7

9 years ago
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.
(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.
(Assignee)

Comment 9

9 years ago
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.
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;
}
Also, if you put it in xul.css with the appropriate ifdef, it would work for non-default themes.
Should I not land this due to Dão's comments?
(Assignee)

Comment 13

9 years ago
Land it now so users can get the benefit now, I'll file a followup bug for perf.
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.
Keywords: checkin-needed

Comment 15

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

No...
(Assignee)

Comment 16

9 years ago
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...
Attachment #301769 - Attachment is obsolete: true
Attachment #302385 - Flags: superreview?(roc)
Attachment #302385 - Flags: review?(roc)

Updated

9 years ago
Attachment #302385 - Flags: review?(gavin.sharp)

Comment 17

9 years ago
(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
Keywords: checkin-needed
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.
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.
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.
(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?
(Assignee)

Comment 22

9 years ago
Actually, IIRC display:none doesn't load its content. I think I now know of a rule to use thos.
(Assignee)

Comment 23

9 years ago
Hmm, turns out the images are also used for alignment due to their defined sizes, so that didn't work.
(Assignee)

Comment 24

9 years ago
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.
Attachment #302385 - Attachment is obsolete: true
Attachment #302472 - Flags: review?(gavin.sharp)
Attachment #302385 - Flags: superreview?(roc)
Attachment #302385 - Flags: review?(roc)
Attachment #302385 - Flags: review?(gavin.sharp)
(Assignee)

Comment 25

9 years ago
Created attachment 302697 [details] [diff] [review]
Patch 6

I think this is the approach gavin wanted.
Attachment #302472 - Attachment is obsolete: true
Attachment #302697 - Flags: review?(gavin.sharp)
Attachment #302472 - Flags: review?(gavin.sharp)
Keywords: checkin-needed
(Assignee)

Comment 26

9 years ago
Comment on attachment 302697 [details] [diff] [review]
Patch 6

Neil, can you take a look at this?
Attachment #302697 - Flags: review?(gavin.sharp) → review?(enndeakin)

Updated

9 years ago
Attachment #302697 - Flags: review?(enndeakin) → review+
Attachment #302697 - Flags: approval1.9?
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.)
(Assignee)

Comment 28

9 years ago
(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...
(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.
Never mind me. Modern also respects OS scrollbar styles.
Sorry for spamming, but what about menulists, e.g. in the helper app prefs?

Updated

9 years ago
Attachment #302697 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4

Updated

7 years ago
Depends on: 564357

Updated

8 months ago
Depends on: 1302157
You need to log in before you can comment on or make changes to this bug.