79416 - document.popupNode returns the wrong element

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[HJ]

David Hyatt asked me to file this bug, so here it is :)

I wrote a small testcase, after having problems with the context menu (bug
78725). This example here will give you an impression of what i did to trigger
the bug.

<!-- start of snap -->
<tabbox>
  <tabs>
    <tab label="mytab" context="MyContextMenu">
  </tabs>
</tabbox>

<popupset>
  <popup id="MyContextMenu">
    <menuitem label="MenuItem"
oncommand="alert(document.popupNode.nodeName.toLowerCase())"/>
  </popup>
</popupset>
<!-- end of snap -->

result:
The alert in build 2001041704 shows 'tab' and the alert in build 2001050608
shows 'menuitem'. 

note:
I don't know if this is directly related to bug 78725 but if it is, please kill
that sucker asap.

Comment 1

[Dan Rosen]

I'm not entirely positive that this is my code causing the problem, but hyatt
and I noticed one oddity in the XUL popup listener, which is attempting to set
the popup node to the tooltip node. Not sure if that's it, though. I'll see.

Comment 2

[HJ]

Attachment: xul testcase

Comment 3

[HJ]

I just added a small xul testcase with one tab. If you right-click on the tab a
tab context menu will open, if bug 78725 doesn't prevent you from opening it.
And if it does, just reload the testcase. Click on the menuitem to see the alert.

Comment 4

[Dan Rosen]

Yep, I tried your testcase and have a variant of my own. Both show the bug. The
fix I mentioned (about tooltips) doesn't work... This'll take some heavier
hunting. Helpwanted!

Comment 5

[Fabian Guisset]

Might this be causing the bookmarks sidebar context menuitems to fail, and the
personal toolbar context menuitems to act as if I was right-clicking in the
sidebar? I'd be tempted to say document.popupNode now returns the selected node
instead of the node on which I right-clicked, but that's without looking at any
code.
If it's not cause of the wierdness with the bookmarks context menus then ignore,
but in the meantime i'm upgrading to major.

Comment 6

[Dan Rosen]

By the way, the general thought here is that my checkin for bug 64313 caused
this, since it changes where document.popupNode is stored (it used to be a
member of nsXULDocument; now it's on nsFocusController and accessed through
either XULDocument or FocusController).

What may have happened, and what I'll look for, is that I set the popup node
myself somewhere entirely wrong...

Comment 7

[HJ]

I sure hope that you get this fixed real soon. My multiZilla project is close to
nothing like it is now. I really _NEED_ document.popupNode for the
tabcontextmenu code to do anything. Or is there a workaround for this?

Comment 8

[HJ]

Attachment: new testcase, see my next comment

Comment 9

[HJ]

In all builds after 2001041704 both of the tabs show 'menuitem' as
document.popupNode and the third menuitem in the contextmenu, for the first tab,
shows 'yes' in builds after 2001041704.

Fabian wrote:
>I'd be tempted to say document.popupNode now returns the selected node
>instead of the node on which I right-clicked

Well, it's the same element for sure!

Something interesting:
The first tab shows 'tab' and the second tab 'menuitem' in build 2001041704.
The second tab produces the same error in build 2001041704 as in all later
builds. >>> The only difference here is a _tooltip_ on the popup node <<<

So if i set a tooltip on the popup node, it's over and out!

Comment 10

[Dan Rosen]

[spam] pushing off non-critical 0.9.2 bugs to 0.9.3. please take the helpwanted
keyword to heart if you'd like to see these fixed in 0.9.2!

Comment 11

[Dan Rosen]

[spam] [e-mail]'s bugs subject to redistribution by chofmann. R!

Comment 12

[David Hyatt]

XUL bug.  Taking.

Comment 13

[David Hyatt]

Attachment: Only set popupnode for non-tooltip popups

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 51201 [details] [diff] [review]
Only set popupnode for non-tooltip popups

sr=[e-mail]

Comment 15

[jag (Peter Annema)]

Comment on attachment 51201 [details] [diff] [review]
Only set popupnode for non-tooltip popups

r=jaggles

Comment 16

[David Hyatt]

Fixed.