Closed Bug 419231 Opened 12 years ago Closed 8 years ago

The styling of the Customize Toolbar window could be improved

Categories

(Toolkit :: Toolbars and Toolbar Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: faaborg, Assigned: jaws)

References

Details

(Keywords: addon-compat, polish, Whiteboard: [polish-easy][polish-visual][polish-p2])

Attachments

(5 files, 20 obsolete files)

58.33 KB, image/png
Details
49.93 KB, image/png
Details
458.01 KB, image/png
Details
20.33 KB, patch
dao
: review+
Details | Diff | Splinter Review
20.50 KB, patch
Details | Diff | Splinter Review
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+.
Flags: blocking-firefox3?
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.
>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.
Should be an easy height fix to the dialog for the default case. Or get rid of cut/copy/paste .. :)
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Blocks: 425582
Blocks: 405605
Depends on: 430217
>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.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
No longer blocks: 405605
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image Shows my solution in Linux (obsolete) —
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
No longer depends on: 430217
Summary: Floating scrollbar in the toolbar customization palette is funky, window isn't tall enough → Floating scrollbar in the toolbar customization palette is funky
Whiteboard: [polish-easy][polish-visual]
uiwanted: need a mockup
Keywords: uiwanted
Attached image screenshot with no margin or padding (obsolete) —
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...
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.
Whiteboard: [polish-easy][polish-visual] → [polish-easy][polish-visual][polish-p2]
Attached patch Patch for bug 419231 (obsolete) — Splinter Review
Assignee: nobody → jwein
Status: REOPENED → ASSIGNED
Attachment #572234 - Flags: feedback?(margaret.leibovic)
Attached image Screenshot of patch (obsolete) —
This is a screenshot of the patch.
Attachment #572235 - Flags: ui-review?(limi)
Target Milestone: Firefox 3 → ---
Comment on attachment 572234 [details] [diff] [review]
Patch for bug 419231

Looks good to me (and I tested on OS X).
Attachment #572234 - Flags: feedback?(margaret.leibovic) → feedback+
Attached patch Patch for bug 419231 (obsolete) — Splinter Review
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.
Attachment #572235 - Attachment is obsolete: true
Attachment #572736 - Flags: feedback?(margaret.leibovic)
Attachment #572235 - Flags: ui-review?(limi)
Attached image Screenshot of patch (obsolete) —
Attachment #574467 - Flags: ui-review?(limi)
Attached patch Patch for bug 419231 (obsolete) — Splinter Review
This patch cleans up the Customize Toolbar window so the toolbar items align nicely in a grid. Screenshot coming soon.
Attachment #572234 - Attachment is obsolete: true
Attachment #572736 - Attachment is obsolete: true
Attachment #574495 - Flags: review?(enndeakin)
Attachment #572736 - Flags: feedback?(margaret.leibovic)
Attached image Screenshot of patch (obsolete) —
Here is a screenshot of the patch, now updated with the labels of the toolbar items vertically-aligned on the baseline.
Attachment #317182 - Attachment is obsolete: true
Attachment #317955 - Attachment is obsolete: true
Attachment #351585 - Attachment is obsolete: true
Attachment #574467 - Attachment is obsolete: true
Attachment #574496 - Flags: ui-review?(limi)
Attachment #574467 - Flags: ui-review?(limi)
Attachment #305260 - Attachment description: Screenshot of the floating scrollbar → Screenshot of the floating scrollbar (before)
Comment on attachment 574496 [details]
Screenshot of patch

