Closed
Bug 940307
Opened 11 years ago
Closed 11 years ago
Australis panel does not properly support type="menu-button" style buttons
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 29
People
(Reporter: nmaier, Assigned: Gijs)
References
(Blocks 2 open bugs)
Details
(Keywords: addon-compat, Whiteboard: [Australis:P3])
Attachments
(13 files, 8 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gijs
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
MattN
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mikedeboer
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
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.
Assignee | ||
Comment 1•11 years ago
|
||
I'm not sure how fixable this is.
Whiteboard: [Australis] → [Australis:P3]
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
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 ;)
Assignee | ||
Comment 4•11 years ago
|
||
(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. :-(
Reporter | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
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
Blocks: australis-addons
Comment 12•11 years ago
|
||
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•11 years ago
|
||
> 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•11 years ago
|
||
I *think* Gijs is referring to _isOnInteractiveElement in browser/components/customizableui/src/CustomizableUI.jsm, but needinfo-ing just in case.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 16•11 years ago
|
||
Here's a patch that fixes the disappearing panel/menu on menu-button click.
I'll look at layout next.
Attachment #8350160 -
Flags: review?(gijskruitbosch+bugs)
Comment 17•11 years ago
|
||
Another place where menu-buttons don't appear correctly is in the chevron panel.
This is way wider than it should be.
Assignee | ||
Comment 18•11 years ago
|
||
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.
Attachment #8350160 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Comment 19•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee | ||
Comment 21•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Comment 22•11 years ago
|
||
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•11 years ago
|
||
> 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.
Assignee | ||
Comment 24•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Dave: Could you post your CSS?
I couldn't get my menu buttons looking correct in the overlay panel.
Comment 27•11 years ago
|
||
Comment 28•11 years ago
|
||
Comment 29•11 years ago
|
||
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•11 years ago
|
||
Attachment #8361551 -
Attachment description: type="menu-button" - command button highlighted → Print Edit button in overflow panel - command button highlighted
Attachment #8361552 -
Attachment description: type="menu-button" - menu opened → Print Edit button in overflow panel - menu opened
Comment 31•11 years ago
|
||
Comment 32•11 years ago
|
||
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•11 years ago
|
||
> 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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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().
Assignee | ||
Comment 37•11 years ago
|
||
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.
Attachment #8362690 -
Flags: review?(mconley)
Comment 38•11 years ago
|
||
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)
Attachment #8362690 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 39•11 years ago
|
||
This covers everything, I believe, and the test passes.
Assignee | ||
Updated•11 years ago
|
Attachment #8362690 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8363050 -
Flags: review?(MattN+bmo)
Comment 40•11 years ago
|
||
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.
Attachment #8363050 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 41•11 years ago
|
||
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Australis:P3] → [Australis:P3][leave open]
Assignee | ||
Updated•11 years ago
|
Attachment #8363050 -
Flags: checkin+
Assignee | ||
Comment 42•11 years ago
|
||
(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•11 years ago
|
||
Flags: in-testsuite+
Assignee | ||
Comment 44•11 years ago
|
||
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?
Attachment #8363921 -
Flags: review?(mdeboer)
Assignee | ||
Comment 45•11 years ago
|
||
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.
Attachment #8364046 -
Flags: review?(mdeboer)
Assignee | ||
Updated•11 years ago
|
Attachment #8363921 -
Attachment is obsolete: true
Attachment #8363921 -
Flags: review?(mdeboer)
Comment 46•11 years ago
|
||
> 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•11 years ago
|
||
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?
Attachment #8364046 -
Flags: review?(mdeboer) → feedback+
Assignee | ||
Comment 48•11 years ago
|
||
(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.
Depends on: 878546
Comment 49•11 years ago
|
||
Here's a quick mockup of what the border around the arrow should look like.
Assignee | ||
Comment 50•11 years ago
|
||
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.
Attachment #8365081 -
Flags: review?(mdeboer)
Assignee | ||
Updated•11 years ago
|
Attachment #8364046 -
Attachment is obsolete: true
Comment 51•11 years ago
|
||
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...
Attachment #8365081 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 52•11 years ago
|
||
Attachment #8365151 -
Flags: review?(mdeboer)
Assignee | ||
Updated•11 years ago
|
Attachment #8365081 -
Attachment is obsolete: true
Comment 53•11 years ago
|
||
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!
Attachment #8365151 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 54•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8365151 -
Flags: checkin+
Comment 55•11 years ago
|
||
Gijs: Thanks! :)
Comment 56•11 years ago
|
||
Assignee | ||
Comment 57•11 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][leave open] → [Australis:P3]
Target Milestone: --- → Firefox 29
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 58•11 years ago
|
||
(filed bug 964288)
Comment 59•11 years ago
|
||
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•11 years ago
|
||
> 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•11 years ago
|
||
Attachment #8361551 -
Attachment is obsolete: true
Attachment #8361552 -
Attachment is obsolete: true
Attachment #8361662 -
Attachment is obsolete: true
Attachment #8361664 -
Attachment is obsolete: true
Comment 62•11 years ago
|
||
Comment 63•11 years ago
|
||
Comment 64•11 years ago
|
||
Comment 65•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 67•11 years ago
|
||
I've filed bug 964721 for the followup issues. The overflow list issues can be dealt with in bug 964288 and bug 952753.
Updated•11 years ago
|
Updated•11 years ago
|
QA Contact: cornel.ionce
Comment 68•11 years ago
|
||
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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•