Missing icons with gtk+-2.22

VERIFIED FIXED in mozilla2.0b12

Status

()

Core
Widget: Gtk
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Pacho Ramos, Assigned: karlt)

Tracking

unspecified
mozilla2.0b12
All
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

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

Attachments

(7 attachments)

(Reporter)

Description

7 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

7 years ago
OS: Linux → Windows CE
(Reporter)

Updated

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

Updated

7 years ago
OS: Mac OS X → Linux

Updated

7 years ago
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

7 years ago
Created attachment 500885 [details]
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

7 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

6 years ago
Whiteboard: [hardblocker] → [hardblocker][2 days]
Karl, if you have a lot of stuff to do, I can probably take over here.
(Assignee)

Comment 8

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

Comment 9

6 years ago
Created attachment 509234 [details] [diff] [review]
remove support for compilation with GTK+ < 2.4.0

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

Comment 10

6 years ago
Created attachment 509235 [details] [diff] [review]
part 2: don't realize unnecessary windows for stock image widget

Drive-by optimization.
(Assignee)

Updated

6 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)
Attachment #509234 - Flags: review?(roc) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 509241 [details] [diff] [review]
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.
Attachment #509241 - Flags: review?(roc)
Attachment #509235 - Flags: review?(roc) → review+
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

6 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

6 years ago
Created attachment 509249 [details] [diff] [review]
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.
Attachment #509249 - Flags: review?(roc)
Attachment #509249 - Flags: review?(roc) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 509253 [details] [diff] [review]
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.
Attachment #509253 - Flags: review?(roc)
Attachment #509253 - Flags: review?(roc) → review+

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

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

Updated

6 years ago
Whiteboard: [hardblocker][can land] → [hardblocker][can land][has patch]
Keywords: checkin-needed
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: 6 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

6 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

6 years ago
Created attachment 510470 [details] [diff] [review]
modifiy comment to address review on part 3

Addresses review comment as indicated in comment 13.
(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

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