Yay!
Attachment #574496 - Flags: ui-review?(limi) → ui-review+
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.
Attachment #574495 - Flags: review-
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.
Attachment #574495 - Flags: review?(enndeakin) → review-
Attached patch Patch for bug 419231 v2 (obsolete) — Splinter Review
(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.
Attachment #574495 - Attachment is obsolete: true
Attachment #575013 - Flags: review?(enndeakin)
Attachment #575013 - Flags: review?(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?
Attachment #575013 - Flags: review?(dao) → review-
Component: Toolbars → Toolbars and Toolbar Customization
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Product: Firefox → Toolkit
QA Contact: toolbars → toolbars
Attached patch Patch for bug 419231 v2.1 (obsolete) — Splinter Review
(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|.
Attachment #575013 - Attachment is obsolete: true
Attachment #579415 - Flags: review?(enndeakin)
Attachment #579415 - Flags: review?(dao)
Attachment #575013 - Flags: review?(enndeakin)
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
Attachment #579415 - Flags: review?(dao) → review-
Attached patch Patch for bug 419231 v2.2 (obsolete) — Splinter Review
Attachment #582139 - Flags: review?(dao)
(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.
Attachment #579415 - Attachment is obsolete: true
Attachment #579415 - Flags: review?(enndeakin)
Attachment #582139 - Flags: review?(enndeakin)
Attached image Screenshot of patch
Attachment #574496 - Attachment is obsolete: true
Attachment #582142 - Attachment is patch: false
Attachment #582142 - Attachment mime type: text/plain → image/png
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 (hyatt@apple.com)
>    Blake Ross (blaker@netscape.com)
>+   Jared Wein (jwein@mozilla.com)

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"].
Attachment #582139 - Flags: review?(dao) → review-
Attached patch Patch for bug 419231 v2.3 (obsolete) — Splinter Review
(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 (hyatt@apple.com)
> >    Blake Ross (blaker@netscape.com)
> >+   Jared Wein (jwein@mozilla.com)
> 
> 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.
Attachment #582139 - Attachment is obsolete: true
Attachment #582722 - Flags: review?(dao)
Attachment #582139 - Flags: review?(enndeakin)
> 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)?
Attached patch Patch for bug 419231 v2.4 (obsolete) — Splinter Review
Updated the selector in places.css to use [place="toolbar"] and child selectors.
Attachment #582722 - Attachment is obsolete: true
Attachment #583346 - Flags: review?(dao)
Attachment #582722 - Flags: review?(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?
(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;|.
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.
Attached patch Patch for bug 419231 v2.4 (obsolete) — Splinter Review
I've moved those styles to /browser/base/content/browser.css
Attachment #583346 - Attachment is obsolete: true
Attachment #583383 - Flags: review?(dao)
Attachment #583346 - Flags: review?(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.
Attachment #583383 - Flags: review?(dao) → review-
Attached patch Patch for bug 419231 v2.5 (obsolete) — Splinter Review
(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?
Attachment #583383 - Attachment is obsolete: true
Attachment #583416 - Flags: review?(dao)
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 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.)
Attachment #583416 - Flags: review?(dao) → review-
(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;
 }
Attached patch Patch for bug 419231 v2.6 (obsolete) — Splinter Review
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.
Attachment #583416 - Attachment is obsolete: true
Attachment #583430 - Flags: review?(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.
Attachment #583430 - Flags: review?(dao) → review-
Attached patch Patch for bug 419231 v2.7 (obsolete) — Splinter Review
(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.
Attachment #583430 - Attachment is obsolete: true
Attachment #583435 - Flags: review?(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.
Attachment #583435 - Flags: review?(dao) → review-
Attached patch Patch for bug 419231 v2.8 (obsolete) — Splinter Review
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;
>  }
Attachment #583435 - Attachment is obsolete: true
Attachment #583437 - Flags: review?(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 }?
Attached patch Patch for bug 419231 v2.9 (obsolete) — Splinter Review
(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.
Attachment #583437 - Attachment is obsolete: true
Attachment #583441 - Flags: review?(dao)
Attachment #583437 - Flags: review?(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?
Moved paletteBox to a global named gPaletteBox.
Attachment #583441 - Attachment is obsolete: true
Attachment #583636 - Flags: review?(dao)
Attachment #583441 - Flags: review?(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 (hyatt@apple.com)
> #   Blake Ross (blaker@netscape.com)
> #   Joe Hewitt (hewitt@netscape.com)
>+#   Jared Wein (jwein@mozilla.com)

nit: <>

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

stray second argument
Attachment #583636 - Flags: review?(dao) → review+
https://hg.mozilla.org/integration/fx-team/rev/6587497e7e96
Whiteboard: [polish-easy][polish-visual][polish-p2] → [polish-easy][polish-visual][polish-p2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6587497e7e96
Status: ASSIGNED → RESOLVED
Closed: 12 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [polish-easy][polish-visual][polish-p2][fixed-in-fx-team] → [polish-easy][polish-visual][polish-p2]
Target Milestone: --- → mozilla12
Depends on: 714277
Keywords: uiwanted
Depends on: 714390
Duplicate of this bug: 625711
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.
Summary: Floating scrollbar in the toolbar customization palette is funky → The styling of the Customize Toolbar window could be improved
(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.
(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.
Duplicate of this bug: 256246
You need to log in before you can comment on or make changes to this bug.