490401 - "Bookmarks Toolbar Items" actually displays bookmarks in customization palette

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Phil Ringnalda (:philor)]

STR:
1. Be sure you have at least six or eight bookmarks on the bookmarks toolbar
2. View - Toolbars - Customize
3. Drag the "Bookmarks Toolbar Items" item off the toolbar and drop it in the palette

Expected:
Since nobody's gotten interested in fixing bug 399755 so there would be a reason to expect an icon with the label below it, expected is "just a label saying 'Bookmarks Toolbar Items' in the palette."

Actual:
The label, and above it the entire set of toolbar bookmarks, sprawling clear across the entire customization sheet, jamming together the other things on the same row and still running over the top of them.

Comment 1

[Alice0775 White]

Same issue in Windows XP SP3.
Regression range:
Works fine:
http://hg.mozilla.org/mozilla-central/rev/0d300ab7a8cf
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081203 Firefox/3.2a1pre ID:20081203041857

Broken:
http://hg.mozilla.org/mozilla-central/rev/169e8eb751a7
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Firefox/3.2a1pre ID:20081204034432

Comment 2

[Nochum Sossonko [:Natch]]

Does this affect 1.9.1?

In any rate pushlog for regression in comment 1:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=150dbe7b6fd7&tochange=169e8eb751a7
->Maybe bug 407725?

Comment 3

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre
Confirmed.

It would be great to get a regression range for 1.9.1, but for now I suspect bug 407725.

Comment 4

[Alice0775 White]

Regression range in 1.9.1:
Works fine:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/18398e0350cd
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081209 Shiretoko/3.1b3pre

Broken:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9269759f81b7
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081210 Shiretoko/3.1b3pre

Changesets between the above range:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=18398e0350cd&tochange=9269759f81b7

Comment 5

[Nochum Sossonko [:Natch]]

Thanks, that fits with bug 407725's landing. For future reference the correct pushlog is:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=9b896a3006fe&tochange=9269759f81b7

Due to the way fromchange/tochange works you have to use the parent changeset in fromchange, i.e. it shows the changesets from, but not including, the specified fromchange, until and including the specified tochange.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> Due to the way fromchange/tochange works you have to use the parent changeset
> in fromchange, i.e. it shows the changesets from, but not including, the
> specified fromchange, until and including the specified tochange.

We don't want fromchange to be included, since it worked with 18398e0350cd.

Comment 7

[Philip Chee]

I think it is possible that Firefox was relying on a bug in the toolkit customization code which was fixed by bug 407725

Comment 8

[Philip Chee]

This fixes the current problem as well as Bug 399755.

@@ -133,14 +133,18 @@
   visibility: hidden;
 }
 
 toolbarpaletteitem[place="toolbar"] .places-toolbar-items {
   display: none;
 }
 
+toolbarpaletteitem[place="palette"] hbox[type="places"] {
+  overflow: hidden;
+}
+
 /* ::::: bookmark menus ::::: */
 
 menu.bookmark-item,
 menuitem.bookmark-item {
   min-width: 0;
   max-width: 26em;
 }

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch

Moving .bookmarks-toolbar-items and .bookmarks-toolbar-customize to where they belong, removing .bookmarks-toolbar-overflow-items as there's no such element, and hiding the places hbox for the palette in browser.css, as there's no places.css in that dialog.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 374952 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

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

I wonder whether one of:

toolbarpaletteitem[place="palette"] > #personal-bookmarks > #bookmarksBarContent
toolbarpaletteitem[place="palette"] #bookmarksBarContent

is more efficient.

bookmarks-toolbar-items and places-toolbar-items appear to be redundant. Perhaps we should remove the unused one on trunk in a followup?

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> I wonder whether one of:
> 
> toolbarpaletteitem[place="palette"] > #personal-bookmarks >
> #bookmarksBarContent
> toolbarpaletteitem[place="palette"] #bookmarksBarContent
> 
> is more efficient.

At that point I could even use #wrapper-personal-bookmarks instead of toolbarpaletteitem. The problem with the id selectors is that they would only do this for the existing bookmarks toolbar item, while an extension could add others with a type="places" hbox and a custom place URI. It seems odd to support the latter (e.g. in places.css via hbox[type="places"]) and leave customization partly broken.

Comment 12

[Philip Chee]

> while an extension could add
> others with a type="places" hbox and a custom place URI
Isn't this the responsibility of the extension author?

Comment 13

[Dão Gottwald [:dao]]

We should either provide this tool correctly or not provide it. Attaching the binding automatically and asking extension authors to duplicate the CSS for making the internals of the binding work as expected doesn't make sense.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/28a713630897

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #10)
> bookmarks-toolbar-items and places-toolbar-items appear to be redundant.
> Perhaps we should remove the unused one on trunk in a followup?

filed bug 491006

Comment 16

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

Comment on attachment 374952 [details] [diff] [review]
patch

a191=beltzner

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e9e27c40d0af

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre ID:20090505030940

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre ID:20090505030932