Closed Bug 1157688 Opened 5 years ago Closed 4 years ago

Toolbarbutton badges don't work on menu-buttons

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: mkaply, Unassigned)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached image Screenshot of problem
If you add a badge to a menu-button toolbar button, it loses its arrow and gets styled strangely.
Blocks: 994280
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!
> Sounds like you could write a patch!

I'm working on it :)
I don't think Gijs will like this one because it uses the attribute to switch binding (see bug 994280 comment #24).
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)
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 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-
Attachment #8604111 - Attachment description: Possible patch → Possible patch - new XBL binding for menu-button-badged
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?
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/mozilla-central/rev/8ab04584ba4a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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)
> 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)
> 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.