583168 - In icons only mode, not all buttons get UI

Status: RESOLVED INVALID

(Firefox :: Theme, defect)

Description

[Mike Kaply [:mkaply]]

We have a toolbar with menu-buttons.

On a Mac when we switch to icon only mode, they get extra stuff painted around them that wasn't there in 3.6 and isn't there in text or icon/text mode.

Image attached.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Picture of problem

Comment 2

[Dão Gottwald [:dao]]

The button affordance is intentional, bug 559033.

Comment 3

[Mike Kaply [:mkaply]]

I read through that bug and saw no referfence to menu-button specifically.

The issue here is that we now get incorrectly incosistent display of buttons on Mac. To be blunt, it looks like crap. Attaching images.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Icon only. Notice the mixed UI.

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Icons+Text - UI is consistent.

Comment 6

[Mike Kaply [:mkaply]]

OK, I guess I understand now. So the problem here is actually the opposite of what I described.

The problem is that the other icons on my toolbar should be getting borders around them similar to the Firefox UI.

We're only putting UI around menu-buttons.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> The problem is that the other icons on my toolbar should be getting borders
> around them similar to the Firefox UI.

These buttons need the toolbarbutton-1 class.

Comment 8

[Mike Kaply [:mkaply]]

Interesting.

I think we might want to evangelize this again for FF4.

While it's in the docs, I did a quick glance at some common toolbar addons and noone seems to include this because it never did anything before...

Comment 9

[Ben Bucksch (:BenB)]

I still think it's a FF bug. Any <toolbarbutton> on <toolbar> should just work. The class should not be necessary, it's implicit. And it worked in FF3.

Comment 10

[Ben Bucksch (:BenB)]

> While it's in the docs

It's not, <https://developer.mozilla.org/en/XUL/toolbarbutton> didn't say anything about it. <https://developer.mozilla.org/en/creating_toolbar_buttons>, but that's an informal howto, not a proper official doc.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> It's not, <https://developer.mozilla.org/en/XUL/toolbarbutton> didn't say
> anything about it.

That's the general toolkit documentation. This is app specific code, there's nothing wrong with requiring add-ons to use app specific hooks on top of the toolkit APIs.

.toolbarbutton-1 isn't new either, it was used before, even in Firefox 3. The only change on trunk is that the default theme changes the style more radically.

Comment 12

[Ben Bucksch (:BenB)]

> there's nothing wrong with requiring add-ons to use app specific hooks
> on top of the toolkit APIs.

There very much is, if it's unnecessary. It is here. If all <toolbarbutton>s on <toolbar> need it, then make it implicit.

You must not break extensions unless it's really necessary, and the new change is then pretty sure to not break again in the future. It's not necessary here. You can't break all extensions just because of the style of the day, be it 18x18 icons or arbitrary style changes (*especially* if they appear on one platform only, so the developer might not even notice!) or whatever.

Comment 13

[Dão Gottwald [:dao]]

We don't want to apply the style on all toolbarbuttons on toolbars.

Comment 14

[Ben Bucksch (:BenB)]

> We don't want to apply the style on all toolbarbuttons on toolbars.

Can you be any more vague?

Seems like it doesn't work without them, you told us to add that style, we felt forced to do just that, and the informal "documentation" quoted does read like that.
And that class breaks something else, see bug 583231. It also broke/changed the padding in FF3, which worked fine before.