419231 - The styling of the Customize Toolbar window could be improved

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The floating scrollbar in the toolbar customization palette doesn't really match window design on any platform, the scroll bar should touch the far right side of the window.

As a quick fix, we could just change the height of this panel to show all of the toolbar controls so the scrollbar doesn't appear by default.  Ideally we would make the panel taller and also get the scroll bar attached to the right side.

This is just a minor polish issue, but I'm requesting blocking to pick up a wanted+.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Screenshot of the floating scrollbar (before)

Comment 2

[Dave Townsend [:mossop]]

We can't make the panel big enough at all times given that extensions add content into this area. Making the scrollbar touch the far side should be an easy fix I think, only question is what colouring to make it look right with only the middle section scrolling I think.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>We can't make the panel big enough at all times

Yeah, I just mean so it isn't scrolling with a fresh install and no extensions.

>only question is what colouring to make it look right with
>only the middle section scrolling I think.

It would be more native overall (especially on OS X and Vista) to keep the middle section the same color, even though this is a little strange when the scroll bars show up.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Should be an easy height fix to the dialog for the default case. Or get rid of cut/copy/paste .. :)

Comment 5

[Ryan A. C.]

>Making the scrollbar touch the far side should be an easy fix I think

That actually makes it look even weirder on windows IMO. Personally I think customize toolbar as a whole looks a little odd on windows. The closes precedent is the "add gadgets" on the vista sidebar (which btw uses pages instead of a scrollbar). Of course it does a great job of showing off glass, but doesn't translate well to a non-glass window.

Comment 6

[Ryan A. C.]

Attachment: A slightly more normal windows look - mockup

So, as I said in the previous comment, the Add Gadget is the closest thing to a precedent as we have on this thing. This is what our customize toolbar would approximately look like with that style. BTW, should this thing really have the windows chrome on it? perhaps it should just be a floating panel like the bookmarks/security.
What's the background color? a 50% opacity white box over top of the normal window... as far as I can tell that's more-or-less what Microsoft did on the Add Gadget dialog when I set it to classic mode (more accurately, they seem to lighten the color value with HSL)
I kept the scroll bar rather than inventing some page X of X widget just for use in this one little thing, and it doesn't look too bad off like that (although it does look a little better without it).

This style looks pretty decent in OS X too:
http://img90.imageshack.us/img90/3530/customizetoolbarmacdq5.png
I don't know if there's an HIG guideline that says the <hr> should be before done, but it looks funny with it there.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

This is fixed in that we now no longer ship with enough buttons for the scroll bar to show up by default.  However, if an extension adds an icon then we are back to a funky floating scroll bar, right?

Comment 8

[Ian Spence]

Attachment: Shows my solution in Linux

I added the following to my userChrome.css a while ago and I think it works well.

#palette-box {
  -moz-appearance: listbox !important;
  margin-left: 0px !important;
  margin-right: 0px !important;
  color: -moz-FieldText !important;
}

#palette-box * {
  color: -moz-FieldText !important;
}

This is only for linux (as I don't know if Windows or Mac have -moz-appearance: listbox;)  Attached is a preview

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

uiwanted: need a mockup

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: screenshot with no margin or padding

So, it turns out we can get rid of the margin and padding to make it go all the way to the right.  It still looks bad in my opinion though because we have a floating scrollbar.

Perhaps, at least on mac, making it look more like the identify pop up and the add bookmark pop up with a white box where the buttons get dragged from.  This is just me rambling though...

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Secondary UI, and I think a floating scroll bar probably easily identifiable as a problem, although amazingly enough not that many people have complained about it so far and it has been there for awhile.

Comment 12

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

Attachment: Patch for bug 419231

Comment 13

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

Attachment: Screenshot of patch

This is a screenshot of the patch.

Comment 14

[:Margaret Leibovic]

Comment on attachment 572234 [details] [diff] [review]
Patch for bug 419231

Looks good to me (and I tested on OS X).

Comment 15

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

Attachment: Patch for bug 419231

Thanks for the feedback Margaret. I've updated the patch to allow more than four items in a column as well as try to align the items in a better grid.

The vertical alignment within rows isn't perfect, but I don't think it is worth spending more time on.

Comment 16

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

Attachment: Screenshot of patch

Comment 17

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

Attachment: Patch for bug 419231

This patch cleans up the Customize Toolbar window so the toolbar items align nicely in a grid. Screenshot coming soon.

Comment 18

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

Attachment: Screenshot of patch

Here is a screenshot of the patch, now updated with the labels of the toolbar items vertically-aligned on the baseline.

Comment 19

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 574496 [details]
Screenshot of patch

Yay!

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 574495 [details] [diff] [review]
Patch for bug 419231

> #palette-box {
>-  overflow: auto; 
>-  margin: 0px 15px 10px 15px;
>+  display: block;
>+  -moz-appearance: listbox;
>+  -moz-box-flex: 1;

display: block; and -moz-box-flex: 1;? How's that going to work?

I don't think you should use -moz-appearance: listbox; in a non-theme file...

>+#palette-box > toolbarpaletteitem {
>   padding-top: 8px;
>   padding-bottom: 8px;
>+  width: 100px;
>+  height: 75px;
>+  overflow: hidden;
>+  display: table-cell;
> }

