Closed Bug 490401 Opened 11 years ago Closed 11 years ago

"Bookmarks Toolbar Items" actually displays bookmarks in customization palette

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: philor, Assigned: dao)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file)

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.
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
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.
Flags: blocking-firefox3.5?
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
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.
Blocks: 407725
OS: Mac OS X → All
Hardware: x86 → All
(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.
I think it is possible that Firefox was relying on a bug in the toolkit customization code which was fixed by bug 407725
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;
 }
Assignee: nobody → dao
Attached patch patchSplinter Review
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.
Attachment #374952 - Flags: review?(gavin.sharp)
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?
Attachment #374952 - Flags: review?(gavin.sharp) → review+
(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.
> 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?
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.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/28a713630897
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Attachment #374952 - Flags: approval1.9.1?
(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
Flags: blocking-firefox3.5? → blocking-firefox3.5-
Attachment #374952 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.