1029937 - badged button (like social buttons) causes havoc with overflow and panel menu

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Shane Caraveo (:mixedpuppy)]

If a badged button goes into the overflow menu, *every other time* you click on the overflow button it closes right away.  This was discovered while working on bug 1011598 but is not related to the patches in that bug.

STR:

- install demo provider from http://mixedpuppy.github.io/socialapi-directory/en-US/ (be sure to modify social.directories pref per the about menu on that site)
- click the fake login button in the sidebar, the associated toolbarbutton should now have a badge with a number
- make the window smaller until the toolbarbutton folds into the overflow menu
- click on overflow menu, it will close right away, rinse and repeat (several times)

If you "logout" in the sidebar, the badge disappears and the overflow menu works normally.

The badge uses relative/absolute position stuff in css, my only guess is it has some odd affect here.

Comment 1

[:Gijs (he/him; back Aug 12)]

Neil, I remember looking at this and not being able to figure out the popup wonkiness here. Can you have a look, please?

Comment 2

[Neil Deakin]

Something is destroying the popup's frame just after it opens. Doesn't look like anything is setting hidden though, nor any other suspicious attribute changes seem to occur:

Here is the flow of the popup:

[0x129ce9d10] popupshowing
set attribute:#widget-overflow animate -> open
[0x129ce9d10] opening
set attribute:#nav-bar-overflow-button open -> true
[0x129ce9d10] visible
[0x129ce9d10] popupshown
set attribute: class = panel-arrowcontainer panelopen -> true
set attribute:#widget-overflow panelopen -> true
[0x129ce9d10] popup frame destroyed

Comment 3

[Shane Caraveo (:mixedpuppy)]

I just found that this also affects the menu panel, if you customize the badged button into the menu panel.  It's even worse there, I cannot get the panel to appear properly at all on windows.

Comment 4

[:Gijs (he/him; back Aug 12)]

Neil, can we figure out what causes the popup frame to be destroyed by catching in a debugger?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #4)
> Neil, can we figure out what causes the popup frame to be destroyed by
> catching in a debugger?

What's happening here is that the frame is being destroyed by the following callstack:

>	xul.dll!nsMenuPopupFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 1930	C++
 	xul.dll!nsFrameList::DestroyFrame(nsIFrame * aFrame)  Line 135 + 0x8 bytes	C++
 	xul.dll!nsPopupSetFrame::RemoveFrame(mozilla::layout::FrameChildListID aListID, nsIFrame * aOldFrame)  Line 66	C++
 	xul.dll!nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID aListID, nsIFrame * aOldFrame)  Line 416	C++
 	xul.dll!nsPlaceholderFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 162 + 0x9 bytes	C++
 	xul.dll!nsBoxFrame::RemoveFrame(mozilla::layout::FrameChildListID aListID, nsIFrame * aOldFrame)  Line 1022	C++
 	xul.dll!nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID aListID, nsIFrame * aOldFrame)  Line 416	C++
 	xul.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer, nsIContent * aChild, nsIContent * aOldNextSibling, nsCSSFrameConstructor::RemoveFlags aFlags, bool * aDidReconstruct)  Line 7877	C++
 	xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent, bool aAsyncInsert)  Line 8965 + 0x21 bytes	C++
 	xul.dll!mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList & aChangeList)  Line 754	C++
 	xul.dll!mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame * aFrame, nsChangeHint aMinChange, mozilla::RestyleTracker & aRestyleTracker, nsRestyleHint aRestyleHint)  Line 3620	C++
 	xul.dll!mozilla::RestyleManager::RestyleElement(mozilla::dom::Element * aElement, nsIFrame * aPrimaryFrame, nsChangeHint aMinHint, mozilla::RestyleTracker & aRestyleTracker, nsRestyleHint aRestyleHint)  Line 938	C++
 	xul.dll!mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aChangeHint)  Line 178	C++
 	xul.dll!mozilla::RestyleTracker::DoProcessRestyles()  Line 281	C++
 	xul.dll!mozilla::RestyleManager::ProcessPendingRestyles()  Line 1580	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush)  Line 4229	C++
 	xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp aNowTime)  Line 1247	C++
 	xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver, __int64 jsnow, mozilla::TimeStamp now)  Line 174	C++

