Widgets with a panel placed in a toolbar are styled incorrectly

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ge3k0s, Assigned: mikedeboer)

Tracking

Trunk
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 attachments, 8 obsolete attachments)

66.43 KB, image/png
Details
28.97 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

4 years ago
Blocks: 872617, 878546
Whiteboard: [Australis:P3]
No longer blocks: 872617
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!!
Flags: needinfo?(ge3k0s)
(Reporter)

Comment 2

4 years ago
Created attachment 8364408 [details]
Hover styling.png

I'm not sure anymore if it's really subviews related.
Flags: needinfo?(ge3k0s)
(Reporter)

Comment 3

4 years ago
Oups wrong bug
(Reporter)

Comment 4

4 years ago
Created attachment 8364412 [details]
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.
Attachment #8364408 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Widgets panels issues → Widgets with a panel placed in a toolbar are styled incorrectly
Assignee: nobody → mdeboer
(Reporter)

Comment 5

4 years ago
Whoops my bad the text issue is not subviews related. 

The "not so good" looking panels are though.
Created attachment 8364449 [details] [diff] [review]
Patch v1: don't show panel header labels when widget is placed in toolbar

Jared, would you like to take this one too?
Attachment #8364449 - Flags: review?(jaws)
(Reporter)

Comment 7

4 years ago
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).
Attachment #8364449 - Flags: review?(jaws)
Created attachment 8364474 [details] [diff] [review]
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...
Attachment #8364449 - Attachment is obsolete: true
Attachment #8364474 - Flags: feedback?(jaws)
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
Attachment #8364474 - Flags: feedback?(philipp)
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?
Flags: needinfo?(philipp)
(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 :)
Flags: needinfo?(philipp)
(Reporter)

Comment 15

4 years ago
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 ;)
Attachment #8364474 - Flags: feedback?(philipp)
Attachment #8364474 - Flags: feedback?(jaws)
(Reporter)

Comment 17

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

Comment 19

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

Comment 21

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

Updated

4 years ago
Attachment #8365011 - Flags: feedback?(gijskruitbosch+bugs)
Blocks: 963480
Created attachment 8365163 [details] [diff] [review]
Patch v1.3: adjust toolbar widget panel styling
Attachment #8365011 - Attachment is obsolete: true
Attachment #8365163 - Flags: review?(gijskruitbosch+bugs)

Comment 23

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

Comment 25

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

4 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Created attachment 8366059 [details] [diff] [review]
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?
Attachment #8365163 - Attachment is obsolete: true
Attachment #8366059 - Flags: feedback?(gijskruitbosch+bugs)
Created attachment 8366130 [details] [diff] [review]
Patch v1.5: adjust toolbar widget panel styling

meh, review is fine too ;)
Attachment #8366059 - Attachment is obsolete: true
Attachment #8366059 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8366130 - Flags: review?(gijskruitbosch+bugs)

Comment 29

4 years ago
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?
Attachment #8366059 - Flags: feedback+
(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

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

4 years ago
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...
Attachment #8366130 - Flags: review?(gijskruitbosch+bugs) → feedback+
Created attachment 8366303 [details] [diff] [review]
Patch v1.6: adjust toolbar widget panel styling
Attachment #8366130 - Attachment is obsolete: true
Attachment #8366303 - Flags: review?(gijskruitbosch+bugs)

Comment 34

4 years ago
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...
Attachment #8366303 - Flags: review?(gijskruitbosch+bugs) → review+
Created attachment 8366605 [details] [diff] [review]
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).
Attachment #8366303 - Attachment is obsolete: true
Attachment #8366605 - Flags: review?(gijskruitbosch+bugs)

Comment 36

4 years ago
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?)
Attachment #8366605 - Flags: review?(gijskruitbosch+bugs) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/c175d9ca4939
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

4 years ago
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.