24314 - Popups on addresses in message pane always operate on last address in list

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[Scott MacGregor]

in messenger.xul, I have a popup defined as follows (note: there is another bug
where you can't define the popup in an overlay):
  <popup id="emailAddressPopup"  popupanchor="bottomleft">
      <menuitem value="&AddToAddressBook.label;"
                oncommand="AddNodeToAddressBook(document.popupElement)"/>
      <menuseparator/>
      <menuitem value="&SendMailTo.label;"
                oncommand="SendMailToNode(document.popupElement)"/>
  </popup>

Now, inside a JS file that gets pulled in from nsMsgHdrViewOverlay.xul, I have
implementations of the methods inside the popup: SendMailToNode and
AddtoAddressBookNode.

when these methods get called, they are being given a null node. So
document.popupElement is not being defined when I use the popup menu.

Comment 1

[David Hyatt]

Sorry about this.  The code has since been changed.  It's "popupNode" and not
popupElement.

Comment 2

[Scott MacGregor]

popNode works for me.

We might want to update the xpfe popup document on mozilla.org to use popNode.

Comment 3

[Scott MacGregor]

unfortunately document.popupNode always returns the last node I added to my xul
document that uses the popup and not the node that was used to bring up the popup.

i.e. I inserted five titled buttons into the message header pane to represent
five addresses. All of these titled buttons use the popup. Regardless of which
address button I push, when I select a command from the popup, the node returned
by document.popupNode is always the 5th address titledbutton.

Hyatt, do you want me to re-open this or file a new bug?

Comment 4

[Scott MacGregor]

I take it back. This may be related to Bug #21223 where the popup doesn't come
up in the right place. As a result, in order to select an item in the popup menu
I have to move the mouse down to the middle of the message pane where the popup
appeared. I wonder if this mouse movement is causing me to mouse over another
titledbutton which shifts the "focus" of the popup such that document.popupNode
now returns the wrong node. Or maybe not....

Comment 5

[leger]

Clearing INVALID resolution due to reopen of this bug.

Comment 6

[Mike Pinkerton (not reading bugmail)]

Taking menu/popup bugs.

Comment 7

[leger]

BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL.  XUL 
component will be deleted.

Comment 8

[Peter Trudelle]

Putting on beta1 radar.

Comment 9

[Mike Pinkerton (not reading bugmail)]

bulk accepting xpmenu/popup bugs. sigh.

Comment 10

[leger]

Putting on PDT+ radar for beta1.

Comment 11

[Mike Pinkerton (not reading bugmail)]

mscott, is there currently any way i can debug this? i know that we're waiting on 
waterson's fix so the popup menus actually show up on those titledButtons in 
mail.

I checked the code, and it looks like we're doing the right thing. We only set |
document.popupNode| on a mouseDown. Maybe it's reset accidentally somewhere, and 
that's what I'd like to look into, but I need a test case in the product. ;)

Comment 12

[Mike Pinkerton (not reading bugmail)]

adding waterson's popup bug as a dependency.

Comment 13

[Scott MacGregor]

Pinkerton, first of all, thank you very very much for fixing the popup
positioning bug. I just verified that your changes did indeed fix the problem
.

I talked to waterson about the JS changes I needed to make for my xul elements
so the titledbuttons will work again. I'll check those in as soon as the tree
opens.

You'll just need to pull msgHdrViewOverlay.js in mailnews/base/resources/content.

Then you'll be able to see the popupNode returning the wrong element problem.

You mentioned that we set popupNode on a mouse down event. I need to mouse down
in order to select the popup menu item. Could this mouse down be causing me to
pick up the wrong popupNode?

Comment 14

[Mike Pinkerton (not reading bugmail)]

yeah, that's my guess. if you can, set a breakpoint in 
nsXULDocument::SetPopupNode and see when it gets called as you go through the 
process. It should be called when you first mousedown, but my guess is that it 
might be called again when you pick the item in the popup.

Comment 15

[Scott MacGregor]

SetPopup node was only getting called once. So that wasn't it. I tried it on the
same message but clicking on two different titledbuttons. For both cases,
XULPopupListenerImpl::MouseDown

appeared to be returning the same dom node in
mouseEvent->GetTarget( getter_AddRefs( targetNode ) );

I'll investigate some more.

Comment 16

[Mike Pinkerton (not reading bugmail)]

i assume this happens on all platforms.

Comment 17

[Mike Pinkerton (not reading bugmail)]

This is not actually the bug we thought it was, but a bug in the JS in 
msgHdrViewOverlay.js. Here's the scoop:

the JS keeps a var called currentAddressData as an array of objects and tries to 
use the XULElements that are the address titledbuttons as indices. So when a 
selection is made from the menu, the code does:

var key = document.popupNode;            // key is of type XULElement
var addrInfo = currentAddressData[key];

However, you just plain can't do this is JS! Arrays can only be indexed by int or 
by string, so using |key| this way causes key's toString() method to be invoked 
and the actual index becomes "[object XULElement]" -- certainly not anything that 
can be used to find the appropriate object!

This is probably why the last address added is always being retreived. Something 
other than the XULElement needs to be used as the key.

Changing the Summary, reassigning to mscott (who shouldn't feel stupid, I didn't 
know any of this either).

Comment 18

[Scott MacGregor]

Shame on me. Thanks for doing all the detecive work Pink.

I changed my code to not index off of dom node.
I owe you one.

Fix in hand.

Comment 19

[Scott MacGregor]

I checked in the fix for this. Note add to address book isn't hooked up yet
(that's in another bug) but clicking on send Mailto in the popup works. And the
one in the commercial build works too =).

Just click on an email address in the message pane, then select send mailto.
Verify that hte compose window comes up filled with the right address.

Comment 20

[Scott MacGregor]

marking as fixed for real.

Comment 21

[Ninoschka Baca]

Mozilla Build 2000-02-16-09M14: NT4
Commercial Build 2000-02-16-08M14: Linux
Commercial Build 2000-02-16-08M14: Mac
Verified Fixed. The correct email address is in the address pane of the new 
message and it also send the message successfully.