1157688 - Toolbarbutton badges don't work on menu-buttons

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: Screenshot of problem

If you add a badge to a menu-button toolbar button, it loses its arrow and gets styled strangely.

Comment 1

[Mike Kaply [:mkaply]]

They don't work on any menu toolbarbuttons.

This CSS

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#141

Has bad specificity.

It should cover the parent button for non menu buttons and for menu buttons, it should apply to the inner button.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #1)
> They don't work on any menu toolbarbuttons.
> 
> This CSS
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#141
> 
> Has bad specificity.
> 
> It should cover the parent button for non menu buttons and for menu buttons,
> it should apply to the inner button.

Sounds like you could write a patch!

Comment 3

[Mike Kaply [:mkaply]]

> Sounds like you could write a patch!

I'm working on it :)

Comment 4

[[e-mail]]

Attachment: Possible patch - let menu-button inherit a specific badge attribute, and switch based on that

I don't think Gijs will like this one because it uses the attribute to switch binding (see bug 994280 comment #24).

Comment 5

[[e-mail]]

Attachment: Possible patch - new XBL binding for menu-button-badged

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8604111 [details] [diff] [review]
Possible patch - new XBL binding for menu-button-badged

Review of attachment 8604111 [details] [diff] [review]:
-----------------------------------------------------------------

Why do we need a new binding?

Comment 7

[Ben Bucksch (:BenB)]

Gijs, to avoid combinatorial explosion, we want to avoid another XBL binding, exactly.

The <toolbarbutton> inside the menu-button already uses a <toolbarbutton>, and that could just xbl:inherit the attribute that switches it to toolbarbutton-badged. However, class is not inherited - probably for good reason, one of them that we'd get an infinite loop. This is why we made patch 8604110 that triggers the toolbarbutton-badge based on a specific attribute, which we can then inherit. For backwards compat, we also switch based on class.

If you don't want the double-usage of having badge="4" also trigger the widget, we could make up another attribute with-badge="true" or whatever, which then triggers the binding. Anything, as long as it isn't the generic "class".

Comment 8

[Ben Bucksch (:BenB)]

Comment on attachment 8604111 [details] [diff] [review]
Possible patch - new XBL binding for menu-button-badged

Neil attached this patch only for demonstration purposes. We are not actually proposing this for inclusion.

Comment 9

[:Gijs (out for now; he/him)]

None of this seems necessary.

toolbarbutton[type=menu-button].badged-button {
 /* bind to regular menu button binding (can probably just add to pre-existing menu button selector for this purpose */
}

toolbarbutton[type=menu-button].badged-button > toolbarbutton /* does this have a more specific class/selector? I forget... */ {
  /* bind to badge binding */
}

and make the badge attribute inherit to the inner toolbarbutton, and we should be done, no?

Comment 10

[[e-mail]]

Attachment: Addressed review comments

This is almost the same as attachment 8604110 [details] [diff] [review] except that it keys off the class on the parent toolbarbutton. I left the CSS reordered as this saves having to fiddle about with artificially specific CSS selectors.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8604319 [details] [diff] [review]
Addressed review comments

Review of attachment 8604319 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me in terms of the basic implementation, though I wouldn't be surprised if this needed more CSS work in order for the result to look nice across different OSes in the Firefox toolbars/panels.

Comment 12

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab04584ba4a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ab04584ba4a

Comment 14

[Mike Kaply [:mkaply]]

Attachment: Extension to show problems.

This patch should be backed out. Badged menu buttons don't work correctly.

1. The button part doesn't highlight.
2. They don't layout correctly when placed in the Australis menu.

1 is the especially bad one.

See attached extension (you'll have to drag and drop the two buttons from customize).

Badged menu buttons need more work.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #14)
> Created attachment 8613527 [details]
> Extension to show problems.
> 
> This patch should be backed out. Badged menu buttons don't work correctly.
> 
> 1. The button part doesn't highlight.
> 2. They don't layout correctly when placed in the Australis menu.

I don't think either of these worked well beforehand, and this bug wasn't about that issue, but about the actual functionality being broken? If more (theme, from the sounds of it) fixes are needed you should file a separate bug.

Comment 16

[Mike Kaply [:mkaply]]

> I don't think either of these worked well beforehand, and this bug wasn't about that issue, but about the actual functionality being broken? If more (theme, from the sounds of it) fixes are needed you should file a separate bug.

Well the badge didn't work at all on menu buttons before. So now we have the badge showing up, but it breaks the menu button. I'm not sure if that is better.

This patch was incomplete. That's why I think it should be backed out.

But I can open separate bugs if you prefer.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #16)
> Well the badge didn't work at all on menu buttons before. So now we have the
> badge showing up, but it breaks the menu button.

It only breaks appearances and not functionality? Sounds better to me. Much more easily fixable by the add-on itself, too.

> This patch was incomplete. That's why I think it should be backed out.

You haven't really answered the relevant question (admittedly I seem to have made my attempt at asking it rambly and run-on - sorry): what did the patch break that was working before, or to phrase it differently, what would be gained by backing it out? The badge would go back to being broken again - what else?

Comment 18

[Mike Kaply [:mkaply]]

> what did the patch break that was working before, or to phrase it differently, what would be gained by backing it out? The badge would go back to being broken again - what else?

There never was a badge on menu-buttons. This patch attempted to add them but when the badge is added, the menu-button is broken. As it stands, we have a feature that if any add-on developer tries to use, it will be broke.

I see no reason to go from something not working at all to something being broken. If add-on developers can't use it yet, it shouldn't be in at all.

Comment 19

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #18)
> > what did the patch break that was working before, or to phrase it differently, what would be gained by backing it out? The badge would go back to being broken again - what else?
> 
> There never was a badge on menu-buttons. This patch attempted to add them
> but when the badge is added, the menu-button is broken. As it stands, we
> have a feature that if any add-on developer tries to use, it will be broke.
> 
> I see no reason to go from something not working at all to something being
> broken. If add-on developers can't use it yet, it shouldn't be in at all.

https://bug1157688.bugzilla.mozilla.org/attachment.cgi?id=8596558 looks broken to me, not "not working at all".

Frankly, I think the best way forward is for you to file bugs and submit CSS patches. The code that was committed here wasn't wrong. I don't think backing it out makes sense. If it doesn't work right for you, and you can't be bothered writing the CSS to make it work, then don't use it. There are plenty of other non-working parts of our platform, and we don't back those out either.