234717 - Right-clicking a File Within a Bookmarks Folder in the Bookmarks Menu Makes That Folder Inaccessible if Nothing is Selected on the Context Menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Parenton]

Right-clicking a file within a folder makes that folder inaccessible for that
session if nothing is chosen from the context menu.  Restarting Firebird is a
workaround.  

The previous workaround for Bug 210910 does not work, as that is what causes
this behavior

Also, this was broken again at the same date that bug 234116 appeared.

JS Console Errors:

Error: uncaught exception: [Exception... "Node was not found"  code: "8"
nsresult: "0x80530008 (NS_ERROR_DOM_NOT_FOUND_ERR)"  location:
"chrome://browser/content/bookmarks/bookmarksMenu.js Line: 32"]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040217
Firefox/0.8.0+

Comment 1

[Jesse Ruderman]

This bug and bug 234116 are probably the same bug because it's the same error
message and line number in bookmarksMenu.js.

Comment 2

[Hussam Al-Tayeb]

Problem seems to have started with the 20040205 trunk build.

Comment 3

[basic]

seems to me that the onpopuphidden isn't being called for sub menus.

Comment 4

[basic]

ok I added a

 window.dump(aTarget.parentNode.id+"\n");

to my "hideOpenInTabsMenuItem" and found that when this happens, the function
doesn't get called for the submenus (which I believe should be).

Comment 5

[basic]

Attachment: work around patch

here's a patch to work around the problem that hideOpenInTabsMenuItem doesn't
get called for each sub menu when a context menu is is involved in the hiding
process. I'm not sure if this is the right fix though, I suspect that the
somehow the context menu is stealing some of the onhidden events or something.

Comment 6

[basic]

Attachment: work around patch

oops bad spacing in the previous patch

Comment 7

[Hussam Al-Tayeb]

Re: comment #6
Has somebody tested this patch?

Comment 8

[amano]

Why don't you ask for review/superreview?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 141799 [details] [diff] [review]
work around patch

>+    var i;
...
>+    for(i = nodeList.length - 1; i >= 0; i--)
>+      nodeList[i].parentNode.removeChild(nodeList[i]);
...
>+    for(i = nodeList.length - 1; i >= 0; i--)
>+      nodeList[i].parentNode.removeChild(nodeList[i]);

I don't know enough to say for sure, but isn't:

  for (var i=nodeList.length-1; i>=0; i--)

the preferred syntax (with respect to using a local looping variable, spacing,
etc.)?	This syntax is used in lines 626, 639, 653, 831, and 852 of the edited
file, so I'd bet it's preferred.  Here's a copy of line 852 for a syntactic
reference:

for (var i=0; i<buttons.childNodes.length; i++)

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #9)
> (From update of attachment 141799 [details] [diff] [review])
> >+    var i;
> ...
> >+    for(i = nodeList.length - 1; i >= 0; i--)
> >+      nodeList[i].parentNode.removeChild(nodeList[i]);
> ...
> >+    for(i = nodeList.length - 1; i >= 0; i--)
> >+      nodeList[i].parentNode.removeChild(nodeList[i]);
> 
> I don't know enough to say for sure, but isn't:
> 
>   for (var i=nodeList.length-1; i>=0; i--)
> 
> the preferred syntax

Actually, as I look at it I realize it loops in reverse (i.e., decrementing
between loops instead of incrementing) of all the other loops I cited.  Perhaps
this is the real preferred way?

  for (var i=0; i<=nodeList.length-1; i++)

Also setting as All/All - it should be a problem on all platforms.

Comment 11

[Jeff Walden [:Waldo]]

It's in an #else for an #ifdef XP_MACOSX, so it's most likely only non-Mac OS X.
 Sorry about the bugspam...

Comment 12

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040229 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

While this build is affected by
http://bugzilla.mozilla.org/show_bug.cgi?id=234116 I am not seeing this bug on
my build. Right-clicking and forcing a fail on the context menu does not the
submenu inaccessible, and the bookmarks still work as well.

Comment 13

[Hussam Al-Tayeb]

