If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Toolbarbutton badges don't work on menu-buttons

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mkaply, Unassigned)

Tracking

Trunk
Firefox 41
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8596558 [details]
Screenshot of problem

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

Updated

3 years ago
Blocks: 994280
(Reporter)

Comment 1

3 years ago
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.
(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!
(Reporter)

Comment 3

3 years ago
> Sounds like you could write a patch!

I'm working on it :)

Comment 4

3 years ago
Created attachment 8604110 [details] [diff] [review]
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

3 years ago
Created attachment 8604111 [details] [diff] [review]
Possible patch - new XBL binding for menu-button-badged
Attachment #8604111 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8604111 - Flags: review?(gijskruitbosch+bugs)

Comment 7

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

3 years ago
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.
Attachment #8604111 - Flags: review-

Updated

3 years ago
Attachment #8604111 - Attachment description: Possible patch → Possible patch - new XBL binding for menu-button-badged

Updated

3 years ago
Attachment #8604110 - Attachment description: Possible patch → Possible patch - let menu-button inherit a specific badge attribute, and switch based on that
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?
Created attachment 8604319 [details] [diff] [review]
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.
Attachment #8604110 - Attachment is obsolete: true
Attachment #8604319 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8604319 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab04584ba4a
https://hg.mozilla.org/mozilla-central/rev/8ab04584ba4a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Reporter)

Comment 14

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

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.
(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.
Flags: needinfo?(mozilla)
(Reporter)

Comment 16

2 years ago
> 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.
Flags: needinfo?(mozilla)
(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?
Flags: needinfo?(mozilla)
(Reporter)

Comment 18

2 years ago
> 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.
Flags: needinfo?(mozilla)
(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.
You need to log in before you can comment on or make changes to this bug.