Closed Bug 509671 Opened 11 years ago Closed 11 years ago

Remove icons from buttons due to Gnome changes

Categories

(Core :: Widget: Gtk, enhancement)

All
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: jhorak, Assigned: jhorak)

References

Details

Attachments

(1 file)

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
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
Version: unspecified → Trunk
Depends on: 509675
Jan, can you provide a patch to honor the gtk settings?
Attached patch Initial patchSplinter Review
Patch is based on similar bug - https://bugzilla.mozilla.org/show_bug.cgi?id=504275. Please could you look into it?
Attachment #404007 - Flags: review?
Attachment #404007 - Flags: review? → review?(roc)
Assignee: nobody → jhorak
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 404007 [details] [diff] [review]
Initial patch

Needs toolkit peer review
Attachment #404007 - Flags: review?(roc)
Attachment #404007 - Flags: review?(gavin.sharp)
Attachment #404007 - Flags: review+
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.
Attachment #404007 - Flags: review?(gavin.sharp) → review?(enndeakin)
Comment on attachment 404007 [details] [diff] [review]
Initial patch

Oops, I missed that Gavin already started reviewing this.
Attachment #404007 - Flags: review?(enndeakin) → review?(gavin.sharp)
(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 on attachment 404007 [details] [diff] [review]
Initial patch

makes sense to me!
Attachment #404007 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/3c034729c7de
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #404007 - Flags: approval1.9.2?
No longer depends on: 509675
Duplicate of this bug: 509675
Component: Themes → Widget: Gtk
Product: Toolkit → Core
QA Contact: themes → gtk
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.
Well, not having images in buttons seems to be the standard on Windows and Mac.
(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?
Depends on: 521410
(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.
(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.
(I also heard complaints on IRC about this removing the Pause and Cancel icons in the download manager.)
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.
(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.
(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.
(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.
+%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.
(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.
Depends on: 520988
No longer depends on: 520988
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?
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 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.
Attachment #404007 - Flags: approval1.9.2?
Attachment #404007 - Flags: approval1.9.2?
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.
Attachment #404007 - Flags: approval1.9.2?
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.
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?)
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.
Whiteboard: [needs landing]
(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.
You need to log in before you can comment on or make changes to this bug.