After bug 878546 landed the widgets panels (when a widget is placed in the toolbar) there seems to be a few issues. First the widgets show icon and text in the toolbar. Secondly the panel content looks pretty bad. For example the header is cut on both sides.

Guillaume, I don't see what you're seeing, I'm afraid :S Could you post a screenshot of the cut-off headers? And perhaps an STR for getting icon & text in the toolbar? Thanks!!

I'm not sure anymore if it's really subviews related.

Oups wrong bug

Here it is. Both text issue and poor panel styling can be seen here. The wrong attachment can be marked obsolete.

Whoops my bad the text issue is not subviews related. The "not so good" looking panels are though.

Jared, would you like to take this one too?

It's a good idea to hide the header. But the footer (of the history panel for example) will still looks bad. A big problem is the arrowpanel padding which is a bit too big I think.

Comment on attachment 8364449 [details] [diff] [review] Patch v1: don't show panel header labels when widget is placed in toolbar Review of attachment 8364449 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this doesn't fix the styling of the footer (similar issue).

This makes the footer kind of a mix 'n match between the new and previous style. I'm not sure if I like it, so I guess I just get to ask ppls opinion here. Maybe get some UX input here as well...

Comment on attachment 8364474 [details] [diff] [review] Patch v1.1: don't show panel header labels when widget is placed in toolbar Review of attachment 8364474 [details] [diff] [review]: ----------------------------------------------------------------- The "Show All History" text looks a tiny-bit smaller than the rest of the text within the popup. Here's a link to a screenshot of the patch with the History widget: http://screencast.com/t/8i7shwiX1iZ

