509671 - Remove icons from buttons due to Gnome changes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

[Jan Horak [:jhorak]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.2) Gecko/20090806 Fedora/3.5.2-3.fc12 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.2) Gecko/20090806 Fedora/3.5.2-3.fc12 Firefox/3.5.2

Gnome is going to remove icons from menus and buttons. 
See: http://bugzilla.gnome.org/show_bug.cgi?id=583352
There is no problem in menus, only with icons in buttons.

Do Mozilla plan to remove icons from buttons? For example in Quit confirmation dialog, Preferences, About dialog, Downloads, Page info, Clear recent history, etc.


Reproducible: Always

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Jan, can you provide a patch to honor the gtk settings?

Comment 2

[Jan Horak [:jhorak]]

Attachment: Initial patch

Patch is based on similar bug - https://bugzilla.mozilla.org/show_bug.cgi?id=504275. Please could you look into it?

Comment 3

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

Comment on attachment 404007 [details] [diff] [review]
Initial patch

Needs toolkit peer review

Comment 4

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

Is this supposed to apply to <xul:button type=menu|panel> as well? It currently does, as written, but I'm not sure that's desired.

Are you sure you want display: none rather than visibility: hidden? Looks like this element has a 2px right margin, so removing it completely could break alignment of the label within the button.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 404007 [details] [diff] [review]
Initial patch

Oops, I missed that Gavin already started reviewing this.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Is this supposed to apply to <xul:button type=menu|panel> as well? It currently
> does, as written, but I'm not sure that's desired.

Why should these buttons be different with regards to the icons?

> Are you sure you want display: none rather than visibility: hidden? Looks like
> this element has a 2px right margin, so removing it completely could break
> alignment of the label within the button.

visibility:hidden doesn't look right at all. I assume you mean visibility:collapse? That doesn't make a visible difference compared to display:none. The alignment looks fine to me. The 2px margin is there to prevent the image and the text from being glued together. This isn't needed if there's no image.

Comment 7

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

Comment on attachment 404007 [details] [diff] [review]
Initial patch

makes sense to me!

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3c034729c7de

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Shouldn't this patch have changed all the other platform nsLookAndFeel implementations to say that this metric is always true?  Otherwise, it seems like there's a significant danger of people writing the wrong thing if they ever write non-ifdef'd CSS.

Comment 11

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

Yes....

Comment 12

[Dão Gottwald [:dao]]

Well, not having images in buttons seems to be the standard on Windows and Mac.

Comment 13

[Phil Ringnalda (:philor)]

