940307 - Australis panel does not properly support type="menu-button" style buttons

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Nils Maier [:nmaier]]

Attachment: menu-button.png

In the process of upgrading my add-ons, I discovered that type="menu-button" toolbar buttons do not display correctly within the panel.
See the attached screenshot.

Comment 1

[:Gijs (he/him)]

I'm not sure how fixable this is.

Comment 2

[Mike de Boer [:mikedeboer]]

Hmm, maybe we should strip the menu-button property, get the items from the list and display those in a subview. But perhaps that's a bit too magical + subviews don't support views that go multiple levels deep.

Comment 3

[Nils Maier [:nmaier]]

This is a type="menu-button" not type="menu". There actually is a button and a separate dropdown, not just one button that will display a drop-down ;)

Comment 4

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Hmm, maybe we should strip the menu-button property, get the items from the
> list and display those in a subview. But perhaps that's a bit too magical +
> subviews don't support views that go multiple levels deep.

Considering we didn't even want to add items to existing menus because it'd be too fragile (bug 892463), I don't think this is viable, no. :-(

Comment 5

[Nils Maier [:nmaier]]

I personally plan on converting this particular button into a view (once I figure out how to do that...)

But still, for add-on compat you'll probably at least create some styles for this (and maybe type="menu", didn't test yet) so that it doesn't look like a horrible mess. Bonus points for making the panel not vanish on displaying the button drop-down.

Comment 6

[:Gijs (he/him)]