Which I'm reading as being a restyle happening as the panel is rendered, causing the frame to be destroyed then recreated.  nsMenuPopupFrame::DestroyFrom hides the popup and I then see the popup being recreated via:

>	xul.dll!nsMenuPopupFrame::nsMenuPopupFrame(nsIPresShell * aShell, nsStyleContext * aContext)  Line 105	C++
 	xul.dll!NS_NewMenuPopupFrame(nsIPresShell * aPresShell, nsStyleContext * aContext)  Line 70 + 0x2a bytes	C++
 	xul.dll!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem & aItem, nsFrameConstructorState & aState, nsContainerFrame * aParentFrame, nsFrameItems & aFrameItems)  Line 3753 + 0x15 bytes	C++
 	xul.dll!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState & aState, nsCSSFrameConstructor::FrameConstructionItemList::Iterator & aIter, nsContainerFrame * aParentFrame, nsFrameItems & aFrameItems)  Line 5812	C++
 	xul.dll!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState & aState, nsCSSFrameConstructor::FrameConstructionItemList & aItems, nsContainerFrame * aParentFrame, nsFrameItems & aFrameItems)  Line 9573	C++
 	xul.dll!nsCSSFrameConstructor::ContentRangeInserted(nsIContent * aContainer, nsIContent * aStartChild, nsIContent * aEndChild, nsILayoutHistoryState * aFrameState, bool aAllowLazyConstruction)  Line 7507	C++
 	xul.dll!nsCSSFrameConstructor::ContentInserted(nsIContent * aContainer, nsIContent * aChild, nsILayoutHistoryState * aFrameState, bool aAllowLazyConstruction)  Line 7104	C++
 	xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent, bool aAsyncInsert)  Line 8984 + 0x26 bytes	C++

