268553 - Can't add a new folder on the bookmarks toolbar after having deleted a folder

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Michali Sarris]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

If i delete a folder from the bookmarks toolbar i'm not able to add a new folder
untill restarting firefox.

Reproducible: Always
Steps to Reproduce:
1. Add a folder to the bookmarks toolbar by rightclicking it and choosing "New
Folder...".
2. Delete the just created folder.
3. Try to add another new folder the same way as point 1.

Actual Results:  
I hear an alert beep and nothing happens.

Expected Results:  
A dialog should have appeared to add a new folder.

Comment 1

[Ger Teunis]

I confirm (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20041107 Firefox/1.0)

Comment 2

[Tracy Walker [:tracy]]

confirmed with Windows branch build 2004-11-07-07-0.11

Comment 3

[Winston]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Extension of this bug?

Reproducible: Always
Steps to Reproduce:
1. Right Click on Bookmarks Toolbar
2. Manage Folder
3. Click New Folder
4. Creates New Folder
5. Drag and Drop links into this Folder
6. Click New Folder
7. Creates New Folder in Bookmarks but not in Bookmarks Toolbar

Actual Results:  
Creates 2nd New Folder in Bookmarks but not Bookmarks Toolbar

Expected Results:  
Add New Folder in Bookmarks Toolbar

Comment 4

[Michali Sarris]

I think this is the bug you mean is:
https://bugzilla.mozilla.org/show_bug.cgi?id=268551

I found it the same time as this bug.

(In reply to comment #3)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107
Firefox/1.0
> 
> Extension of this bug?
> 
> Reproducible: Always
> Steps to Reproduce:
> 1. Right Click on Bookmarks Toolbar
> 2. Manage Folder
> 3. Click New Folder
> 4. Creates New Folder
> 5. Drag and Drop links into this Folder
> 6. Click New Folder
> 7. Creates New Folder in Bookmarks but not in Bookmarks Toolbar
> 
> Actual Results:  
> Creates 2nd New Folder in Bookmarks but not Bookmarks Toolbar
> 
> Expected Results:  
> Add New Folder in Bookmarks Toolbar
> 

Comment 5

[Todd Baur]

Confirmed on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5)
Gecko/20041107 Firefox/1.0

Comment 6

[Tracy Walker [:tracy]]

still seeing this on 1.0.1 from 02/22

a sort of workaround is to open a new window and add the new folder there.

Comment 7

[Chris]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050410
Firefox/1.0+

I tried to reproduce this bug. On the current nightly I'm using, I cannot even
create a 'new folder' once straight from the bookmarks toolbar.

Comment 8

[Chris]

WFM
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050422
Firefox/1.0+
(Windows 2000)

Comment 9

[Ger Teunis]

WFM, seems to be fixed on the trunk!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050418
Firefox/1.0+

Comment 10

[Asaf Romano (gone)]

WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050507
Firefox/1.0+

Comment 11

[Jim Ginn]

Reopening.

I can reproduce this 100% with a clean profile.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050511
Firefox/1.0+

Comment 12

[Jaime Mitchell (use [e-mail] for email)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050512
Firefox/1.0+ ID:2005051212

WFM following steps in comment 0, on both an existing and a new profile.

Comment 13

[Mathieu Parent]

The problem is that BookmarksMenu._target is not released (unset) after a
bookmark deletion. After, it is passed as argument for
BookmarksCommand.createNewFolder(realTarget); (line 1110)

Adding one line in firefox\chrome\browser.jar\content\browser\bookmarks\bookmarks.js
should fix this


  deleteBookmark: function (aSelection)
  {
    // call checkSelection here to update the immutable and other
    // flags on the selection; when new resources get created,
    // they're temporarily not valid because they're not in a
    // bookmark container.  So, they can't be removed until that's
    // fixed.
    BookmarksUtils.checkSelection(aSelection);
    BookmarksUtils.removeAndCheckSelection("delete", aSelection);
//adding the next line
    BookmarksMenu._target= null;
  },


of course, i don't know if this is the good syntax and if it is in the good file
(maybe in BookmarksUtils.removeAndCheckSelection() from ?)

kind-of-review ?

Comment 14

[Mike Connor [:mconnor]]

Ooh, good call.

-> me, unless someone else wants to write a real patch.

Comment 15

[Grayson Mixon (ColdFusion650)]

wfm. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050609
Firefox/1.0+ ID:2005060908. someone needs to mark this resolved.

Comment 16

[Peter van der Woude [:Peter6]]

(In reply to comment #15)
> wfm. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050609
> Firefox/1.0+ ID:2005060908. someone needs to mark this resolved.

No, I can still reproduce this
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050615
Firefox/1.0+ ID:2005061513

Comment 17

[Asaf Romano (gone)]

This also happens on "Cut".

Comment 18

[Jaime Mitchell (use [e-mail] for email)]

Attachment: Patch from comment 13

This patch uses the code from comment 13, also added into the cut command as
per comment 17. I have briefly tested this and it doesn't appear to break any
of the functions however as per comment 12 I am unable to reproduce so can't
verify that it fixes the problem.

Could someone test this to see if it fixes this bug.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 189116 [details] [diff] [review]
Patch from comment 13

won't this break the bookmarks manager?

Comment 20

[Jaime Mitchell (use [e-mail] for email)]

(In reply to comment #19)
> (From update of attachment 189116 [details] [diff] [review] [edit])
> won't this break the bookmarks manager?

After some testing, it does not break the bookmarks manager, however it does
give the following error in the JS console:

Error: BookmarksMenu is not defined
Source File: chrome://browser/content/bookmarks/bookmarks.js
Line: 408

So a different solution is probably required.
(thanks Mano)

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch

Just clear out _target and _selection in destroyContextMenu.  Note that the
patch includes some extra comment cleanup (# isn't a comment character in JS,
grr), but the fix is just two lines near the start.

Comment 22

[Asaf Romano (gone)]

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

r=mano

Comment 23

[Asa Dotzler [:asa]]

Can we get this landed on the trunk and verified before taking on the branch?

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

k, in on trunk.

Comment 25

[Marcia Knous [:marcia]]

I am unable to verify this on today's trunk build since I can't create a new
folder by right clicking - I filed Bug 306457 to address this issue. 

Comment 26

[Chris Beard]

still blocking on accessibility regression in order to verify fix on trunk
before branch approval

Comment 27

[Aaron Leventhal]

(In reply to comment #25)
> I am unable to verify this on today's trunk build since I can't create a new
> folder by right clicking - I filed Bug 306457 to address this issue. 

That is only broken now if you have no bookmarks toolbar folder. I'm working on
a fix for that, but it should be good enough to test this fix.

Comment 28

[Tracy Walker [:tracy]]

verified on trunk with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050901 Firefox/1.6a1

Comment 29

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change