This looks like it's going to cut off strings longer than 100px.

Comment 21

[Neil Deakin]

Comment on attachment 574495 [details] [diff] [review]
Patch for bug 419231

> #palette-box {
>-  overflow: auto; 
>-  margin: 0px 15px 10px 15px;
>+  display: block;
>+  -moz-appearance: listbox;
>+  -moz-box-flex: 1;
>+  overflow: auto;
>+  margin: 0 0 10px;
>+  min-height: 3em;

The -moz-appearance should go in the platform specific theme's customizeToolbar.css

The flex should be removed and put back in the xul file as it was.
 
>-#palette-box > hbox > toolbarpaletteitem {
>+#palette-box > toolbarpaletteitem {
>   padding-top: 8px;
>   padding-bottom: 8px;
>+  width: 100px;
>+  height: 75px;
>+  overflow: hidden;
>+  display: table-cell;

table-cell? There are no tables here.


> }
> 
>-#palette-box > hbox {
>-  min-height: 8em;
>+.toolbarpaletteitem-box {
>+  -moz-box-pack: center;
>+  -moz-box-align: center;
>+  -moz-box-flex: 1;
>+  width: 100px;
>+}

The pack/align/flex should go in the xul/xbl file, but I can't tell what's expected as it would be inside a 'table-cell' (which should ignore the flex anyway)


>+
>+#separator {
>+  height: 30px;
>+}

Are you using this somewhere?


> /**
>  * Creates a new palette item for a cloned template node and
>  * adds it to the last slot in the palette.
>  */
> function appendPaletteItem(aItem)
> {
>   var paletteBox = document.getElementById("palette-box");
>-  var lastRow = paletteBox.lastChild;
>   var lastSpacer = lastRow.lastChild;

You've removed 'lastRow' here but then used it in the next line.


>+  if (lastSpacer.localName != "spacer")
>+    wrapPaletteItem(aItem, paletteBox, null);
>+  else
>+    wrapPaletteItem(aItem, paletteBox, null);

Doesn't seem to be a need for the condition as both cases do the same thing.


> }
> 
> /**
>  * Makes sure that an item that has been cloned from a template
>  * is stripped of any attributes that may adversely affect its
>  * appearance in the palette.
>  */
> function cleanUpItemForPalette(aItem, aWrapper)

>-  <vbox flex="1" id="palette-box"
>+  <hbox id="palette-box"

This was fine as is.


On my Mac, three rows of customized buttons appear, but that is slightly taller than the available space,
causing a scrollbar to appear. It would be a good idea to reduce the gap between the rows slightly to avoid
this. Also, the flexible space image is scrunched up in the topleft corner of a box.

Comment 22

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

Attachment: Patch for bug 419231 v2

(In reply to Dão Gottwald [:dao] from comment #20)
> Comment on attachment 574495 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 419231
> 
> > #palette-box {
> >-  overflow: auto; 
> >-  margin: 0px 15px 10px 15px;
> >+  display: block;
> >+  -moz-appearance: listbox;
> >+  -moz-box-flex: 1;
> 
> display: block; and -moz-box-flex: 1;? How's that going to work?

The |display: block;| allows the children to flow, and the |-moz-box-flex: 1;| allows the box to adjust vertically to when the window is resized.

> I don't think you should use -moz-appearance: listbox; in a non-theme file...

Thanks, I've moved the |-moz-appearance: listbox;| to the theme-specific files.

> >+#palette-box > toolbarpaletteitem {
> >   padding-top: 8px;
> >   padding-bottom: 8px;
> >+  width: 100px;
> >+  height: 75px;
> >+  overflow: hidden;
> >+  display: table-cell;
> > }
> 
> This looks like it's going to cut off strings longer than 100px.

