291781 - bookmark context menu no longer works after customizing toolbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Takeshi Nishimura]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; ja-JP; rv:1.7.6) Gecko/20050318 Firefox/1.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050419 Firefox/1.0+

Normal XUL element bookmarks-menu has a controller called BookmarksMenuController.
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#760
> bm.controllers.appendController(BookmarksMenuController);

After you customize toolbar this controller is badly removed.

Reproducible: Always

Steps to Reproduce:
1.Click View > Toolbars > Customize...
2.Click Done.
Though you cannot see the result on release versions.



Currently this bug does not cause any problem because Firefox fortunately seems
not to use the controller. You should fix this bug before it appears.
For example, the patch Bug 246158 comment #7 will be broken.

At my first debugging, it is correct just before menubar-items is removed from
the document in wrapToolbarItem().
http://lxr.mozilla.org/mozilla/source/toolkit/content/customizeToolbar.js#294
> aToolbarItem.parentNode.removeChild(aToolbarItem);
removeChild() may probably cause this bug.

Comment 1

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

Is this still an issue in recent trunk builds? Can you provide more detail as to
what the appropriate fix would be?

Comment 2

[Takeshi Nishimura]

This bug still exists on trunk-20050623.

Quick fix: re-add the BookmarksMenuController to `bookmarks-menu' in
BrowserToolboxCustomizeDone() (like to `bookmarks-ptf').
Or we can modify the functions removeChild() and appendChild() to preserve
nodes' controllers.

Again, this bug causes nothing as far as Firefox does not use bookmarks-menu's
controller.
I don't know this inability breaks other controllers elsewhere.

Comment 3

[Takeshi Nishimura]

This should be fixed because the recently checked-in patch from bug 246158
depends on bookmarks-menu.

Steps to Reproduce (again):
1.Click View > Toolbars > Customize...
2.Click Done.
3.Context menu on Bookmark menu and Bookmarks Toolbar no longer works.

Comment 4

[Takeshi Nishimura]

Attachment: Quick fix

Re-add the BookmarksMenuController to `bookmarks-menu' in
BrowserToolboxCustomizeDone() (like to `bookmarks-ptf').
This is a simple patch fixing regression.

Comment 5

[Takeshi Nishimura]

Summary changed.
was: bookmarks-menu's controller(BookmarksMenuController) is badly removed after
"Customize..." toolbar dialog

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I can reproduce with 2005091406-trunk/WinXP.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 196019 [details] [diff] [review]
Quick fix

that'll work, thanks

Comment 8

[Takeshi Nishimura]

Comment on attachment 196019 [details] [diff] [review]
Quick fix

mconnor,
Thanks for review.
Can you check in the patch for me? I don't have CVS access.

Adding approval1.8b5?
This is a simple patch fixing regression.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk.

Comment 10

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

1.8 Branch:
mozilla/browser/base/content/browser.js; new revision: 1.479.2.27;

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

*** Bug 308809 has been marked as a duplicate of this bug. ***