(In reply to comment #6)
> (In reply to comment #4)
> > Is this supposed to apply to <xul:button type=menu|panel> as well? It currently
> > does, as written, but I'm not sure that's desired.
> 
> Why should these buttons be different with regards to the icons?

Mmm, because the feed button in Firefox's location bar is a button type="menu" with no text, so this patch completely removed that UI on Linux?

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Is this supposed to apply to <xul:button type=menu|panel> as well? It currently
> > > does, as written, but I'm not sure that's desired.
> > 
> > Why should these buttons be different with regards to the icons?
> 
> Mmm, because the feed button in Firefox's location bar is a button type="menu"
> with no text, so this patch completely removed that UI on Linux?

This needs a Firefox-specific fix.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #12)
> Well, not having images in buttons seems to be the standard on Windows and Mac.

Then shouldn't this patch work by not having images the same way that Windows and Mac don't have images?  Then we wouldn't have all these Linux-specific problems that are likely to go unfixed for a while, and perhaps even undetected when new features land.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(I also heard complaints on IRC about this removing the Pause and Cancel icons in the download manager.)

Comment 17

[Phil Ringnalda (:philor)]

I'd also expect it to future-break some bookmarks manager search UI that I think is currently not enabled, once it's reenabled, which ought to be a fun bit of bustage, several years down the line.

But, basically, "you cannot use a button with just an image in the way that you've used them since the dawn of time, even though it works perfectly on your Mac, and works perfectly in your Windows VM, because for some Linux users that will be a blank button or maybe a total blank spot" sounds like just begging for failure. I don't think the xul.css hunk, that didn't get the review the top of the file says it's supposed to  have gotten, that broke XUL, was a good idea. Use the system metric a million times in gnomestripe where it's adding images to buttons, fine, but don't break all button elements just because some shouldn't have the images we add to them.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #15)
> (In reply to comment #12)
> > Well, not having images in buttons seems to be the standard on Windows and Mac.
> 
> Then shouldn't this patch work by not having images the same way that Windows
> and Mac don't have images?

We don't add icons in the first place on Windows and Mac. On Linux, we want to support the case where gtk-button-images is enabled.

Comment 19

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

(In reply to comment #17)
> I don't think the xul.css hunk, that didn't get the review the top
> of the file says it's supposed to  have gotten, that broke XUL, was a good
> idea.

What XUL did it break that isn't fixed by the patch for bug 521410?

I think enforcing the system setting in the toolkit has some value (automatically bringing in line themes that wouldn't otherwise take that system setting into consideration). Whether that's enough value to be worth compatibility costs is less clear, mostly because those are harder to measure - an answer to the question above would help.

Either way, I don't think there's any need to rush to get this in on the branch.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Either way, I don't think there's any need to rush to get this in on the
> branch.

Bug 521410 suggests that people are already using effected Gnome versions.

Comment 21

[Neil Deakin]

+%ifdef MOZ_WIDGET_GTK2
+/********* detection of system setting to use icons in buttons ***********/
+.button-icon:not(:-moz-system-metric(images-in-buttons)) {
+  display: none;
+}
+%endif
+

This will hide images on all buttons, whether they show system icons or not. It will also hide images for every theme. Neither of these are what you want.

We don't have proper image buttons in XUL currently. Until we do, we shouldn't be removing image support from buttons.

The right fix is to change toolkit/themes/gnomestripe/global/button.css where the images for the system icons are actually specified.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> +%ifdef MOZ_WIDGET_GTK2
> +/********* detection of system setting to use icons in buttons ***********/
> +.button-icon:not(:-moz-system-metric(images-in-buttons)) {
> +  display: none;
> +}
> +%endif
> +
> 
> This will hide images on all buttons, whether they show system icons or not.

We do want this. Whether it's a moz-icon or an icon that's part of the theme doesn't matter.

> It will also hide images for every theme. Neither of these are what you want.

Why should a theme by default overrule the system settings? If a theme wished to do so, it could still explicitly overrule xul.css.

Comment 23

[Neil Deakin]

So what exactly is the setting supposed to do? Turn off system icons, or any icon on any button? What about buttons that only have an icon?

Comment 24

[Dão Gottwald [:dao]]

It's supposed to affect any icon on any button, similar to images-in-menus from bug 415810. Buttons that only have an icon would be an exception that the patch in bug 521410 takes care of.

Comment 25

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

Comment on attachment 404007 [details] [diff] [review]
Initial patch

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think this patch should be backed out since it broke significant pieces of UI for some users.  And it certainly shouldn't be landed on branch without those problems fixed.

Comment 27

[Pascal Chevrel (relman team) -> :pascalc]

I think we lost RSS feed icons (and therefore access to the feature) since october on Linux trunk builds because of that (bug 536631), the check-in date of this patch corresponds to the regression window.

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Given that bug 521410 doesn't seem to be making any progress, I think this should get backed out until it does.  (Remember our plan about backing things out when the cause regressions instead of letting them sit?)

Comment 29

[Dão Gottwald [:dao]]

I don't see the point of backing this out. Bug 521410 is trivial, it's known what needs to happen, and it's blocking the next milestone.

Comment 30

[[e-mail]]

(In reply to comment #10)
> Shouldn't this patch have changed all the other platform nsLookAndFeel
> implementations to say that this metric is always true?  Otherwise, it seems
> like there's a significant danger of people writing the wrong thing if they
> ever write non-ifdef'd CSS.
I just fell foul of this. I wanted to write some CSS in a third-party theme and couldn't easily because the metric only ever returns true in GTK builds.