Strings longer than 100px will get elided, but I'm not sure what else we could do here. The previous implementation suffered from this issue too, just in a different way. Previously, each full row would have four items regardless of the width of the window. If the window was narrow, then the palette items would overlap each other.

(In reply to Neil Deakin from comment #21)
> Comment on attachment 574495 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 419231
> 
> > #palette-box {
> >-  overflow: auto; 
> >-  margin: 0px 15px 10px 15px;
> >+  display: block;
> >+  -moz-appearance: listbox;
> >+  -moz-box-flex: 1;
> >+  overflow: auto;
> >+  margin: 0 0 10px;
> >+  min-height: 3em;
> 
> The -moz-appearance should go in the platform specific theme's
> customizeToolbar.css

Done. There isn't a theme file for gnomestripe. Should we add one or leave it as-is?

> The flex should be removed and put back in the xul file as it was.

OK, I'm still trying to figure out when to use the CSS property instead of the XUL attribute. What is this convention based on? Is it just something that started in XUL and by convention we have decided to keep it this way for consistency?

> >-#palette-box > hbox > toolbarpaletteitem {
> >+#palette-box > toolbarpaletteitem {
> >   padding-top: 8px;
> >   padding-bottom: 8px;
> >+  width: 100px;
> >+  height: 75px;
> >+  overflow: hidden;
> >+  display: table-cell;
> 
> table-cell? There are no tables here.

I was trying to use |table-cell| for vertical-alignment, but it turns out that I could use |inline-block| and get better results anyways. (thanks fryn!)

> 
> > }
> > 
> >-#palette-box > hbox {
> >-  min-height: 8em;
> >+.toolbarpaletteitem-box {
> >+  -moz-box-pack: center;
> >+  -moz-box-align: center;
> >+  -moz-box-flex: 1;
> >+  width: 100px;
> >+}
> 
> The pack/align/flex should go in the xul/xbl file, but I can't tell what's
> expected as it would be inside a 'table-cell' (which should ignore the flex
> anyway)

This rule is applied to the anonymous content and as such the XUL file doesn't already have access to these elements. I could query for them within the file, but it seems unnecessary if we can do it simpler using a CSS selector.

> 
> >+
> >+#separator {
> >+  height: 30px;
> >+}
> 
> Are you using this somewhere?

Without this rule, the separator in the palette has a height of zero. Maybe this should be moved to the theme-specific CSS files?

> > /**
> >  * Creates a new palette item for a cloned template node and
> >  * adds it to the last slot in the palette.
> >  */
> > function appendPaletteItem(aItem)
> > {
> >   var paletteBox = document.getElementById("palette-box");
> >-  var lastRow = paletteBox.lastChild;
> >   var lastSpacer = lastRow.lastChild;
> 
> You've removed 'lastRow' here but then used it in the next line.

Yeah, I'm stupid. I've fixed this.

> >+  if (lastSpacer.localName != "spacer")
> >+    wrapPaletteItem(aItem, paletteBox, null);
> >+  else
> >+    wrapPaletteItem(aItem, paletteBox, null);
> 
> Doesn't seem to be a need for the condition as both cases do the same thing.

Another oversight, I've fixed it now.

> > }
> > 
> > /**
> >  * Makes sure that an item that has been cloned from a template
> >  * is stripped of any attributes that may adversely affect its
> >  * appearance in the palette.
> >  */
> > function cleanUpItemForPalette(aItem, aWrapper)
> 
> >-  <vbox flex="1" id="palette-box"
> >+  <hbox id="palette-box"
> 
> This was fine as is.
> 
> 
> On my Mac, three rows of customized buttons appear, but that is slightly
> taller than the available space,
> causing a scrollbar to appear. It would be a good idea to reduce the gap
> between the rows slightly to avoid
> this. Also, the flexible space image is scrunched up in the topleft corner
> of a box.

