Missing icons with gtk+-2.22

VERIFIED FIXED in mozilla2.0b12

Status

()

defect
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: pachoramos1, Assigned: karlt)

Tracking

unspecified
mozilla2.0b12
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday], )

Attachments

(7 attachments)

Reporter

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: 

Originally reported in:
https://bugzilla.gnome.org/show_bug.cgi?id=629878

"After updating from 2.21.7 to 2.21.8 I get these messages when starting
Firefox:

----------------------------------------------------------------------
(firefox-bin:4965): Gtk-WARNING **: Error loading theme icon 'gtk-go-back-ltr'
for stock: Icon 'gtk-go-back-ltr' not present in theme

(firefox-bin:4965): GLib-GObject-CRITICAL **: g_object_unref: assertion
`G_IS_OBJECT (object)' failed

(firefox-bin:4965): Gtk-CRITICAL **: gtk_default_render_icon: assertion
`base_pixbuf != NULL' failed

(firefox-bin:4965): Gtk-CRITICAL **: IA__gtk_style_render_icon: assertion
`pixbuf != NULL' failed

(firefox-bin:4965): GLib-GObject-CRITICAL **: g_object_ref: assertion
`G_IS_OBJECT (object)' failed
----------------------------------------------------------------------

And similar for 'gtk-go-forward-ltr'."

Those icons are missing also per downstream reports like:
http://bugs.gentoo.org/show_bug.cgi?id=339319


gtk+ developers think this is a mozilla issue per:
https://bugzilla.gnome.org/show_bug.cgi?id=629878#c11
"1) What those mozilla themes do (or rather what firefox does with those stock
ids) is a bug in mozilla. Stock ids need to be looked up as stock items, icon
names need to be looked up in the icon theme."


Thanks a lot for fixing this :-)

Reproducible: Always
Reporter

Updated

9 years ago
OS: Linux → Windows CE
Reporter

Updated

9 years ago
OS: Windows CE → Mac OS X
Hardware: x86_64 → All
Version: unspecified → 3.6 Branch
Reporter

Updated

9 years ago
OS: Mac OS X → Linux
blocking2.0: --- → ?
Component: Theme → ImageLib
Product: Firefox → Core
QA Contact: theme → imagelib
Version: 3.6 Branch → unspecified
Before I decide on blocking, can I get a screenshot of what Firefox looks like without these buttons?

Also, from the gnome bugzilla report, it seems like this is a theme bug.
Component: ImageLib → Widget: Gtk
QA Contact: imagelib → gtk
Reporter

Comment 2

9 years ago
Posted image 1.png
Screenshot attached :-)
I don't understand why this would be a theme bug. What exactly should the theme be doing differently?
This is a hard blocker for any distribution that includes GTK+ 2.22.

I think the theme just needs to not use the -ltr version of the icon; Karl, can you confirm?
Assignee: nobody → karlt
Status: UNCONFIRMED → NEW
blocking2.0: ? → final+
Ever confirmed: true
Whiteboard: [hard blocker]
Whiteboard: [hard blocker] → [hardblocker]
I think you need to rename more than those two. Looking in browser/themes and toolkit/themes, there are other icons that no longer exist.

All the name changes are at

http://git.gnome.org/browse/gtk+/commit/?h=gtk-2-22&id=5c74a696d5c1593be0f6b801cb85a4baf1087883
Assignee

Comment 6

9 years ago
nsIconChannel::Init uses the "path" in the url to lookup stock icons
first by stock id and second by icon name.

For moz-icon://stock/gtk-go-forward-ltr?size=menu, gtk-go-forward was the stock id, so the stock id lookup failed, but gtk-go-forward-ltr was the icon name and that succeeded.

Since 2.21.8 (or 5c74a696d5c1593be0f6b801cb85a4baf1087883), the icon names have
changed and so the icon name lookup also fails.

Using the new icon name ("go-previous-ltr") works with newer GTK, but not for older versions.

Using the stock id ("gtk-go-back") works in new and old versions, but we need to come up with a new method for providing the info from -moz-locale-dir.
Assignee

Updated

9 years ago
Whiteboard: [hardblocker] → [hardblocker][2 days]

Comment 7

8 years ago
Karl, if you have a lot of stuff to do, I can probably take over here.
Assignee

Comment 8

8 years ago
Thanks Ehsan, but I think I can finish this off today or tomorrow.
Assignee

Comment 9