However, I can't see how the popup could know the old state was "open" - and even if it could, we'd see the popup flicker (which isn't quite as bad, but still not good enough IMO)

The CSS in question is this line in xul.css:

toolbarbutton.badged-button .toolbarbutton-badge[badge]:not([badge=""])::after {
  content: attr(badge);
}

which is setting content to the "badge" attribute.  IOW, removing this line makes the problem go away.

I'm not sure how to proceed here.  I'd *guess* we just need to find some CSS secret-sauce to avoid this reflow.

Comment 8

[:Gijs (he/him; back Aug 12)]

Why is the frame only created while the popup is showing? Do we set the badge after the popup starts showing?


Seems like a possible fix would be making the badge element be a real element inside the XBL and display:none'ing it when there's no badge.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Discussed in IRC, but FTR:

(In reply to :Gijs Kruitbosch from comment #8)
> Why is the frame only created while the popup is showing?

Initial creation on first showing is expected IIUC.  Reflow causing attempt to replace (ie, recreate) isn't.

> Do we set the badge after the popup starts showing?

Nope - it is set as the button is created (ie, well before popup is created.)  I'm confident this has nothing to do with attached popup* event handlers.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

I don't think I can proceed here - but see next comment for more details.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Neil, please see comment 7 for more context, but the underlying issue is that the ::after selector causes the nsMenuPopupFrame do be destroyed and a new one created.  Best I can tell, this frame recreation is expected behaviour as the pseudo-element is created for ::after.  So it seems like we have 2 options:

* Tweak/modify either the layout or menu frame code such that either the frame isn't destroyed, or it doesn't lose the state when that does happen.  Neil, do you think either of these are feasible?  (Or can you please needinfo? someone else, eg, in the layout team who might be able to answer this?)

* Tweak/modify the XUL/CSS such that ::after isn't used.  I made a few attempts at this (eg, making the button itself a label and tweaking the CSS so it overlayed the icon) - but failed miserably.  The fact the badge is done with ::after makes me think that whoever implemented it this way in the first place also struggled to do it the "obvious" way and had to resort to ::after to work around some other problem.

Either way, I don't know how to push this forward at the current time.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #11)
> I made a few attempts at this (eg, making the button itself a label

should read: making the *badge* itself a label

Comment 13

[Neil Deakin]

Did you investigate what was causing the restyle? The stacks above only show that restyle is happening. A break at RestyleManager::PostRestyleEventCommon might show us something. I'm not sure that I got that far when I investigated above.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #13)
> Did you investigate what was causing the restyle? The stacks above only show
> that restyle is happening. A break at RestyleManager::PostRestyleEventCommon
> might show us something. I'm not sure that I got that far when I
> investigated above.

Breaking there is somewhat tricky as the "normal" flow changes due to interacting with the debugger, and that function is called *many* times between the popup first showing and subsequently hiding.  However, I'm starting to guess it's somehow also related to animations - in the cases where I see the failure, 2 interesting stacks I see are:

>	xul.dll!mozilla::RestyleManager::PostRestyleEventCommon(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, bool aForAnimation)  Line 1659	C++
 	xul.dll!nsIPresShell::RestyleForAnimation(mozilla::dom::Element * aElement, nsRestyleHint aHint)  Line 3043	C++
 	xul.dll!mozilla::AnimationPlayerCollection::PostRestyleForAnimation(nsPresContext * aPresContext)  Line 275	C++
 	xul.dll!nsTransitionManager::FlushTransitions(mozilla::css::CommonAnimationManager::FlushFlags aFlags)  Line 861	C++
 	xul.dll!nsTransitionManager::WillRefresh(mozilla::TimeStamp aTime)  Line 756	C++
 	xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp aNowTime)  Line 1167	C++
and:

>	xul.dll!mozilla::RestyleManager::PostRestyleEventCommon(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, bool aForAnimation)  Line 1659	C++
 	xul.dll!nsIPresShell::RestyleForAnimation(mozilla::dom::Element * aElement, nsRestyleHint aHint)  Line 3043	C++
 	xul.dll!mozilla::AnimationPlayerCollection::PostRestyleForAnimation(nsPresContext * aPresContext)  Line 275	C++
 	xul.dll!nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty, const mozilla::StyleTransition & aTransition, mozilla::dom::Element * aElement, mozilla::AnimationPlayerCollection * & aElementTransitions, nsStyleContext * aOldStyleContext, nsStyleContext * aNewStyleContext, bool * aStartedAny, nsCSSPropertySet * aWhichStarted)  Line 573	C++
 	xul.dll!nsTransitionManager::StyleContextChanged(mozilla::dom::Element * aElement, nsStyleContext * aOldStyleContext, nsStyleContext * aNewStyleContext)  Line 226	C++
 	xul.dll!mozilla::RestyleManager::TryStartingTransition(nsPresContext * aPresContext, nsIContent * aContent, nsStyleContext * aOldStyleContext, nsRefPtr<nsStyleContext> * aNewStyleContext)  Line 1902 + 0x1c bytes	C++

More experimentation shows that if I add animate="false" as an attribute on the <panel id="widget-overflow".../> element the problem also goes away.  IOW, either adding that attribute *or* removing the ...::after {content: ...} rule (see comment 7) makes the problem go away.

I'm not going to find to dig into this for a couple of weeks at least as I'm going to be focused on sync migration work.

Comment 15

[:Gijs (he/him; back Aug 12)]

(In reply to Mark Hammond [:markh] from comment #14)
> (In reply to Neil Deakin from comment #13)
> > Did you investigate what was causing the restyle? The stacks above only show
> > that restyle is happening. A break at RestyleManager::PostRestyleEventCommon
> > might show us something. I'm not sure that I got that far when I
> > investigated above.
> 
> Breaking there is somewhat tricky as the "normal" flow changes due to
> interacting with the debugger, and that function is called *many* times
> between the popup first showing and subsequently hiding.  However, I'm
> starting to guess it's somehow also related to animations - in the cases
> where I see the failure, 2 interesting stacks I see are:
> 
> >	xul.dll!mozilla::RestyleManager::PostRestyleEventCommon(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, bool aForAnimation)  Line 1659	C++
>  	xul.dll!nsIPresShell::RestyleForAnimation(mozilla::dom::Element *
> aElement, nsRestyleHint aHint)  Line 3043	C++
>  
> xul.dll!mozilla::AnimationPlayerCollection::
> PostRestyleForAnimation(nsPresContext * aPresContext)  Line 275	C++
>  
> xul.dll!nsTransitionManager::FlushTransitions(mozilla::css::
> CommonAnimationManager::FlushFlags aFlags)  Line 861	C++
>  	xul.dll!nsTransitionManager::WillRefresh(mozilla::TimeStamp aTime)  Line
> 756	C++
>  	xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp
> aNowTime)  Line 1167	C++
> and:
> 
> >	xul.dll!mozilla::RestyleManager::PostRestyleEventCommon(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, bool aForAnimation)  Line 1659	C++
>  	xul.dll!nsIPresShell::RestyleForAnimation(mozilla::dom::Element *
> aElement, nsRestyleHint aHint)  Line 3043	C++
>  
> xul.dll!mozilla::AnimationPlayerCollection::
> PostRestyleForAnimation(nsPresContext * aPresContext)  Line 275	C++
>  	xul.dll!nsTransitionManager::ConsiderStartingTransition(nsCSSProperty
> aProperty, const mozilla::StyleTransition & aTransition,
> mozilla::dom::Element * aElement, mozilla::AnimationPlayerCollection * &
> aElementTransitions, nsStyleContext * aOldStyleContext, nsStyleContext *
> aNewStyleContext, bool * aStartedAny, nsCSSPropertySet * aWhichStarted) 
> Line 573	C++
>  	xul.dll!nsTransitionManager::StyleContextChanged(mozilla::dom::Element *
> aElement, nsStyleContext * aOldStyleContext, nsStyleContext *
> aNewStyleContext)  Line 226	C++
>  	xul.dll!mozilla::RestyleManager::TryStartingTransition(nsPresContext *
> aPresContext, nsIContent * aContent, nsStyleContext * aOldStyleContext,
> nsRefPtr<nsStyleContext> * aNewStyleContext)  Line 1902 + 0x1c bytes	C++
> 
> More experimentation shows that if I add animate="false" as an attribute on
> the <panel id="widget-overflow".../> element the problem also goes away. 
> IOW, either adding that attribute *or* removing the ...::after {content:
> ...} rule (see comment 7) makes the problem go away.
> 
> I'm not going to find to dig into this for a couple of weeks at least as I'm
> going to be focused on sync migration work.

I don't know what debugger you use, but maybe you can set a trace breakpoint and make the debugger continue (ie not stop/break) but dump stacks? I know Visual Studio and XCode both support this...

Alternatively, Neil, do you have time to investigate further with the above stacks? We're getting more of these buttons (loop, and we're now badging the menu button, though I suppose that'll never be in the panel...), and this is going to start biting more users, and that will be sad. :-(

Comment 16

[Jack W]

Is there an update on this? FF36 is in beta now, and a bug like this blocks having badged buttons in the release channel IMO.

Comment 18

[josesigna]

Any updates on this? This is now live in production (FF 36).

Comment 20

[bart.vda]

As said in other bugreports this bug is not related to specific addons, simply the fact the addonbadge shows a counter. Some addons enable the option to disable their counters. Upon disabling it, the menu works flawless. Maybe force addons to hide their counters when dragged into the menu?

Comment 21

[Alice0775 White]

[Tracking Requested - why for this release]: Main Panel UI is broken

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Gijs: do you think this is fixable for 38? It has some duplicate bugs, has been broken for a long time, and breaks the panel UI.  Thanks!

Tracking for 38 and 39.

Comment 23

[:Gijs (he/him; back Aug 12)]

I do not have the right combination of availability and knowledge to address this for 38.

Comment 24

[Dave Townsend [:mossop]]

Neil is going to take a look at this and if necessary pass it on to someone more appropriate.

Comment 25

[Neil Deakin]

Attachment: Fix

This fixes the problem here. The test is based on a reduced testcase.

Normally, when a transform is changed on an element which contains absolute/relative positioned descendants, this causes a frame reconstruction. However, this doesn't happen when the element having its transform changed is itself positioned. Arrow popup panels assign an animated transform when they open. So we want to treat menupopup frames as positioned elements here.

I'm not sure how to test with existing toolbarbuttons that have badges, but when I added a badge to the home button, this patch fixes this bug.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8583883 [details] [diff] [review]
Fix

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

I don't understand why we need to treat menupopups as special here. You said "Arrow popup panels assign an animated transform when they open", but if you mean our JS/CSS does that, couldn't that apply to any kind of element?

Is the problem that the arrow popup panel gets an animated transform, and that triggers a frame reconstruction, and that triggers something *else* bad happening? If so, we should figure out what else bad is happening and fix that.

Comment 27

[Neil Deakin]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 8583883 [details] [diff] [review]
> Fix
> 
> Review of attachment 8583883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why we need to treat menupopups as special here.

I'm assuming that menupopups should be treated the same as fixed positioned elements.

> "Arrow popup panels assign an animated transform when they open", but if you
> mean our JS/CSS does that, couldn't that apply to any kind of element?
> 
> Is the problem that the arrow popup panel gets an animated transform, and
> that triggers a frame reconstruction, and that triggers something *else* bad
> happening? If so, we should figure out what else bad is happening and fix
> that.

I don't know if there is something else that triggers this. It doesn't matter whether the transform is animated, just changed, as the test shows. The specific combination here needs some positioned elements inside the popup and the :after css parts.

It could happen to other elements I guess, but it wouldn't be as noticeable. The panel is an issue because it stops opening because the frame goes away while it is trying to open.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Neil, do you think you will be able to fix this for the 38 cycle? Thanks

Comment 31

[Neil Deakin]

No, I am not working on this right now.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

OK. We have this bug for a while. I am going to wontfix for 38 and untrack it for 39.

Comment 34

[Mike Kaply [:mkaply]]

So the thing that causes the problem here is the position: absolute on the badge text.

I'm wondering about not showing the badge at all when it is in the panels? That seems like a better solution then not having the panel work at all...

Comment 35

[[e-mail]]

Attachment: WIP possibility

Something along these lines seems to fix the bug, but I've only got as far as fixing up the CSS for Windows so far.

Comment 36

[:Gijs (he/him; back Aug 12)]

Comment on attachment 8632351 [details] [diff] [review]
WIP possibility

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

The approach seems sensible enough to me, but I'm not familiar with Loop. Maybe Mike can comment some more.

Did you test the menubutton badge as well? I don't see any CSS adjustments for that...

::: toolkit/themes/windows/global/toolbarbutton.css
@@ +154,5 @@
>    display: none;
>  }
>  
> +.toolbarbutton-badge[value]:not([value=""]) {
> +  display: -moz-box;

Seems like you could just keep:

.toolbarbutton-badge:not([value]),
.toolbarbutton-badge[value=""] {
  display: none;
}

instead?

@@ -163,5 @@
>    font-size: 10px;
>    font-weight: bold;
>    padding: 1px 2px 2px;
>    color: #fff;
> -  background-color: inherit;

Why remove this, OOI?

@@ +164,5 @@
>    border-radius: 2px;
>    box-shadow: 0 1px 0 hsla(0, 100%, 100%, .2) inset,
>                0 -1px 0 hsla(0, 0%, 0%, .1) inset,
>                0 1px 0 hsla(206, 50%, 10%, .2);
> +  margin: -4px -2px 0px 0px !important;

margin: 4px 0 0;
-moz-margin-end: -2px;

Will avoid having a special RTL rule.

Comment 37

[[e-mail]]

(In reply to Gijs Kruitbosch from comment #36)
> The approach seems sensible enough to me, but I'm not familiar with Loop.
All I'm doing is making the CSS rules more sensible according to the efficient CSS guide. (Also this means that I can fiddle with the class names.)

> Did you test the menubutton badge as well? I don't see any CSS adjustments
> for that...
So far I only tested the default buttons in the Firefox UI on Windows.

> > +.toolbarbutton-badge[value]:not([value=""]) {
> > +  display: -moz-box;
> 
> Seems like you could just keep:
> 
> .toolbarbutton-badge:not([value]),
> .toolbarbutton-badge[value=""] {
>   display: none;
> }
> 
> instead?
Didn't seem to work for me; the rule never appeared to apply. I'll give it another try.

> > -  background-color: inherit;
> 
> Why remove this, OOI?
Because I'm already "adding" the background colour by combining two style rules.

> > +  margin: -4px -2px 0px 0px !important;
> 
> margin: 4px 0 0;
> -moz-margin-end: -2px;
> 
> Will avoid having a special RTL rule.
Sorry, it's just dawned on me the RTL was only necessary because there's no right/left equivalent of -moz-margin-end.

Comment 38

[[e-mail]]

(In reply to comment #37)
> (In reply to Gijs Kruitbosch from comment #36)
> > Did you test the menubutton badge as well? I don't see any CSS adjustments
> > for that...
> So far I only tested the default buttons in the Firefox UI on Windows.
I was unable to "tweak" Firefox's buttons since if I added type="menu" then they didn't display properly. I installed a couple of addons with menu buttons and I was able to tweak them to add badges and those badges appeared to display correctly.

> > > +.toolbarbutton-badge[value]:not([value=""]) {
> > > +  display: -moz-box;
> > 
> > Seems like you could just keep:
> > 
> > .toolbarbutton-badge:not([value]),
> > .toolbarbutton-badge[value=""] {
> >   display: none;
> > }
> > 
> > instead?
> Didn't seem to work for me; the rule never appeared to apply. I'll give it
> another try.
OK I got it to work now, I don't know what was wrong the first time; probably a silly typo. I also moved it to XUL.css because it was just duplication. (I was initially confused because the Windows theme has a display: none; rule that the others don't.)

Comment 39

[[e-mail]]

Attachment: Proposed patch

I tweaked the padding slightly because the xul label appears to be a pixel taller than the pseudoelement.

The end offset on Mac badges is much larger than on other platforms, to the point where if you put a badge on a button with a label then the badge underlaps the label.

Comment 40

[:Gijs (he/him; back Aug 12)]

Comment on attachment 8633057 [details] [diff] [review]
Proposed patch

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

This looks reasonable to me.

::: toolkit/themes/windows/global/toolbarbutton.css
@@ +155,3 @@
>    font-size: 10px;
>    font-weight: bold;
> +  padding: 1px 2px;

This reduces the bottom padding by 1px, right? Why?

Comment 41

[:Gijs (he/him; back Aug 12)]

This is going to break add-ons that have their own bindings that have the same classname, though I suppose that's hard to avoid.

Comment 42

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68404184c42f

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68404184c42f

Comment 44

[Brian Grinstead [:bgrins]]

There is still one instance of toolbarbutton-badge::after referenced in browser/themes/shared/customizableui/panelUIOverlay.inc.css: http://mxr.mozilla.org/mozilla-central/search?string=toolbarbutton-badge%3A%3Aafter.  Should this also be removed or switched over to the new implementation?

Comment 45

[[e-mail]]

(In reply to Brian Grinstead from comment #44)
> There is still one instance of toolbarbutton-badge::after referenced in
> browser/themes/shared/customizableui/panelUIOverlay.inc.css:
> http://mxr.mozilla.org/mozilla-central/search?string=toolbarbutton-
> badge%3A%3Aafter.  Should this also be removed or switched over to the new
> implementation?

Ugh, I'd only searched for toolbarbutton-badge-container, so I overlooked this. Sorry about that.

Comment 46

[[e-mail]]

Attachment: Fix the update badge

As Gijs is away, please can you review or forward this as appropriate? Thanks.

Comment 47

[Brian Grinstead [:bgrins]]

Comment on attachment 8636076 [details] [diff] [review]
Fix the update badge

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

Looks alright overall, although, I see one issue in an unfocused window on OSX that I can add a screenshot for.  I filed Bug 1185725 to do that - can we move this discussion over there?

Comment 49

[:Gijs (he/him; back Aug 12)]

(In reply to Loic from bug 1196275 comment #10)
> Gijs, I believe bug 1029937 has fixed this current issue.
> 
> Do you know if it could be backported into FF41?

I would be worried about backporting this + the 3 dep bugs (1188001 should be fixed "soon") to 41 at this point. It's a large change that will likely break some third-party themes. I realize that the current situation is hardly ideal, but uplifting everything at this point seems risky to the extent that I would be very hesitant.

The other thing is that this has existed since 38 and so one more release of waiting doesn't seem that bad.