I have fixed the flexible space image being scrunched up in the topleft corner of the box. We can try to make the items shorter, but we may risk some add-ons that get their image clipped.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 575013 [details] [diff] [review]
Patch for bug 419231 v2

* "Activity Indicator" and "Zoom Controls" are cropped over here (Linux). There should at least be tooltips exposing the full labels.

* The "Bookmarks Toolbar Items" looks completely broken. I'll attach a screenshot.

* The margins should be set in the theme files.

* The fact that you need to set a height on the separator seems worrisome. Can you make it so that it so that the items automatically take up the space again?

Comment 24

[Dão Gottwald [:dao]]

Attachment: Bookmarks Toolbar Items

Comment 25

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

Attachment: Patch for bug 419231 v2.1

(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 575013 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 419231 v2
> 
> * "Activity Indicator" and "Zoom Controls" are cropped over here (Linux).
> There should at least be tooltips exposing the full labels.

I've added tooltips to all the items in the palette now.

> * The "Bookmarks Toolbar Items" looks completely broken. I'll attach a
> screenshot.

The "Bookmarks Toolbar Items" was broken previous to this bug, but I have fixed it in this version of the patch. The toolbar palette item was trying to show the |#personal-bookmarks| toolbar within the item.
 
> * The margins should be set in the theme files.

I've moved the margins to the theme files. Please let me know if you think other properties should be moved to the theme files.
 
> * The fact that you need to set a height on the separator seems worrisome.
> Can you make it so that it so that the items automatically take up the space
> again?

I've fixed this by removing the |-moz-box-align| property on the |.toolbarpaletteitem-box|.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 579415 [details] [diff] [review]
Patch for bug 419231 v2.1

>--- a/toolkit/content/customizeToolbar.css
>+++ b/toolkit/content/customizeToolbar.css

>+/* Hide the personal-bookmarks section of the bookmarks toolbar when it is
>+   placed in the palette. */
>+#personal-bookmarks {
>+  display: none;
>+}

This doesn't belong in toolkit/.

>--- a/toolkit/themes/winstripe/global/customizeToolbar.css
>+++ b/toolkit/themes/winstripe/global/customizeToolbar.css

>+.bookmarks-toolbar-customize {
>+  list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png") !important;
>+}
>+
>+.bookmarks-toolbar-customize > .toolbarbutton-text {
>+  display: none !important;
>+}

ditto

Comment 27

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

Attachment: Patch for bug 419231 v2.2

Comment 28

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

(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 579415 [details] [diff] [review]
> Patch for bug 419231 v2.1
> 
> >--- a/toolkit/content/customizeToolbar.css
> >+++ b/toolkit/content/customizeToolbar.css
> 
> >+/* Hide the personal-bookmarks section of the bookmarks toolbar when it is
> >+   placed in the palette. */
> >+#personal-bookmarks {
> >+  display: none;
> >+}
> 
> This doesn't belong in toolkit/.

I have moved this to /browser/

> 
> >--- a/toolkit/themes/winstripe/global/customizeToolbar.css
> >+++ b/toolkit/themes/winstripe/global/customizeToolbar.css
> 
> >+.bookmarks-toolbar-customize {
> >+  list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png") !important;
> >+}
> >+
> >+.bookmarks-toolbar-customize > .toolbarbutton-text {
> >+  display: none !important;
> >+}

These weren't necessary and so I have removed them.

Comment 29

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

Attachment: Screenshot of patch

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 582139 [details] [diff] [review]
Patch for bug 419231 v2.2

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#wrapper-personal-bookmarks[place="palette"] #personal-bookmarks {
>+  display: none;
> }

Please use the child selector.

> .bookmarks-toolbar-customize {
>   max-width: 15em !important;
>   list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png") !important;
> }

The above seems to make bookmarks-toolbar-customize obsolete; it won't be displayed. Apparently there's a bunch of code that should be removed.

>--- a/toolkit/content/customizeToolbar.css
>+++ b/toolkit/content/customizeToolbar.css

>  Contributor(s):
>    David Hyatt ([e-mail])
>    Blake Ross ([e-mail])
>+   Jared Wein ([e-mail])

nit: use <>, no need to be consistent with the above email addresses (alternatively, just fix them as well).

>--- a/toolkit/themes/pinstripe/global/customizeToolbar.css
>+++ b/toolkit/themes/pinstripe/global/customizeToolbar.css