(In reply to comment #8)
> Why don't you ask for review/superreview?
How do I do that?

Comment 14

[amano]

It is Basic's patch. He should set the R/SR flags if he thinks that his patch
should be merged into the trunk.

Comment 15

[Hussam Al-Tayeb]

This unofficial build 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040303
Firefox/0.8.0+ (djeter)
includes this patch
"http://bugzilla.mozilla.org/attachment.cgi?id=141799&action=view"
and it's working fine.

Comment 16

[Hussam Al-Tayeb]

Any chance of this being fixed? the patch
http://bugzilla.mozilla.org/attachment.cgi?id=141799&action=view
seems to be working fine 

Comment 17

[Hussam Al-Tayeb]

The proposed patch also fixes bug 234116

Comment 18

[basic]

my guess is that this bug is somewhat related to the bugs listed in bug 214881,
should this bug be added to that list?

Comment 19

[basic]

Attachment: testcase

I've a simplified testcase that don't use the onpopuphidden event. Seems to me
that something is wrong with putting context menus on menupopups are involved.

Comment 20

[basic]

ok, I've been informed that menupopups with context menus are just flaky and
hard to fix. So before I ask for a review of my work around patch, are we sure
that the patch works? For both this bug and bug 234116 ?

Comment 21

[Hussam Al-Tayeb]

Yes, djeter did a 20040314 trunk build with this patch and the bug does not show
up in his build. See http://forums.mozillazine.org/viewtopic.php?p=431036

Comment 22

[Peter van der Woude [:Peter6]]

Is there a reason for not checking this one into trunk and marking Fixed ?

Comment 23

[Robert Parenton]

(In reply to comment #22)
> Is there a reason for not checking this one into trunk and marking Fixed ?

It hasn't been marked review+.

Comment 24

[u116418]

The workaround is still slightly bugged. Right-click on a subfolder in the
Bookmarks menu and click back into the browser window (do not select an item on
the context menu). The menu persists while the Bookmarks menu collapses.

Comment 25

[basic]

Comment on attachment 141799 [details] [diff] [review]
work around patch

(In reply to comment #24)
> The workaround is still slightly bugged. Right-click on a subfolder in the
> Bookmarks menu and click back into the browser window (do not select an item on
> the context menu). The menu persists while the Bookmarks menu collapses.
I suspected as much. I'll try to come up with a better workaround, stay tune.

Comment 26

[basic]

Attachment: patch (ensure that all submenus are hidden)

here's a patch to ensure that all the submenus are hidden when the bookmarks
menu is hidden. I've tested it, and it seems to work well. Please help test it.
If we check this in, this code should be backed out when context menus on
menupopups work correctly (sometime in the future).

Comment 27

[basic]

Attachment: patch (ensure that all submenus are hidden)

oops wrong patch

Comment 28

[u116418]

Seems to work well!

Comment 29

[Pierre Chanial]

Comment on attachment 144204 [details] [diff] [review]
patch (ensure that all submenus are hidden)

getElementByAttribute was introduced to work around a bug: when a bookmark was
dropped in the menupopup, it could be displayed between the separator and the
'open in tab' menuitem.

The good thing is that the bug that broke our context menus very probably also
fixed the underlying problem.

Comment 30

[Pierre Chanial]

marking fixed.

Comment 31

[basic]

I'm not entirely sure this is really fixed, the patch that was checked in does
not ensure that all the submenus are closed...

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/components/bookmarks/content&command=DIFF_FRAMESET&file=bookmarksMenu.js&rev1=1.28&rev2=1.29&root=/cvsroot

Comment 32

[u116418]

(In reply to comment #31)

Verified that in 04/02/2004 the context menu does not go away if I right-click
on a folder, then click out.

Comment 33

[basic]

(In reply to comment #32)
> (In reply to comment #31)
> 
> Verified that in 04/02/2004 the context menu does not go away if I right-click
> on a folder, then click out.
Does this bug still exists as stated in the summary? If not please file another
bug for that.

Comment 34

[mmortal03]

Obviously, a portion of this bug still exists.  Try it out for yourself.  It 
doesn't happen repetitively, and I think when it occurs consecutively it has 
something to do with right clicking deeper and deeper folder depth.

Comment 35

[Asa Dotzler [:asa]]

Please file new bugs for any remaining issues. Thanks.

Comment 36

[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