The styling of the Customize Toolbar window could be improved

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Toolbars and Toolbar Customization
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: faaborg, Assigned: jaws)

Tracking

({addon-compat, polish})

Trunk
mozilla12
addon-compat, polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy][polish-visual][polish-p2])

Attachments

(5 attachments, 20 obsolete attachments)

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

Comment 1

9 years ago
Created attachment 305260 [details]
Screenshot of the floating scrollbar (before)
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.
(Reporter)

Comment 3

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

Updated

9 years ago
Blocks: 425582
(Reporter)

Updated

9 years ago
Blocks: 405605

Updated

9 years ago
Depends on: 430217

Comment 5

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

9 years ago
Created attachment 317182 [details]
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.

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: --- → Firefox 3

Updated

9 years ago
No longer blocks: 405605
(Reporter)

Comment 7

9 years ago
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 → ---

Comment 8

9 years ago
Created attachment 317955 [details]
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

Updated

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

Updated

9 years ago
Whiteboard: [polish-easy][polish-visual]
(Reporter)

Comment 9

9 years ago
uiwanted: need a mockup
Keywords: uiwanted
Created attachment 351585 [details]
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...
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]
Created attachment 572234 [details] [diff] [review]
Patch for bug 419231
Assignee: nobody → jwein
Status: REOPENED → ASSIGNED
Attachment #572234 - Flags: feedback?(margaret.leibovic)
Created attachment 572235 [details]
Screenshot of patch

This is a screenshot of the patch.
Attachment #572235 - Flags: ui-review?(limi)
Target Milestone: Firefox 3 → ---

Comment 14

6 years ago
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+
Created attachment 572736 [details] [diff] [review]
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.
Attachment #572235 - Attachment is obsolete: true
Attachment #572736 - Flags: feedback?(margaret.leibovic)
Attachment #572235 - Flags: ui-review?(limi)
Created attachment 574467 [details]
Screenshot of patch
Attachment #574467 - Flags: ui-review?(limi)
Created attachment 574495 [details] [diff] [review]
Patch for bug 419231

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)
Created attachment 574496 [details]
Screenshot of patch

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-
Created attachment 575013 [details] [diff] [review]
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.
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-

Updated

6 years ago
Component: Toolbars → Toolbars and Toolbar Customization
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Product: Firefox → Toolkit
QA Contact: toolbars → toolbars
Created attachment 578599 [details]
Bookmarks Toolbar Items
Created attachment 579415 [details] [diff] [review]
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|.
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-
Created attachment 582139 [details] [diff] [review]
Patch for bug 419231 v2.2
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)
Created attachment 582142 [details]
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-
Created attachment 582722 [details] [diff] [review]
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 (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)?
Created attachment 583346 [details] [diff] [review]
Patch for bug 419231 v2.4

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.
Created attachment 583383 [details] [diff] [review]
Patch for bug 419231 v2.4

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-
Created attachment 583416 [details] [diff] [review]
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?
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;
 }
Created attachment 583430 [details] [diff] [review]
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.
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-
Created attachment 583435 [details] [diff] [review]
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.
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-
Created attachment 583437 [details] [diff] [review]
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;
>  }
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 }?
Created attachment 583441 [details] [diff] [review]
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.
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?
Created attachment 583636 [details] [diff] [review]
Patch for bug 419231 v2.10

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+
Created attachment 583650 [details] [diff] [review]
Patch for checkin
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
Last Resolved: 9 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [polish-easy][polish-visual][polish-p2][fixed-in-fx-team] → [polish-easy][polish-visual][polish-p2]
Target Milestone: --- → mozilla12

Updated

6 years ago
Depends on: 714277

Updated

6 years ago
Keywords: uiwanted

Updated

6 years ago
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.
Keywords: addon-compat, dev-doc-needed
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.
Keywords: dev-doc-needed
Duplicate of this bug: 256246
You need to log in before you can comment on or make changes to this bug.