>+.toolbarpaletteitem-box[type="spring"] {
>+  background-position: center;
>+  margin-left: 8px;
>+  margin-right: 8px;
>+}
>+
>+.toolbarpaletteitem-box[type="spacer"],
>+.toolbarpaletteitem-box[type="spring"] {
>+  margin-top: 0;
> }

This looks like it belongs in toolbar.css, with [places="palette"].

Comment 31

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

Attachment: Patch for bug 419231 v2.3

(In reply to Dão Gottwald [:dao] from comment #30)
> Comment on attachment 582139 [details] [diff] [review]
> Patch for bug 419231 v2.2
> 
> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> >+#wrapper-personal-bookmarks[place="palette"] #personal-bookmarks {
> >+  display: none;
> > }
> 
> Please use the child selector.

Fixed.

> > .bookmarks-toolbar-customize {
> >   max-width: 15em !important;
> >   list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png") !important;
> > }
> 
> The above seems to make bookmarks-toolbar-customize obsolete; it won't be
> displayed. Apparently there's a bunch of code that should be removed.

With that rule removed, the bookmarks-toolbar shows the generic "missing-favicon icon" when placed in the toolbar but in Customize mode. 

> >--- a/toolkit/content/customizeToolbar.css
> >+++ b/toolkit/content/customizeToolbar.css
> 
> >  Contributor(s):
> >    David Hyatt ([e-mail])
> >    Blake Ross ([e-mail])
> >+   Jared Wein ([e-mail])
> 
> nit: use <>, no need to be consistent with the above email addresses
> (alternatively, just fix them as well).
> 
> >--- a/toolkit/themes/pinstripe/global/customizeToolbar.css
> >+++ b/toolkit/themes/pinstripe/global/customizeToolbar.css
> 
> >+.toolbarpaletteitem-box[type="spring"] {
> >+  background-position: center;
> >+  margin-left: 8px;
> >+  margin-right: 8px;
> >+}
> >+
> >+.toolbarpaletteitem-box[type="spacer"],
> >+.toolbarpaletteitem-box[type="spring"] {
> >+  margin-top: 0;
> > }
> 
> This looks like it belongs in toolbar.css, with [places="palette"].

Fixed.

Comment 32

[Dão Gottwald [:dao]]

> With that rule removed, the bookmarks-toolbar shows the generic
> "missing-favicon icon" when placed in the toolbar but in Customize mode.

Ok, so... can you update http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.css#10 to use [place=toolbar] (and child selectors, while you're at it)?

Comment 33

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

Attachment: Patch for bug 419231 v2.4

Updated the selector in places.css to use [place="toolbar"] and child selectors.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 583346 [details] [diff] [review]
Patch for bug 419231 v2.4

>+#wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
>+  display: none;
> }

Is this still needed?

Comment 35

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

(In reply to Dão Gottwald [:dao] from comment #34)
> Comment on attachment 583346 [details] [diff] [review]
> Patch for bug 419231 v2.4
> 
> >+#wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
> >+  display: none;
> > }
> 
> Is this still needed?

Yes, that's still needed. Without it, the personal-bookmarks toolbar tries to display in the Customize Toolbar dialog. This is because there is a rule for |*| in chrome://global/content/xul.css that sets |display: -moz-box;|.

Comment 36

[Dão Gottwald [:dao]]

This has nothing to do with xul.css. browser/components/places/content/places.css should be hiding it, but apparently it doesn't affect customizeToolbar.xul. So we need to either change that or move those styles to browser/content/browser.css.

Comment 37

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

Attachment: Patch for bug 419231 v2.4

I've moved those styles to /browser/base/content/browser.css

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 583383 [details] [diff] [review]
Patch for bug 419231 v2.4

