521410 - buttons without labels should ignore the images-in-buttons setting (RSS icon doesn't appear in the Firefox URL bar)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Hideo Oshima]

There is no feed button on the location bar with recent Linux trunk build.

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a1pre) Gecko/20091009 Minefield/3.7a1pre

My Environment:
Distro : Debian GNU/Linux unstable
GNOME: 2.28
GTK+: 2.18.2-1
GLIB: 2.22.2-1

Addtional information:
Bookmarks -> Subscribe to This Page does work.

Comment 1

[Ria Klaassen (not reading all bugmail)]

The icon is present on Windows Vista.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

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

Could we get away with

button[label]:not([label=""]):not(:-moz-system-metric(images-in-buttons)) .button-icon

instead, to avoid having to make the button.xml changes?

(is there a bug about removing the apparently unused buttonleft-ile binding?)

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> Could we get away with
> 
> button[label]:not([label=""]):not(:-moz-system-metric(images-in-buttons))
> .button-icon
> 
> instead, to avoid having to make the button.xml changes?

We could, but what's the problem with the button.xml changes? We did this for toolbarbuttons too.

> (is there a bug about removing the apparently unused buttonleft-ile binding?)

bug 521547

Comment 5

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

(In reply to comment #4)
> We could, but what's the problem with the button.xml changes? We did this for
> toolbarbuttons too.

What use are labels on <xul:image>s, apart from allowing you to use a more efficient selector for this particular case?

Comment 6

[Dão Gottwald [:dao]]

It's just that, the more efficient selector.

Comment 7

[Neil Deakin]

Comment on attachment 405640 [details] [diff] [review]
patch

This should instead be fixed by changing the patch for bug 509671, either by reopening the bug or filing a new one.

Comment 9

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

Comment on attachment 405640 [details] [diff] [review]
patch

Looks right to me (but get Neil to sign off on it anyways)

Comment 10

[Neil Deakin]

I don't really like the idea of copying the string on every button just for this feature. We should at least only do this on linux.

Is a rule button[label]:not... .buttonicon less efficient?

The right way, of course, is to have an <imagebutton> element specifically for buttons intended to be used with images. I'll file a bug on adding this.

Comment 11

[Dão Gottwald [:dao]]

The efficiency of button[label]:not... depends on whether we can use > to get to the .button-icon. Without >, every node up to the root element needs to be tested if button[label]:not... doesn't match the button element.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to comment #10)
> I don't really like the idea of copying the string on every button just for
> this feature. We should at least only do this on linux.
> 
> Is a rule button[label]:not... .buttonicon less efficient?

Addressed this by using:
button[label]:not([label=""]) > .button-box > .button-icon
... for ordinary buttons and:
button[label]:not([label=""]) > .button-box > .box-inherit > .button-icon
... for menu buttons.

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch v2

the right file

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/abb82f981e02

Comment 17

[Hideo Oshima]

I confirmed to fix the problem with recent build.
Thanks for great work.