8 years ago
Some clean up.  We've required GTK+ 2.10 for a long time now.
Attachment #509234 - Flags: review?(roc)
Assignee

Updated

8 years ago
Attachment #509235 - Attachment description: don't realize unnecessary windows for stock image widget → part 2: don't realize unnecessary windows for stock image widget
Attachment #509235 - Flags: review?(roc)
Assignee

Comment 11

8 years ago
No behaviour is affected here, but this makes it clearer that the GtkIconSet created is the one used to render, and this direct relationship will be required in part 4.
Attachment #509241 - Flags: review?(roc)
Comment on attachment 509241 [details] [diff] [review]
part 3: rearrange nsIconChannel::Init to make what is happening clearer

+  // gtk_icon_set_render_icon() never returns NULL, except when we have
+  // https://bugzilla.gnome.org/show_bug.cgi?id=629878#c13

It's not obvious how to parse this. Add "... when we have the problem described here:"
Attachment #509241 - Flags: review?(roc) → review+
Assignee

Comment 13

8 years ago
(In reply to comment #12)
Updated part 3 in my queue to say:
 // According to documentation, gtk_icon_set_render_icon() never returns
 // NULL, but it does return NULL when we have the problem reported here:
 // https://bugzilla.gnome.org/show_bug.cgi?id=629878#c13
Assignee

Comment 14

8 years ago
When different directions are involved, there may be several icon names for one stock id, so we can't know in advance what other icon names should be included when we create the stock id; these could change when the theme changes.  It is better to just look up by icon name each time.

This loses the caching provided by the GtkIconSet, but after part 5, our default theme will always be looking up by stock id anyway (which uses the cache).  I make this claim because all occurances of /stock/ in (toolkit|browser)/themes/gnomestripe/**/* match /stock/gtk-; "gtk-" usually indicates a stock id.  GTK is changing icon names beginning with "gtk-" to match standard icon names.

The icon name lookup here is kept for compatibility with other themes/extensions.
Attachment #509249 - Flags: review?(roc)
Assignee

Comment 15

8 years ago
This parses the moz-icon stock name into a gtk stock id and direction, so that gtk-go-back-ltr is found by the stock id look up instead of the icon name lookup.
I might have considered doing this with
moz-icon://stock/gtk-go-back?size=toolbar&state=disabled&direction=ltr
but preserving interfaces, including treatment of moz-icon names, seems better.

GTK used to add the -ltr/-rtl extensions to the stock ids to generate the icon names for the particular direction, and our icon name lookup used to find these icon names.  Changes in GTK mean these icon names no longer exist, causing this bug.
Attachment #509253 - Flags: review?(roc)

Updated

8 years ago
Status: NEW → ASSIGNED
Assignee

Updated

8 years ago
Whiteboard: [hardblocker][2 days] → [hardblocker][can land]

Updated

8 years ago
Whiteboard: [hardblocker][can land] → [hardblocker][can land][has patch]

Updated

8 years ago
Keywords: checkin-needed

Comment 16

8 years ago
http://hg.mozilla.org/mozilla-central/rev/bc23b0490406
http://hg.mozilla.org/mozilla-central/rev/9a9f6f3eec6b
http://hg.mozilla.org/mozilla-central/rev/57536671a363
http://hg.mozilla.org/mozilla-central/rev/29df2b0fa77b
http://hg.mozilla.org/mozilla-central/rev/562ae97b2636
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][can land][has patch] → [hardblocker]
Target Milestone: --- → mozilla2.0b12
Verified fixed with Firefox 4b11.
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
This has reversed the forward and backward arrows for me with RTL UI

My libgtk version is 2.22.0-0ubuntu1
Assignee

Comment 19

8 years ago
Can you file a new bug for that please, Simon, including which icon theme you are using.  I tested the rtl paths of this code against GTK+-2.22.1 by changing -ltr to -rtl in browser.css, which gave expected results with default GTK icon themes.

If you actually had icons for back and forward before this landed, then they must have come from the icon theme.
Depends on: 631978
Assignee

Comment 20

8 years ago
Addresses review comment as indicated in comment 13.

Comment 21

8 years ago
(In reply to comment #20)
> Created attachment 510470 [details] [diff] [review]
> modifiy comment to address review on part 3
> 
> Addresses review comment as indicated in comment 13.

Landed as: http://hg.mozilla.org/mozilla-central/rev/00fcffcbd800
Assignee

Updated

8 years ago
Duplicate of this bug: 640850
You need to log in before you can comment on or make changes to this bug.