>--- a/browser/components/places/content/places.css
>+++ b/browser/components/places/content/places.css
>@@ -2,17 +2,17 @@ tree[type="places"] {
>   -moz-binding: url("chrome://browser/content/places/tree.xml#places-tree");
> }
> 
> .bookmarks-toolbar-customize,
> toolbarpaletteitem #PlacesToolbarItems {
>   display: none;
> }
> 
>-toolbarpaletteitem .bookmarks-toolbar-customize {
>+toolbarpaletteitem[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
>   display: -moz-box;
> }

Move this to browser/base/content/browser.css as well and merge it with what you already have there.

Comment 39

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

Attachment: Patch for bug 419231 v2.5

(In reply to Dão Gottwald [:dao] from comment #38)
> Comment on attachment 583383 [details] [diff] [review]
> Patch for bug 419231 v2.4
> 
> >--- a/browser/components/places/content/places.css
> >+++ b/browser/components/places/content/places.css
> >@@ -2,17 +2,17 @@ tree[type="places"] {
> >   -moz-binding: url("chrome://browser/content/places/tree.xml#places-tree");
> > }
> > 
> > .bookmarks-toolbar-customize,
> > toolbarpaletteitem #PlacesToolbarItems {
> >   display: none;
> > }
> > 
> >-toolbarpaletteitem .bookmarks-toolbar-customize {
> >+toolbarpaletteitem[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
> >   display: -moz-box;
> > }
> 
> Move this to browser/base/content/browser.css as well and merge it with what
> you already have there.

I have moved this to /browser/base/content/browser.css but I'm not sure what is meant by "merging it". Can you please describe this goal a little more?

Comment 40

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

Would you like it changed like so, such that the selector for .bookmarks-toolbar-customize does not need to filter based on the toolbarpaletteitem[place="toolbar"]?

> toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"],
> #wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
>   display: none;
> }
> 
> #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
>   display: -moz-box;
> }

Or would you like it such that the element name is not used in the selector, like so?

> toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"],
> #wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
>   display: none;
> }
> 
> #wrapper-personal-bookmarks[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
>   display: -moz-box;
> }

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 583416 [details] [diff] [review]
Patch for bug 419231 v2.5

>--- a/browser/components/places/content/places.css
>+++ b/browser/components/places/content/places.css
>@@ -7,10 +7,6 @@ toolbarpaletteitem #PlacesToolbarItems {
>   display: none;
> }
> 
>-toolbarpaletteitem .bookmarks-toolbar-customize {
>-  display: -moz-box;
>-}

Move the rule above this as well. (This patch has insufficient context.)

Comment 42

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jwein and :jaws] from comment #39)
> I have moved this to /browser/base/content/browser.css but I'm not sure what
> is meant by "merging it". Can you please describe this goal a little more?

--- a/browser/base/content/browser.css
+++ b/browser/base/content/browser.css

-toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"] {
+toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"],
+#wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
   display: none;
 }

should merge with:

--- a/browser/components/places/content/places.css
+++ b/browser/components/places/content/places.css

 .bookmarks-toolbar-customize,
 toolbarpaletteitem #PlacesToolbarItems {
   display: none;
 }
 
-toolbarpaletteitem .bookmarks-toolbar-customize {
+toolbarpaletteitem[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
   display: -moz-box;
 }

Comment 43

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

Attachment: Patch for bug 419231 v2.6

Thanks for the clarification and your patience.

I have moved the other rules to /browser/base/content/browser.css and removed the rule for |.bookmarks-toolbar-customize| in the process since it was made obsolete since it's nested inside of the #PlacesToolbar which is already |display:none|.

I have also fixed the 8 lines of context in the patch.

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 583430 [details] [diff] [review]
Patch for bug 419231 v2.6

>-toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"] {
>+toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"],

I realize you're only extending the existing selector, but please use classes, ids, etc.

>+ {
>+  display: none;
>+}

Something went wrong here.

Comment 45

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

Attachment: Patch for bug 419231 v2.7

(In reply to Dão Gottwald [:dao] from comment #44)
> Comment on attachment 583430 [details] [diff] [review]
> Patch for bug 419231 v2.6
> 
> >-toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"] {
> >+toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"],
> 
> I realize you're only extending the existing selector, but please use
> classes, ids, etc.

It doesn't look like that selector is actually being applied anywhere. The only toolbarpaletteitem that has a toolbaritem child with an hbox child is the #wrapper-personal-bookmarks but that doesn't have a [type="places"], so I have removed this selector.
 
> >+ {
> >+  display: none;
> >+}
> 
> Something went wrong here.

:facepalm: I've removed that in this latest patch.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 583435 [details] [diff] [review]
Patch for bug 419231 v2.7

>+#wrapper-personal-bookmarks[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
>+  display: -moz-box;
>+}

This looks like a no-op, because there's no longer any code hiding .bookmarks-toolbar-customize. However, there *should* be such code, as we need to hide .bookmarks-toolbar-customize on the toolbar while not customizing.

Comment 47

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

Attachment: Patch for bug 419231 v2.8

I have added back the selector for .bookmarks-toolbar-customize as follows:

> +.bookmarks-toolbar-customize,
>  #wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks,
>  #wrapper-personal-bookmarks > #personal-bookmarks > #PlacesToolbar > hbox > #PlacesToolbarItems {
>    display: none;
>  }

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 583437 [details] [diff] [review]
Patch for bug 419231 v2.8

