364092 - Related node on DOMAttrModified listener isn't the same as attr node removed by RemoveAttributeNode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alex Vincent [:WeirdAl]]

When I drop an attribute via the RemoveAttributeNode() method, an event listener on the owner element (or an ancestor) gets an event with a related node that is similar to, but not exactly, the same node being removed.

Testcase coming up as an attachment.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20061216 SeaMonkey/1.5a

also seen on Windows, OS->All, Platform->All.  This is not a regression; the same bug is known to exist on Firefox 2.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Steps to reproduce:  Load the test page.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: stack where things begin to go wrong

In nsDOMAttributeMap::GetAttribute (frame 03), we try to find the attribute.  However, the lookup fails, so at nsDOMAttributeMap.cpp#160 we create a new DOM attribute object with an empty attribute value.  It is this new node which gets set as the related node of the event, instead of the original node.

Comment 3

[Alex Vincent [:WeirdAl]]

So nsDOMAttributeMap::RemoveNamedItem is interesting.  First it calls GetAttribute(ni, aReturn, PR_TRUE).  The third argument says "Remove the attribute," which GetAttribute helpfully does.

Then it calls mContent->UnsetAttr(ni->NamespaceID(), ni->NameAtom(), PR_TRUE), which leads to the code for the mutation event to get the attribute node.

What would the impact be of simply switching these two calls?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

This should do it, or am I missing something ...
First create the attr node normally using GetAttribute, and
then it gets removed from the map when UnsetAttr is called.

Comment 5

[Alex Vincent [:WeirdAl]]

smaug: so if I'm reading the code right, nsXULElement::UnsetAttr and nsGenericElement::UnsetAttr both call on slots->mAttributeMap->DropAttribute(aNameSpaceID, aName) to get rid of the attribute.

I notice the code for nsDOMAttributeMap::DropAttribute is very similar to code for the latter half of nsDOMAttribute::GetAttribute(), so I think that achieves our objective.  Just as an aside for future work, would it be worth combining the two?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 248985 [details] [diff] [review]
proposed patch

That's simple enough :)

Comment 7

[Alex Vincent [:WeirdAl]]

I'll want to add this to a DOM test suite (there isn't one from the W3C for DOM Events).

Comment 8

[Alex Vincent [:WeirdAl]]

smaug, I don't see a patch or checkin that put this bug's testcase into the test harnesses; where was it added?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

There is a modified mochitest testcase.