(In reply to Nils Maier [:nmaier] from comment #5)
> I personally plan on converting this particular button into a view (once I
> figure out how to do that...)
> 
> But still, for add-on compat you'll probably at least create some styles for
> this (and maybe type="menu", didn't test yet) so that it doesn't look like a
> horrible mess. Bonus points for making the panel not vanish on displaying
> the button drop-down.

This is the weirdest, as there was another report of this and there's actually code that's specifically meant to prevent that from happening. Maybe it's the difference between type="menu" and type="menu-button", I haven't had a chance to investigate.

Comment 11

[Mike de Boer [:mikedeboer]]

More informative from Reuben (bug 9440028):

* https://dl.dropboxusercontent.com/u/10968786/dropdown.webm

Higher quality on a MOV container (reading ffmpeg man pages is too hard): https://dl.dropboxusercontent.com/u/10968786/dropdown.mov

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Vertical menu-button

I tried making the button vertical just to see what that would do, but that doesn't work well either. It messes up with the alignment.

Also, as a side note, it looks like the CSS for the menu-button in the menu-panel isn't working, since they are all getting small icons.

Comment 13

[Mike Kaply [:mkaply]]

> This is the weirdest, as there was another report of this and there's actually code that's specifically meant to prevent that from happening.

Would you happen to know where that code is?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I *think* Gijs is referring to _isOnInteractiveElement in browser/components/customizableui/src/CustomizableUI.jsm, but needinfo-ing just in case.

Comment 15

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #14)
> I *think* Gijs is referring to _isOnInteractiveElement in
> browser/components/customizableui/src/CustomizableUI.jsm, but needinfo-ing
> just in case.

Yes. IIRC I checked inbetween writing my original comment and now and found it was only caring about type="menu", not "menu-button" (I love how we have two of these where the distinction is not obvious from the name). The problem is that it walks the DOM tree and so there are various issues (filed) about how if you click items in the menus that then popup, the panel still doesn't go away, etc. etc. All of that needs looking at (preferably in one go, although I suppose not closing the panel when we should isn't as bad as closing it when we shouldn't...). I've not had the time to do it.

Comment 16

[Mike Kaply [:mkaply]]

Attachment: Don't dismiss panel or chevron on menu-button

Here's a patch that fixes the disappearing panel/menu on menu-button click.

I'll look at layout next.

Comment 17

[Mike Kaply [:mkaply]]

Attachment: Menu-button too wide in chevron panel

Another place where menu-buttons don't appear correctly is in the chevron panel.

This is way wider than it should be.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8350160 [details] [diff] [review]
Don't dismiss panel or chevron on menu-button

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

f+ for the right idea, but this needs wholesale adjusting. With this fix, even if you click the button rather than the dropdown, the panel won't close. I'd rather fix this properly.

Comment 19

[Mike Kaply [:mkaply]]

Weird. It was closing for me. But I see what you are saying. I'm marking the whole button as "inMenu" even though it depends on whether you are clicking the menu part or not.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Screenshot of things working better

So by changing the orientation of the menu-button, I've been able to get something that looks reasonable in the panel.

What's strange is that the box is the correct size, and there is no overflow, but for some reason it's placed lower than it should be.

I've looked through the CSS and nothing is jumping out at me.

Gijs: Do you know where these buttons determine their position in the grid and how?

Comment 21

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #20)
> Created attachment 8360689 [details]
> Screenshot of things working better
> 
> So by changing the orientation of the menu-button, I've been able to get
> something that looks reasonable in the panel.
> 
> What's strange is that the box is the correct size, and there is no
> overflow, but for some reason it's placed lower than it should be.
> 
> I've looked through the CSS and nothing is jumping out at me.
> 
> Gijs: Do you know where these buttons determine their position in the grid
> and how?

We're trying to figure this out. It is more complicated than it looks. Basically, it's a wrapping new-style flexbox layout, IIRC. And the vertical positioning can be... weird. We're having issues with the downloads button as well (STR: put downloads button in toolbar, click it, put it in the menu, open the menu, and it's also vertically offset). So far I know of these things that influence things:

- size of the icon
- size of the box used for label (so if you hardcode font size or padding on .toolbarbutton-text or whatever, that breaks stuff)
- potentially whether or not it's a wrapped label or not. Currently labels for buttons with a type attribute don't wrap. The wrapping is causing a lot of issues, and doesn't support proper crop (we're setting textContent inside a label element, and XUL crop doesn't work on that, AFAICT), and is awful for several reasons. So we're looking at what we're doing here. Reverting to single-line labels is an option, so is trying to fix the wrapping to be less awful (however that ends up happening).

Tomorrow should bring some more clarity (we're in the middle of a work week and tomorrow is the first day with enough schedule space to hack/discuss extensively again). See also the other deps of the metabug I filed an hour or two ago.

Comment 22

[:Gijs (he/him)]

So as regards styling of the inside of the button, I'll try and come up with a decent style to make both parts work, but if necessary, we'll probably hide the dropmarker.

Comment 23

[Mike Kaply [:mkaply]]

> but if necessary, we'll probably hide the dropmarker.

That can't really work. Then it's not a menu button.

You can see from my screenshot that making them vertical works. You get separation between the button and the arrow.

Comment 24

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #23)
> > but if necessary, we'll probably hide the dropmarker.
> 
> That can't really work. Then it's not a menu button.

We do the opposite for the bookmarks menu button. It's livable. I don't like it, but really, we're running out of time and this problem is hard. No promises, no free lunch. I'm simply stating a plan because otherwise I'll forget. We can't keep track of the ~250 open bugs without writing stuff down.

> You can see from my screenshot that making them vertical works. You get
> separation between the button and the arrow.

It doesn't to the extent that it wouldn't horizontally line up well, especially with larger fonts, which is something we'll be doing soon. The dropmarker needs to go horizontally next to the icon, just like in a toolbar. I don't know how hard it is to make that work well yet, especially while keeping the icon aligned correctly. We'll look at it tomorrow.

Comment 25

[dave.r.wilson]

I'm not sure why this is such a difficult issue.  Two of my add-ons (Print Edit & Tile Tabs) have toolbar buttons with type="menu-button", and a third add-on (Zoom Page) has a menu button with type="menu" and an associated popup menu.  I have updated all three add-ons for Australis and, with some additional CSS styling, the buttons look fine on the toolbar, in the menu panel, in the overlay panel and in the customization palette.

Is the issue that you are trying to do this generically without any additional CSS styling?

Comment 26

[Mike Kaply [:mkaply]]

Dave: Could you post your CSS?

I couldn't get my menu buttons looking correct in the overlay panel.

Comment 27

[dw-dev]

Attachment: Print Edit button in overflow panel - command button highlighted

Comment 28

[dw-dev]

Attachment: Print Edit button in overflow panel - menu opened

Comment 29

[dw-dev]

Mike,

Screen shots attached.

Note, I had to add a call to CustomizableUI.hidePanelForNode() in my code, to close the overflow panel after clicking on the (inner) command button.

The XUL for the Print Edit button looks like this:

<toolbarbutton id="printedit-buttonmenu" class="toolbarbutton-1 chromeclass-toolbar-additional" type="menu-button" noautoclose="true" .... > 

The CSS for the Print Edit button in the overflow panel looks like this:

#widget-overflow-list #printedit-buttonmenu
{
    list-style-image: url("chrome://printedit/skin/printedit-button-small.png");  /* 16 x 16 */
}

#widget-overflow-list #printedit-buttonmenu > toolbarbutton
{
    padding: 3px 6px;
}

#widget-overflow-list #printedit-buttonmenu > toolbarbutton:hover
{
    -moz-appearance: none;
    padding-left: 5px;
    background-color: hsla(210,4%,10%,.08);
    border-color: hsla(210,4%,10%,.1);
}

#widget-overflow-list #printedit-buttonmenu > toolbarbutton > image
{
    padding: 1px;
}

#widget-overflow-list #printedit-buttonmenu > toolbarbutton > label
{
    padding-left: 5px;
    font-size: 10px;
    text-align: left;
}

Comment 30

[dw-dev]

Attachment: Print Edit button in menu panel - command button highlighted

Comment 31

[dw-dev]

Attachment: Print Edit button in menu panel - menu opened

Comment 32

[dw-dev]

Mike,

For completeness .....

Two more screen shots attached for Print Edit button in menu panel.

The CSS for the Print Edit button in the menu panel looks like this:

#PanelUI-contents #printedit-buttonmenu
{
    list-style-image: url("chrome://printedit/skin/printedit-button-panel.png");
}

#PanelUI-contents #printedit-buttonmenu > toolbarbutton
{
    min-width: 0px;
}

#PanelUI-contents #printedit-buttonmenu > toolbarbutton > image
{
    margin: 5px;
}

#PanelUI-contents #printedit-buttonmenu > toolbarbutton > label
{
    font-size: 10px;
}

Obviously, the Print Edit button CSS may need tweaking for both menu panel and overflow panel, once the generic appearance of buttons in these panels has settled down.

Comment 33

[Ben Bucksch (:BenB)]

> Is the issue that you are trying to do this generically without any additional CSS styling?

Yes. menu-button is a standard type and should just work without extra CSS.

Comment 34

[Mike Kaply [:mkaply]]

I tried to do some investigation into this and honestly got lost in what's going on in CustomizableUI.

There's a system level click handler attached to the panel which is being used to handle the event, but the problem is at the point of click, you don't know whether or not you are on the dropmarker or not, since you only have a toolbarbutton.

The only thing I could find that could be done (and it would be a hack) is that the originalTarget differs between the button and the arrow. When you click on the button of a menu-button, the originalTarget is xul:toolbarbutton. When you click on the dropmarker, the originalTarget is toolbarbutton.

Not sure if that helps anyone, but it's as far as I could figure out.

Comment 35

[dw-dev]

I don't really like the idea of changing the orientation for menu buttons, for several reasons:
- in the past, the default orientation was always horizontal.
- the orientation will be different in the menu panel, as opposed to the toolbar and overflow panel.
- in your screen shot of the menu panel, the button icon is much smaller than for the other buttons.

With regards to the CustomizableUI click handler, when the dropmarker is clicked the originalTarget will have the type="menu-button" attribute, but when the inner button is clicked there is no such attribute.

Comment 36

[dw-dev]

I have tried styling Nils Maier's Download Them All 'menu-button' buttons in the menu panel, using the same approach as I use for Print Edit and Tile Tabs. I had to make one additional tweak to stretch the icon to 32x32, since the existing icon is 16x16.

I don't see why these few CSS rules cannot be applied to all 'menu-button' buttons in the menu panel:

#PanelUI-contents toolbarbutton[type="menu-button"] > toolbarbutton { min-width: 0px; }

#PanelUI-contents toolbarbutton[type="menu-button"] > toolbarbutton > image { margin: 5px; width: 32px; height: 32px; }

#PanelUI-contents toolbarbutton[type="menu-button"] > toolbarbutton > label { font-size: 10px; }

In addition, it is necessary to add noautoclose="true" to the 'menu-button' toolbarbutton to stop the menu panel closing when the menu is opened.

And the CustomizableUI click handler needs changing to close the menu panel when the inner toolbarbutton is clicked -- or each add-on has to have its own click handler and call CustomizableUI.hidePanelForNode().

Comment 37

[:Gijs (he/him)]

Attachment: improve Australis' click-detection code so we close the panel at the right time,

After the closemenu bug, want to review this one, too? It fixes the type='menubutton' toolbarbutton case, as well as type='menu' (bug 961782), as well as the cut/copy/paste issue described in bug 947387, AFAICT.

Comment 38

[Matthew N. [:MattN]]

Comment on attachment 8362690 [details] [diff] [review]
improve Australis' click-detection code so we close the panel at the right time,

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

r- for now since there are a few bugs (undefined variables) which lead me to believe this isn't working properly and may need some more changes. Also, please add some sanity tests.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1199,5 @@
>     * We also check for being outside of any toolbaritem/toolbarbutton, ie on a blank
>     * part of the menu.
>     */
>    _isOnInteractiveElement: function(aEvent) {
> +    function getMenuPopupFor(aNode) {

getMenuPopupForDescendant? I initially thought that you could pass in a menu-button and this would get you the menupopup but it's going somewhat in the other direction.

@@ +1200,5 @@
>     * part of the menu.
>     */
>    _isOnInteractiveElement: function(aEvent) {
> +    function getMenuPopupFor(aNode) {
> +      let lastPopup;

Initialize to null?

@@ +1218,5 @@
> +    // whether we're in a toolbarbutton[type="menu"]
> +    let inMenuTBB = false;
> +    // whether we're in a toolbarbutton[type="menu-button"]
> +    // (yes, that's different)
> +    let inMenuButtonTBB = false;

Remove inMenuTBB and inMenuButtonTBB and inline the checks. (They are assigned to and used once.

@@ +1226,3 @@
>      let inItem = false;
> +    // whether the current node in the loop below is a menuitem
> +    let nowInMenu = false;

Move this inside the while loop below and maybe rename to something like "isMenuItem" to be a bit more clear.

@@ +1243,5 @@
> +      inMenuTBB = inItem && target.type == "menu";
> +      inMenuButtonTBB = inItem && target.type == "menu-button";
> +      if (inItem && target.hasAttribute("closemenu")) {
> +        let closemenuVal = target.getAttribute("closemenu");
> +        closemenu = (closemenuVal == "single" || closemenuval == "none") ?

s/closemenuval/closemenuVal/

This is a good sign that we need new tests for the changes although I was going to ask for some anyways.

@@ +1251,5 @@
> +      // isn't necessarily in their parentNode chain:
> +      if (nowInMenu) {
> +        if (target.hasAttribute("closemenu")) {
> +          let closemenuVal = target.getAttribute("closemenu");
> +          menuitemCloseMenu = (closemenuVal == "single" || closemenuval == "none") ?

ditto

@@ +1255,5 @@
> +          menuitemCloseMenu = (closemenuVal == "single" || closemenuval == "none") ?
> +                              closemenuVal : "auto";
> +        }
> +        let topmostMenuPopup = getMenuPopupFor(target);
> +        target = topmostMenuPopup.triggerNode || target.parentNode;

topmostMenuPopup may be null/undefined if there is a menuitem outside of a "menu…" element which would be weird but, I believe, possible.

@@ +1280,5 @@
> +    // If we're not in a menu, and we *are* in a type="menu-button" toolbarbutton,
> +    // it depends whether we're in the dropmarker or the 'real' button:
> +    if (inMenuButtonTBB) {
> +      // 'real' button (which has a single action):
> +      if (originalTarget.getAttribute("anonid") == "button") {

aEvent.originalTarget (originalTarget is not defined)

Comment 39

[:Gijs (he/him)]

Attachment: improve Australis' click-detection code so we close the panel at the right time,

This covers everything, I believe, and the test passes.

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8363050 [details] [diff] [review]
improve Australis' click-detection code so we close the panel at the right time,

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

Thanks for the tests! r=me

::: browser/components/customizableui/test/browser_940307_panel_click_closure_handling.js
@@ +106,5 @@
> +  }
> +});
> +
> +function isPanelUIOpen() {
> +  return PanelUI.panel.state == "open" || PanelUI.panel.state == "showing";

Nit: might be useful to move to head.js but not necessary.

Comment 41

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/0451fb80e3a2

Comment 42

[:Gijs (he/him)]

(In reply to dw-dev from comment #36)
> I have tried styling Nils Maier's Download Them All 'menu-button' buttons in
> the menu panel, using the same approach as I use for Print Edit and Tile
> Tabs. I had to make one additional tweak to stretch the icon to 32x32, since
> the existing icon is 16x16.
> 
> I don't see why these few CSS rules cannot be applied to all 'menu-button'
> buttons in the menu panel:
> 
> #PanelUI-contents toolbarbutton[type="menu-button"] > toolbarbutton {
> min-width: 0px; }
> 
> #PanelUI-contents toolbarbutton[type="menu-button"] > toolbarbutton > image
> { margin: 5px; width: 32px; height: 32px; }

Because this isn't specific enough for add-on buttons that do #mybutton > toolbarbutton > image { width: 16/18/20px }, in some cases with !important included (especially on OSX, because retina).

I have a somewhat working patch right now, but in some cases it will still depend on add-on authors updating their stylesheets; we're not going to sprinkle !important everywhere until it works.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0451fb80e3a2

Comment 44

[:Gijs (he/him)]

Attachment: fix Australis menu-button styling in menu panel,

So this conflicts with your subview patch, but it should give you an idea what I want to do here. We should also remove the child styling for the toolbarbutton in the overflow panel, but that can be a separate patch. In any case, this seems to make things not look as awful as they are now, at least on OS X. I'll test other platforms shortly. Unfortunately, there are a lot of options here and none of them are ideal. Changing the orientation of the button so the dropdown is below the icon is weird because then the icon is resized or the button is much bigger. Having the dropdown be as large as the 'real' button shifts the label (often leading to wrapping/hyphenation) as well as the icon, which looks off compared to the grid of the icons of the other buttons, too. So I opted to have the dropdown on the right, but kept the centering of the icon. The downside of this is that it will overlap the icon on (very) small OS font sizes, and that the dropdown still appears at the bottom of the button, even though the dropdown icon is now at the top (a problem it shares with the first option I named earlier). Otherwise, this seems to work fine. Mike, what do you think?

Comment 45

[:Gijs (he/him)]

Attachment: fix Australis menu-button styling in menu panel,

I missed some bits, apparently. This works on Windows as well. The important usage is a bit sad, it's because some of the other styles for the dropmarker use that to set its borders... Otherwise, I'm fairly happy with this patch. It'll need the subview patch not to break subview styling, I think, but otherwise it works, AFAICT.

Comment 46

[Ben Bucksch (:BenB)]

> we're not going to sprinkle !important everywhere until it works.

;-)

If you're specifically crafting a rule to be more specific than another one, please make sure to note that as source code comment, so that the generations after you (i.e. devs in 2 years) don't accidentally regress things.

Comment 47

[Mike de Boer [:mikedeboer]]

Comment on attachment 8364046 [details] [diff] [review]
fix Australis menu-button styling in menu panel,

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

f+, because I do have a couple of questions and one other point:

Even though this looks a lot better than what we have at the moment, I think it's still good to present this to UX for some early ideas. If we have a chance to get this right in one go, I'd go for it!

::: browser/themes/osx/browser.css
@@ +1102,5 @@
>    }
>  }
>  
> +toolbar .toolbarbutton-1:not([type="menu-button"]),
> +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button,

Don't we have `cui-areatype="toolbar"` for this one, instead of using a descendent selector?

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +144,5 @@
> +}
> +
> +.panelUI-grid .toolbarbutton-1:not([buttonover])@buttonStateHover@ > .toolbarbutton-menubutton-dropmarker {
> +  background-color: hsla(210,4%,10%,.08) !important;
> +  border-color: hsla(210,4%,10%,.1) !important;

Why the `!important` here (also in the block above)? Is this a case of add-ons messing with our plan? If so, can you document that here?

I see that you removed the expand/ contract rules with transition properties, but the transitions still work when I try this patch locally. I couldn't understand _why_ exactly, so could you explain that to me?

@@ +173,5 @@
> +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-icon,
> +.customization-palette .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> +.customization-palette .toolbarbutton-1 > .toolbarbutton-icon,
> +.panel-customization-placeholder-child > .toolbarbutton-icon {

I really like the simplification of the rules here!

@@ +323,5 @@
>    display: none;
>  }
>  
> +panelview .toolbarbutton-1,
> +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button,

This breaks the styling of buttons inside subviews. I know that I'm addressing that in bug 878546, but I think it's still good to not depend on it, right?

Otherwise, we should make this bug depend on bug 878546.

@@ +346,5 @@
>    margin-top: 6px;
>  }
>  
> +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button {
> +  border: 0 none;

I *think* `border: 0` would do the trick just as well, right?

Comment 48

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #47)
> Comment on attachment 8364046 [details] [diff] [review]
> fix Australis menu-button styling in menu panel,
> 
> Review of attachment 8364046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+, because I do have a couple of questions and one other point:
> 
> Even though this looks a lot better than what we have at the moment, I think
> it's still good to present this to UX for some early ideas. If we have a
> chance to get this right in one go, I'd go for it!
> 
> ::: browser/themes/osx/browser.css
> @@ +1102,5 @@
> >    }
> >  }
> >  
> > +toolbar .toolbarbutton-1:not([type="menu-button"]),
> > +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button,
> 
> Don't we have `cui-areatype="toolbar"` for this one, instead of using a
> descendent selector?

Several people have complained that that doesn't cater for add-on buttons inside toolbaritems. We used to apply this for all of the toolbarbuttons, so I figured we shouldn't now restrict it and break add-ons.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +144,5 @@
> > +}
> > +
> > +.panelUI-grid .toolbarbutton-1:not([buttonover])@buttonStateHover@ > .toolbarbutton-menubutton-dropmarker {
> > +  background-color: hsla(210,4%,10%,.08) !important;
> > +  border-color: hsla(210,4%,10%,.1) !important;
> 
> Why the `!important` here (also in the block above)? Is this a case of
> add-ons messing with our plan? If so, can you document that here?

No, it's a case of overriding these rules:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1169
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#81

which use !important themselves. Sigh.

> I see that you removed the expand/ contract rules with transition
> properties, but the transitions still work when I try this patch locally. I
> couldn't understand _why_ exactly, so could you explain that to me?

That code is dead, the attributes are no longer used for the transition.
 
> @@ +173,5 @@
> > +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> > +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-icon,
> > +.customization-palette .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> > +.customization-palette .toolbarbutton-1 > .toolbarbutton-icon,
> > +.panel-customization-placeholder-child > .toolbarbutton-icon {
> 
> I really like the simplification of the rules here!
> 
> @@ +323,5 @@
> >    display: none;
> >  }
> >  
> > +panelview .toolbarbutton-1,
> > +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button,
> 
> This breaks the styling of buttons inside subviews. I know that I'm
> addressing that in bug 878546, but I think it's still good to not depend on
> it, right?
> 
> Otherwise, we should make this bug depend on bug 878546.

Yes.

> 
> @@ +346,5 @@
> >    margin-top: 6px;
> >  }
> >  
> > +.panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button {
> > +  border: 0 none;
> 
> I *think* `border: 0` would do the trick just as well, right?

Probably.

Comment 49

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Quick Mockup

Here's a quick mockup of what the border around the arrow should look like.

Comment 50

[:Gijs (he/him)]

Attachment: fix Australis menu-button styling in menu panel,

After some discussion with phlsa, we decided to drop the border entirely (because it doesn't contrast with the darker background enough to make it work) and just make the background color match the outer border of the button. This patch does that.

Comment 51

[Mike de Boer [:mikedeboer]]

Comment on attachment 8365081 [details] [diff] [review]
fix Australis menu-button styling in menu panel,

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

I *think* the border-radius in RTL mode is the only thing left!

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +157,5 @@
> +}
> +
> +.panelUI-grid .toolbarbutton-1:not([buttonover])@buttonStateHover@ > .toolbarbutton-menubutton-dropmarker {
> +  background-color: hsla(210,4%,10%,.1) !important;
> +  border-radius: 0 0 0 2px;

This won't work in RTL mode I'm afraid...

Comment 52

[:Gijs (he/him)]

Attachment: fix Australis menu-button styling in menu panel,

Comment 53

[Mike de Boer [:mikedeboer]]

Comment on attachment 8365151 [details] [diff] [review]
fix Australis menu-button styling in menu panel,

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

<3-ing this!

Comment 54

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/88c332b7c875

Still leaving this open and I'll look at the overflow panel styling over the weekend / on Monday.

Comment 55

[Ben Bucksch (:BenB)]

Gijs: Thanks! :)

Comment 56

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/88c332b7c875

Comment 57

[:Gijs (he/him)]

This seems to work acceptably in the overflow panel right now. There's no hover styling specifically for the dropdown arrow, but it isn't nearly the mess that it was, or as bad as it was in the menu panel. I'm going to mark this fixed, and file a separate (lower priority) bug for the overflow panel.

Comment 58

[:Gijs (he/him)]

(filed bug 964288)

Comment 59

[Mike Kaply [:mkaply]]

Attachment: PopupScreenshot.jpg

This looks so much better than before. Great job.

I'm still seeing my icon a little offset above the rectangle (although the rectangle always draws consistently).

Is this something I need to fix or are more tweaks needed?

As as side note, I found it impossible to screen capture the Australis dropdown on Windows 7, using the built in screenshot tool. It's not that the menu went away, it's that it simply did not appear in any of the resulting captures (fullscreen or otherwise).

Comment 60

[Ben Bucksch (:BenB)]

> This looks so much better than before. Great job.

Indeed, it looks really nice, thank you!

> I'm still seeing my icon a little offset above the rectangle

and the whole button (icon and text) seems 1em too high in comparison to the other "Developer" button. Maybe adding just a bit more margin-top ?

Comment 61

[dw-dev]

Attachment: Print Edit Button - one-line text label - incorrectly aligned

Comment 62

[dw-dev]

Attachment: Download Them All Button - one-line text label - incorrectly aligned

Comment 63

[dw-dev]

Attachment: Download Them All Button -two-line text label - correctly aligned

Comment 64

[dw-dev]

Attachment: Print Edit Button - dropmarker still highlighted after closing menu by clicking menu panel background

Comment 65

[dw-dev]

Please see attachments 8366515, 8366516, 8366517 & 8366518.

I don't think this is fixed.

The styling is much improved, BUT the positioning of the icon and text label is ONLY correct if the text label wraps into a second line.

If the text label fits on one line, then both the icon and the text label are displayed too high, and the icon extends outside the button.

I have tested this with Print Edit's button and Download Them All's buttons.

Also, if you click on the dropmarker to show the menu, and then click on the menu panel background, the menu closes as expected, BUT the dropmarker is still highlighted.

Comment 66

[dw-dev]

With regards to menu buttons in the Overflow Panel, the styling is much improved, but tere are still some issues.

The dropmarker should be highlighted when the mouse hovers over it, to be consistent with the dropmarker highlighting in the Menu Panel.

Bug 952753 relating to the Overflow Panel has not been fixed, but the same use cases do work correctly in the Menu Panel, indicating an inconsistency between how these panels are implemented.

Comment 67

[:Gijs (he/him)]

I've filed bug 964721 for the followup issues. The overflow list issues can be dealt with in bug 964288 and bug 952753.

Comment 68

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on the latest Aurora (Build ID: 20140309004003) and the latest Nightly (Build ID: 20140309030204), using specific add-ons [1] under the following platforms:
- Windows 7 64-bit [2]
- Ubuntu 13.10 64-bit [3]
- Mac OS X 10.9.1 [4]

1. Firebug 1.12.7; Session Manager 0.8.1.0; Click&Clean 4.1; Print Edit 11.4; Tile Tabs 11.4. 
2. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
4. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0