65014 - Extra "Save As..." and Separator in navigator context menu

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Stephen Walker]

Using the 2001011004 nightly on win2k, there are 2 separators above Save As in 
the navigator context menu, where there should only be one. Assigning to morse 
since blake thinks morse forgot to hide the extra separator that separated 
prefill form and save form data.

Comment 1

[Stephen P. Morse]

No, I haven't changed anything here in a while.  Furthermore, I'm unable to 
reproduce this bug.

Reassigning to Bill Law since he handles context menu.

Comment 2

[Blake Ross]

I can reproduce this.  There's also two "Save As..."'s.  will look into it later

Comment 3

[:deanis74]

resummarizing based on Blake's comments

Comment 4

[:deanis74]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

I checked some old builds.  The menu works correctly in 2001-01-09-08 and before
is broken in 2001-01-09-21 and after.

I suspect that ben's checkin to fix bug 56719 and bug 57108 may be causing this.

Comment 7

[John Morrison]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Looking at it, basically looks like the entire context menu is duplicated --
those two separators are the two separators in that menu that are never removed,
and that option is the one option that is never removed.

Comment 9

[Blake Ross]

Attachment: [patch] scary "fix"

Comment 10

[Blake Ross]

I don't know enough about how deep merging works to understand why this patch 
seems to fix the problem.  Ben, thoughts?

Comment 11

[Jure Repinc (JLP)]

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

Comment 12

[Jure Repinc (JLP)]

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

Comment 13

[hirata masakazu]

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

Comment 14

[Blake Ross]

Well, this fix doesn't hurt anything and this is annoying, so I'd like to get 
this in until Ben and I can figure it out.

Comment 15

[Håkan Waara]

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

Comment 16

[Blake Ross]

alec, sr?

Comment 17

[timeless]

r=timeless

Comment 18

[Alec Flett]

sr=alecf

Comment 19

[Bernd]

>I don't know enough about how deep merging works to understand why this patch 
>seems to fix the problem.  Ben, thoughts?

How can a patch with such a comment get a r and a sr? I thought that the review
guidelines describe a slightly different mechanism. Could somebody explain, why
the patch is good and will not cause regressions. I think this would help also,
if later, one has to return to this bug and all the intentions, that are now
clear, may be difficult to understand without explanation.

Comment 20

[Blake Ross]

Because the patch is completely harmless.  It just changes the id of one 
menuitem (that isn't used anywhere else), and adds id's to two separators.  I 
checked in that workaround, and now I'll leave this open to eventually figure 
out why this happened.  It may, as Ben said, just have been bad use of xul and 
overlays (a menuitem and a popup menuitem needlessly sharing the same id).

Comment 21

[timeless]

http://lxr.mozilla.org/seamonkey/search?string=context-savepage

/xpfe/browser/resources/content/navigatorOverlay.xul, line 166 -- <menuitem 
id="context-savepage" key="key_savePage"
observes="Browser:SavePage"/>
/xpfe/communicator/resources/content/nsContextMenu.js, line 106 -- 
this.showItem( "context-savepage", !this.inDirList );
/xpfe/communicator/resources/content/contentAreaContextOverlay.xul, line 125 -- 
<menuitem id="context-savepage"
--
Giving separators id's is harmless, splitting the id's for the save as item is 
odd, but if blake promises to continue to investigate then it's not a big deal.

Comment 22

[Warner Young]

Looks fixed in build 2001020504.

Comment 23

[sairuh (rarely reading bugmail)]

yes...interesting... appears fixed to me as well [2001.02.05.xx on winnt and
mac, 2001.02.07.12 on linux]. blake, was something else checked in? should this
be marked resolved...? thx!

Comment 24

[Blake Ross]

I checked in the workaround, but want to leave this open to figure out why it 
happened at all.

Comment 25

[Blake Ross]

This fix was fine, and since it's generally a Bad Thing to have two nodes with 
the same id's, I don't think further investigation is needed here. Marking 
fixed.

Comment 26

[:deanis74]

verifying