Removing the header from the panel makes total sense :) Ideally, the text would line up vertically (see http://cl.ly/image/3l2v1x382X0n) and reduce the space between the favicons and the labels (like in the bookmarks panel). Regarding the footer, I think the consistent thing would be to place the "Show All History" item at the top of the menu, style it like a menu entry and also show its keyboard shortcut. This would be the same behavior as in the bookmarks panel.

Moving the "Show All Bookmarks" item to the top would also make bug 963480 somewhat less severe.

(In reply to Philipp Sackl [:phlsa] from comment #12) > Moving the "Show All Bookmarks" item to the top would also make bug 963480 > somewhat less severe. Moving that item to the top regardless would make our styling woes go away *magically*. The footer-button is just a pain, TBH. Is that a possibility?

(In reply to Mike de Boer [:mikedeboer] from comment #13) > (In reply to Philipp Sackl [:phlsa] from comment #12) > > Moving the "Show All Bookmarks" item to the top would also make bug 963480 > > somewhat less severe. > > Moving that item to the top regardless would make our styling woes go away > *magically*. The footer-button is just a pain, TBH. Is that a possibility? That would be a very welcome side effect :)

Long term goal would be to implement the styling seen on the Win 8 mockup (bookmarks menu button) : http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html

Hmmm, I see now. In that case I should just make it work then. I kinda dislike long term goals like that ;)

AFAIK it's by design to have the footer always the same at least for history, bookmarks and downloads with for every of them the "Show all..." at the bottom.

You're right, I should have looked at the mockups first. Anyway, I think the rest of my feedback still stands.

(In reply to Philipp Sackl [:phlsa] from comment #18) > You're right, I should have looked at the mockups first. > Anyway, I think the rest of my feedback still stands. The mockup shows all the items aligned to the left. It means that the favicons are aligned with the items that haven't one.

Gijs, I'm looking for ideas here; for the life of me, I can't get the footer to be displayed without margin inside the widget panel and adding rounded corners to its bottom is also not working for me. Do you know why and how we might be able to pull this off?

(In reply to Mike de Boer [:mikedeboer] from comment #20) > Created attachment 8365011 [details] [diff] [review] > Patch v1.2: adjust toolbar widget panel styling > > Gijs, I'm looking for ideas here; for the life of me, I can't get the footer > to be displayed without margin inside the widget panel and adding rounded > corners to its bottom is also not working for me. > > Do you know why and how we might be able to pull this off? You're giving the arrowcontent 4px padding. As the footer is inside that, it is 4px removed from the outside of the box. You should probably set that to 0 and set something else to have the padding you need for the items to show up with the appropriate amount of space...

Comment on attachment 8365163 [details] [diff] [review] Patch v1.3: adjust toolbar widget panel styling Review of attachment 8365163 [details] [diff] [review]: ----------------------------------------------------------------- I have too many questions. :-( ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +116,5 @@ > -moz-padding-start: 8px; > display: -moz-box; > } > > +#customizationui-widget-panel > .panel-arrowcontainer > .panel-arrowcontent { Put this together with the widget-overflow style which does exactly the same. Also, id rather than class selector - is that necessary to override the style? @@ +367,5 @@ > border: none; > } > > +.PanelUI-subView .subviewbutton.panel-subview-footer > .toolbarbutton-text, > +#customizationui-widget-panel .subviewbutton.panel-subview-footer > .toolbarbutton-text { Why doesn't this match the first rule, and can't we just fix that instead? @@ +385,5 @@ > font-weight: normal; > color: inherit; > } > > +.customizationui-widget-panel .subviewbutton:not(.panel-subview-footer) { Does this work with the bookmarks menu/panel? @@ +390,5 @@ > + margin-left: 4px; > + margin-right: 4px; > +} > + > +.customizationui-widget-panel .subviewbutton:not(.panel-subview-footer):first-of-type { This won't work if the subviewbuttons are in separate containers within the view, because they'll be the first of their type in the container, so these margins will apply to several of the items - e.g. the character encoding menu. Why don't you add padding-top to the arrow contents of the panel?

(In reply to :Gijs Kruitbosch from comment #23) > Does this work with the bookmarks menu/panel? I didn't work on the Bookmarks panel... I guess I can do that in this bug too... > This won't work if the subviewbuttons are in separate containers within the > view, because they'll be the first of their type in the container, so these > margins will apply to several of the items - e.g. the character encoding > menu. > > Why don't you add padding-top to the arrow contents of the panel? I will, but what do you suggest for the bottom padding that's now missing? If there's a footer, the padding should be 0 to make it hug the panel border and if there is no padding, it should have 4px.

Fixing this bug will require to rewrite panel tests. (As we noticed with the other Windows 8 bug trying to change the panel padding).

(In reply to Tim Nguyen [:ntim] from comment #25) > Fixing this bug will require to rewrite panel tests. (As we noticed with the > other Windows 8 bug trying to change the panel padding). I'm pretty sure it won't - the styles Mike is adding are specific to these panels, and those tests create their own panels - the earlier commit was falling foul of them because the *default* padding was changed for all panels. (In reply to Mike de Boer [:mikedeboer] from comment #24) > (In reply to :Gijs Kruitbosch from comment #23) > > Does this work with the bookmarks menu/panel? > > I didn't work on the Bookmarks panel... I guess I can do that in this bug > too... > > > This won't work if the subviewbuttons are in separate containers within the > > view, because they'll be the first of their type in the container, so these > > margins will apply to several of the items - e.g. the character encoding > > menu. > > > > Why don't you add padding-top to the arrow contents of the panel? > > I will, but what do you suggest for the bottom padding that's now missing? > If there's a footer, the padding should be 0 to make it hug the panel border > and if there is no padding, it should have 4px. To be honest, the easiest would be to add a class to the container if there is a footer. :-) If that seems ugly, can you always add padding and give the footer negative bottom margin, or is there some reason that doesn't work?

Alright, went all the way including the Bookmarks panel... hence the request for feedback. Is this the right direction? Is this good enough for review?

meh, review is fine too ;)

Comment on attachment 8366059 [details] [diff] [review] Patch v1.4: adjust toolbar widget panel styling Review of attachment 8366059 [details] [diff] [review]: ----------------------------------------------------------------- Generally I think this is OK, but a bunch of questions follow. :-) ::: browser/base/content/browser.xul @@ +725,5 @@ > onpopupshowing="BookmarkingUI.onPopupShowing(event); > + if (!this.parentNode._placesView) { > + new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU', { > + menuClass: 'subviewbutton', > + menuitemClass: 'subviewbutton' Hrm. Maybe just add a single property for now? For now, there's no distinction, so having more than one of these doesn't seem necessary. Call the property 'extraClass' or something. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +6,5 @@ > > %define menuPanelWidth 22.35em > %define exitSubviewGutterWidth 38px > +%define buttonStateHover :not(:-moz-any([disabled],[open],[checked="true"],:active)):hover > +%define buttonStateActive :not([disabled]):-moz-any([open],[checked="true"],:hover:active) Uhh, this hunk probably doesn't go in this patch? @@ +49,5 @@ > > .subviewbutton.panel-subview-footer { > margin: 4px -4px -4px; > box-shadow: 0 1px 0 hsla(210,4%,10%,.08) inset; > + -moz-box-ordinal-group: 2; Hmm, why this? @@ +384,5 @@ > border-radius: 0; > border: none; > } > > +.PanelUI-subView .subviewbutton.panel-subview-footer > .toolbarbutton-text, Do we need the first class + descendant selector? @@ +396,5 @@ > .PanelUI-subView .subviewbutton:not(.panel-subview-footer) { > margin: 2px 0; > } > > +.PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .toolbarbutton-text, Ditto. @@ +406,5 @@ > font-weight: normal; > color: inherit; > } > > +.cui-widget-panelview .subviewbutton:not(.panel-subview-footer) { Ditto! @@ +428,5 @@ > } > > panelview .toolbarbutton-1@buttonStateHover@, > panelview .subviewbutton@buttonStateHover@, > +menupopup .subviewbutton@buttonStateHover@, Why not just .subviewbutton@buttonStateHover@ ? Are these ever anywhere but in a menupopup/panelview? :-) @@ +444,5 @@ > } > > panelview .toolbarbutton-1@buttonStateActive@, > panelview .subviewbutton@buttonStateActive@, > +menupopup .subviewbutton@buttonStateActive@, Ditto. @@ +479,5 @@ > border-top: 1px solid ThreeDShadow; > margin: 5px 0; > } > > +panelview .subviewbutton > .menu-accel-container, Same here - do we need the tagname descendant selector before it?

(In reply to :Gijs Kruitbosch from comment #29) > @@ +49,5 @@ > > > > .subviewbutton.panel-subview-footer { > > margin: 4px -4px -4px; > > box-shadow: 0 1px 0 hsla(210,4%,10%,.08) inset; > > + -moz-box-ordinal-group: 2; > > Hmm, why this? This is there to make sure that the footer is displayed at the bottom of a list. For example, the bookmarks code appends the bookmark items, which makes it not possible to place the footer button at the bottom in browser.xml or panelUI.inc.xml.

(In reply to Mike de Boer [:mikedeboer] from comment #30) > (In reply to :Gijs Kruitbosch from comment #29) > > @@ +49,5 @@ > > > > > > .subviewbutton.panel-subview-footer { > > > margin: 4px -4px -4px; > > > box-shadow: 0 1px 0 hsla(210,4%,10%,.08) inset; > > > + -moz-box-ordinal-group: 2; > > > > Hmm, why this? > > This is there to make sure that the footer is displayed at the bottom of a > list. For example, the bookmarks code appends the bookmark items, which > makes it not possible to place the footer button at the bottom in > browser.xml or panelUI.inc.xml. I'd prefer to fix the bookmarks code to insertBefore the footer...

Comment on attachment 8366130 [details] [diff] [review] Patch v1.5: adjust toolbar widget panel styling Meant to do this earlier, clearly it didn't work...

Comment on attachment 8366303 [details] [diff] [review] Patch v1.6: adjust toolbar widget panel styling Review of attachment 8366303 [details] [diff] [review]: ----------------------------------------------------------------- This actually LGTM, assuming you've tested appropriately (of course you have!) ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +121,5 @@ > + padding: 4px 0; > +} > + > +.cui-widget-panel.cui-widget-panelWithFooter > .panel-arrowcontainer > .panel-arrowcontent { > + padding: 4px 0 0; nit: padding-bottom: 0; At first I was confused why this was even a separate rule...

I had to take a different approach wrt the bookmark menus; secondary level sub-menus were given the subviewbutton style as well, which is not what we want (yet).

Comment on attachment 8366605 [details] [diff] [review] Patch v1.7: adjust toolbar widget panel styling Review of attachment 8366605 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for catching that. This is looking great. r+ with the following addressed. ::: browser/base/content/browser.xul @@ +729,5 @@ > + mainLevel: 'subviewbutton' > + }, > + insertionPoint: '.panel-subview-footer' > + }); > + }" At this point, I think this should move to its own function on BookmarkingUI. ::: browser/components/places/content/browserPlacesViews.js @@ +89,5 @@ > + get options() this._options, > + set options(val) { > + if (!val) > + val = {}; > + if (this._options == val) nit: Why this? Different objects are never equal, and I'm not sure why there needs to be a check for the same object (by reference) being passed in, which your patch never does... @@ +370,5 @@ > let before = aBefore || aPopup._endMarker; > + > + if (element.localName == "menuitem" || element.localName == "menu") { > + let extraClasses = this.options.extraClasses; > + if (aPopup == this._rootElt && ("mainLevel" in extraClasses)) && typeof extraClasses.mainLevel == "string" @@ +372,5 @@ > + if (element.localName == "menuitem" || element.localName == "menu") { > + let extraClasses = this.options.extraClasses; > + if (aPopup == this._rootElt && ("mainLevel" in extraClasses)) > + element.classList.add(extraClasses.mainLevel); > + else if ("secondaryLevel" in extraClasses) Ditto @@ +1813,5 @@ > } > else { > button = document.createElement("toolbarbutton"); > + let className = "bookmark-item"; > + if ("mainLevel" in this.options.extraClasses) Ditto @@ +1814,5 @@ > else { > button = document.createElement("toolbarbutton"); > + let className = "bookmark-item"; > + if ("mainLevel" in this.options.extraClasses) > + className += " " + this.options.extraClasses.mainLevel; Why no classList here? :-) ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +452,4 @@ > } > > +#BMB_bookmarksPopup > .subviewbutton:not([disabled="true"]), > +#BMB_bookmarksPopup > .subviewbutton:not([disabled="true"]) { Eh? :-) Also, your previous patch had: .subviewbutton.bookmark-item { font-weight: normal; color: inherit; } I think that can be used here instead of the child selector. Or does specificity get the better of us again? :-( (of course, you have to split it up and add :not([disabled="true"]) for the color bit, but it's still nicer than this, IMO - but maybe I'm missing something?)

(In reply to :Gijs Kruitbosch from comment #36) > Also, your previous patch had: > > .subviewbutton.bookmark-item { > font-weight: normal; > color: inherit; > } > > I think that can be used here instead of the child selector. Or does > specificity get the better of us again? :-( Specificity... This goes borky when the bookmarks button is on the bookmarks toolbar. Why would you put it there? No clue, but it IS possible... so voilà!

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

Verified as fixed on Firefox 29 beta 4 using Windows 8.1 32bit, Windows 7 64bit, Windows XP 32bit, Ubuntu 12.04 32bit and Mac OS X 10.9.2.