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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mixedpuppy, Assigned: neil)

Tracking

(Blocks 1 bug, {addon-compat})

Trunk
Firefox 42
x86
All
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38+ wontfix, firefox39- affected, firefox42 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

5 years ago
Neil, I remember looking at this and not being able to figure out the popup wonkiness here. Can you have a look, please?
Flags: needinfo?(enndeakin)

Comment 2

5 years ago
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
Flags: needinfo?(enndeakin)
Reporter

Comment 3

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

5 years ago
Neil, can we figure out what causes the popup frame to be destroyed by catching in a debugger?
Flags: needinfo?(enndeakin)
Summary: badged button causes havoc with overflow menu → badged button (like social buttons) causes havoc with overflow and panel menu

Updated

5 years ago
Duplicate of this bug: 1037528
Duplicate of this bug: 1035803
Flags: needinfo?(dtownsend+bugmail)
Flags: qe-verify-
Flags: firefox-backlog+
Points: --- → 5
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 36.1
(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

5 years ago
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.
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.
I don't think I can proceed here - but see next comment for more details.
Assignee: mhammond → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(enndeakin)
Flags: needinfo?(dtownsend+bugmail)
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.
Flags: needinfo?(enndeakin)
Iteration: 36.1 → ---
(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

5 years ago
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.
Flags: needinfo?(enndeakin)
(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

5 years ago
(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. :-(
Flags: needinfo?(enndeakin)

Comment 16

4 years ago
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.
Duplicate of this bug: 1124663

Comment 18

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

Updated

4 years ago
Duplicate of this bug: 1146808

Comment 20

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

4 years ago
[Tracking Requested - why for this release]: Main Panel UI is broken
Blocks: 1083327
Flags: needinfo?(dtownsend)
OS: Mac OS X → All
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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 23

4 years ago
I do not have the right combination of availability and knowledge to address this for 38.
Flags: needinfo?(gijskruitbosch+bugs)
Neil is going to take a look at this and if necessary pass it on to someone more appropriate.
Assignee: nobody → enndeakin
Flags: needinfo?(dtownsend)

Comment 25

4 years ago
Posted patch FixSplinter Review
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.
Flags: needinfo?(enndeakin)
Attachment #8583883 - Flags: review?(roc)
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
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.
Attachment #8583883 - Flags: review?(roc) → review-

Comment 27

4 years ago
(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.

Updated

4 years ago
Duplicate of this bug: 1148282
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr

Updated

4 years ago
Duplicate of this bug: 1140823
Neil, do you think you will be able to fix this for the 38 cycle? Thanks
Flags: needinfo?(enndeakin)

Comment 31

4 years ago
No, I am not working on this right now.
Flags: needinfo?(enndeakin)
OK. We have this bug for a while. I am going to wontfix for 38 and untrack it for 39.
Duplicate of this bug: 1141945
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Blocks: 994280
Iteration: 40.2 - 27 Apr → 40.3 - 11 May

Updated

4 years ago
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
Iteration: 40.3 - 11 May → ---
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...

Updated

4 years ago
See Also: → 1179275
Assignee

Comment 35

4 years ago
Posted patch WIP possibility (obsolete) — Splinter Review
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.
Attachment #8632351 - Flags: feedback?(gijskruitbosch+bugs)

Comment 36

4 years ago
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.
Attachment #8632351 - Flags: feedback?(mdeboer)
Attachment #8632351 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8632351 - Flags: feedback+
Assignee

Comment 37

4 years ago
(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.
Assignee

Comment 38

4 years ago
(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.)
Assignee

Comment 39

4 years ago
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.
Attachment #8632351 - Attachment is obsolete: true
Attachment #8632351 - Flags: feedback?(mdeboer)
Attachment #8633057 - Flags: review?(gijskruitbosch+bugs)

Comment 40

4 years ago
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?
Attachment #8633057 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 41

4 years ago
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.
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/68404184c42f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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?
Flags: needinfo?(neil)
Assignee

Comment 45

4 years ago
(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.
Assignee

Comment 46

4 years ago
Posted patch Fix the update badge (obsolete) — Splinter Review
As Gijs is away, please can you review or forward this as appropriate? Thanks.
Flags: needinfo?(neil)
Attachment #8636076 - Flags: review?(bgrinstead)
Depends on: 1185725
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?
Attachment #8636076 - Flags: review?(bgrinstead)
Assignee: nobody → neil
Attachment #8636076 - Attachment is obsolete: true
Depends on: 1189250

Updated

4 years ago
Depends on: 1188001

Updated

4 years ago
Duplicate of this bug: 1196275

Comment 49

4 years ago
(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.

Updated

4 years ago
Duplicate of this bug: 1196397
Depends on: 1206100
Duplicate of this bug: 1162312
You need to log in before you can comment on or make changes to this bug.