621962 - Missing icons with gtk+-2.22

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Pacho Ramos]

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

Comment 1

[Joe Drew (not getting mail)]

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.

Comment 2

[Pacho Ramos]

Attachment: 1.png

Screenshot attached :-)

Comment 3

[Dão Gottwald [:dao]]

I don't understand why this would be a theme bug. What exactly should the theme be doing differently?

Comment 4

[Joe Drew (not getting mail)]

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?

Comment 5

[Frédéric L. W. Meunier]

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

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

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.

Comment 7

[(no longer active)]

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

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

Thanks Ehsan, but I think I can finish this off today or tomorrow.

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: remove support for compilation with GTK+ < 2.4.0

Some clean up.  We've required GTK+ 2.10 for a long time now.

Comment 10

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: part 2: don't realize unnecessary windows for stock image widget

Drive-by optimization.

Comment 11

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: part 3: rearrange nsIconChannel::Init to make what is happening clearer

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.

Comment 12

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

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:"

Comment 13

[Karl Tomlinson (:karlt back Feb 10)]

(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

Comment 14

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: part 4: no longer add stock id to icon name mappings for icons found by name

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.

Comment 15

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: part 5: convert -ltr/-rtl moz-icon stock names to gtk stock ids

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.

Comment 16

[(no longer active)]

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

Comment 17

[u279076]

Verified fixed with Firefox 4b11.

Comment 18

[Simon Montagu :smontagu]

This has reversed the forward and backward arrows for me with RTL UI

My libgtk version is 2.22.0-0ubuntu1

Comment 19

[Karl Tomlinson (:karlt back Feb 10)]

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.

Comment 20

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: modifiy comment to address review on part 3

Addresses review comment as indicated in comment 13.

Comment 21

[(no longer active)]

(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