963095 - Widgets with a panel placed in a toolbar are styled incorrectly

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

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.

Comment 1

[Mike de Boer [:mikedeboer]]

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!!

Comment 2

[u428464]

Attachment: Hover styling.png

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

Comment 3

[u428464]

Oups wrong bug

Comment 4

[u428464]

Attachment: Widgets and panels bug.png

Here it is. Both text issue and poor panel styling can be seen here.

The wrong attachment can be marked obsolete.

Comment 5

[u428464]

Whoops my bad the text issue is not subviews related. 

The "not so good" looking panels are though.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: don't show panel header labels when widget is placed in toolbar

Jared, would you like to take this one too?

Comment 7

[u428464]

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 8

[Jared Wein [:jaws] (please needinfo? me)]

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).

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: don't show panel header labels when widget is placed in toolbar

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 10

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 11

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

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.

Comment 12

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

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

Comment 13

[Mike de Boer [:mikedeboer]]

(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?

Comment 14

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

(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 :)

Comment 15

[u428464]

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

Comment 16

[Mike de Boer [:mikedeboer]]

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

Comment 17

[u428464]

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.

Comment 18

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

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

Comment 19

[u428464]

(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.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: 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?

Comment 21

[:Gijs (he/him)]

(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 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3: adjust toolbar widget panel styling

Comment 23

[:Gijs (he/him)]

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?

Comment 24

[Mike de Boer [:mikedeboer]]

(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.

Comment 25

[Tim Nguyen :ntim]

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

Comment 26

[:Gijs (he/him)]

(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?

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4: adjust toolbar widget panel styling

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?

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.5: adjust toolbar widget panel styling

meh, review is fine too ;)

Comment 29

[:Gijs (he/him)]

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?

Comment 30

[Mike de Boer [:mikedeboer]]

(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.

Comment 31

[:Gijs (he/him)]

(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 32

[:Gijs (he/him)]

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 33

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.6: adjust toolbar widget panel styling

Comment 34

[:Gijs (he/him)]

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...

Comment 35

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.7: adjust toolbar widget panel styling

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 36

[:Gijs (he/him)]

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?)

Comment 37

[Mike de Boer [:mikedeboer]]

(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à!

Comment 38

[Mike de Boer [:mikedeboer]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c175d9ca4939

Comment 40

[Bogdan Maris, Desktop QA]

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.