>+.bookmarks-toolbar-customize,
>+#wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks,
>+#wrapper-personal-bookmarks > #personal-bookmarks > #PlacesToolbar > hbox > #PlacesToolbarItems {
>   display: none;
> }
> 
>+#wrapper-personal-bookmarks[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
>+  display: -moz-box;
>+}

So at this point, what's the use of #wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks { display: none }?

Comment 49

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

Attachment: Patch for bug 419231 v2.9

(In reply to Dão Gottwald [:dao] from comment #48)
> Comment on attachment 583437 [details] [diff] [review]
> Patch for bug 419231 v2.8
> 
> >+.bookmarks-toolbar-customize,
> >+#wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks,
> >+#wrapper-personal-bookmarks > #personal-bookmarks > #PlacesToolbar > hbox > #PlacesToolbarItems {
> >   display: none;
> > }
> > 
> >+#wrapper-personal-bookmarks[place="toolbar"] > #personal-bookmarks > #PlacesToolbar > .bookmarks-toolbar-customize {
> >+  display: -moz-box;
> >+}
> 
> So at this point, what's the use of
> #wrapper-personal-bookmarks[place="palette"] > #personal-bookmarks {
> display: none }?

Sigh... there is no use for that now. I've updated the patch to remove that line.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 583441 [details] [diff] [review]
Patch for bug 419231 v2.9

> var gToolboxDocument = null;
> var gToolbox = null;
> var gCurrentDragOverItem = null;
> var gToolboxChanged = false;
> var gToolboxSheet = false;

>+function wrapPaletteItem(aPaletteItem, aPaletteBox)

Instead of passing palettebBox around, would it make sense to make it global like those other elements?

Comment 51

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

Attachment: Patch for bug 419231 v2.10

Moved paletteBox to a global named gPaletteBox.

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 583636 [details] [diff] [review]
Patch for bug 419231 v2.10

>--- a/toolkit/content/customizeToolbar.js
>+++ b/toolkit/content/customizeToolbar.js
>@@ -18,38 +18,38 @@
> # David Hyatt.
> # Portions created by the Initial Developer are Copyright (C) 2002
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   David Hyatt ([e-mail])
> #   Blake Ross ([e-mail])
> #   Joe Hewitt ([e-mail])
>+#   Jared Wein ([e-mail])

nit: <>

>+      wrapPaletteItem(document.importNode(wrapper.firstChild, true), gPaletteBox);

stray second argument

Comment 53

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

Attachment: Patch for checkin

Comment 54

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

https://hg.mozilla.org/integration/fx-team/rev/6587497e7e96

Comment 55

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6587497e7e96

Comment 57

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

Adding the dev-doc-needed and addon-compat keywords to this bug since this patch changes the allowed size of the toolbarpaletteitem in the Customize Toolbar dialog to 110px wide by 94px tall.

I'm not sure if dev-doc-needed is necessary, but I will let someone from DevMo make that decision.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jared Wein [:jwein and :jaws] from comment #57)
> Adding the dev-doc-needed and addon-compat keywords to this bug since this
> patch changes the allowed size of the toolbarpaletteitem in the Customize
> Toolbar dialog to 110px wide by 94px tall.

I feel that also localizers have to be made aware of this change. CC'ing Pike.

Comment 59

[Dão Gottwald [:dao]]

(In reply to Henrik Skupin (:whimboo) from comment #58)
> (In reply to Jared Wein [:jwein and :jaws] from comment #57)
> > Adding the dev-doc-needed and addon-compat keywords to this bug since this
> > patch changes the allowed size of the toolbarpaletteitem in the Customize
> > Toolbar dialog to 110px wide by 94px tall.
> 
> I feel that also localizers have to be made aware of this change. CC'ing
> Pike.

Nope. What's relevant to locales is bug 714277.