329122 - Event dispatching code in nsGenericDOMDataNode doesn't handle event retargeting

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Event dispatching code in nsGenericDOMDataNode doesn't handle event retargeting.
Currently nsGenericDOMDataNode::HandleDOMEvent, after bug 234455 nsGenericDOMDataNode::PreHandleEvent .

Comment 1

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

Should the BindingParent handling of nsGenericDOMDataNode
changed too. Right now nsGenericDOMDataNode uses its parent's BindingParent.
That is wrong, right?

<binding>
  <content>Some text</content>
</binding>
Binding parent should be the parent node of the text node, not the binding parent of the parent node.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, the problem is not wanting to add another pointer to textnodes...

Do we have a free bit on nsGenericDOMDataNode?  If so, we could stick the binding parent in the property table in the rare case when it differ's from our parent's...  Or I guess the only way they could differ is if the binding parent _is_ the parent, so we could flag that with the bit...

Comment 3

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

We should just stick mFlagsOrSlots on nsGenericDOMDataNode IMHO. Yes, it'll cost us a few bytes, but really, i doubt it'll be that bad.

And that'd let us move the member to nsIContent and inline a bunch of stuff...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... So that would be sizeof(void*) more bytes per textnode, right?

Comment 5

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

Yes, i think that is something that wouldn't make the sky fall.

Comment 6

[Peter Van der Beken [:peterv]]

Sticking it in the property hash doesn't really require another bit, just call CouldHaveProperties. It means an additional hash lookup if the node is a range boundary, has an event listener or has other properties (all of those switch the IS_IN_A_HASH bit). It would also add a hash lookup for looking up all those when the bindingparent != parent. Wouldn't make the sky fall either ;-).

Comment 7

[Boris Zbarsky [:bzbarsky]]

I guess either way.  Let's just do it one way or the other?  ;)

Comment 8

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

Attachment: testcase for event retargeting

Comment 9

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

Attachment: mFlagsOrSlots to nsIContent

Moved mFlagsOrSlots to nsIContent, nsDOMSlots to its own header file,
some generic implementations of nsIContent methods (used by nsGenericElement and nsGenericDOMDataNode, but not always by nsXULElement - some other generic nsIContent method implementations can be done later), event retargeting in nsGenericDOMDataNode.

What do you think about this, Jonas?

Comment 10

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

Attachment: better mutation events handling

Comment 11

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

Drats, so we have duplicated work. I have a patch 334075 that does a lot of the same thing. Mind if we checked that one in first?

Comment 12

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

(In reply to comment #11)
> Drats, so we have duplicated work. I have a patch 334075 that does a lot of the
> same thing. Mind if we checked that one in first?
> 

Sounds good to me.

Comment 13

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

Comment on attachment 220645 [details] [diff] [review]
better mutation events handling

I'll do a new patch after bug 334075

Comment 14

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

Attachment: testcase for textNode.childNodes == textNode.childNodes

Comment 15

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

Attachment: nsDOMSlots for nsGenericDOMDataNode

nsGenericDOMDataNode doesn't need all the members that 
nsGenericElement::nsDOMSlots has, so I decided to do a simpler class
for nsGenericDOMDataNode, (not to move nsDOMSlots to nsIContent for example).
The patch fixes both testcases.

Comment 16

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

Comment on attachment 222345 [details] [diff] [review]
nsDOMSlots for nsGenericDOMDataNode

>Index: src/nsGenericDOMDataNode.cpp

>+nsGenericDOMDataNode::nsDOMSlots::nsDOMSlots(PtrBits aFlags)

Please name the class something different than nsDOMSlots to avoid confusion with the one in nsGenericElement. nsDataSlots for example.

Ideally we should rename the nsGenericElement one to nsElemSlots or some such.

> nsGenericDOMDataNode::GetChildNodes(nsIDOMNodeList** aChildNodes)
...
>+  nsDOMSlots *slots = GetDOMSlots();
>+
>+  if (!slots) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }

I like NS_ENSURE_TRUE(slots, NS_ERROR_OUT_OF_MEMORY). Up to you. Same applies elsewhere in the patch.

Looks great otherwise! Please keep an eye on regressions though since i'm not sure if we have any code that'll start freaking out when we start support bindingParents on textnodes.

r=me

Comment 17

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

Attachment: nsDOMSlots -> nsDataSlots

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 222357 [details] [diff] [review]
nsDOMSlots -> nsDataSlots

>Index: src/nsGenericDOMDataNode.cpp
> nsGenericDOMDataNode::GetChildNodes(nsIDOMNodeList** aChildNodes)

Please set *aChildNodes to null up front, so it's null in the error cases?

>+  NS_PRECONDITION(!GetBindingParent() ||
>+                  aBindingParent == GetBindingParent() ||
>+                  (aParent &&
>+                   aParent->GetBindingParent() == GetBindingParent()),
>+                  "Already have a binding parent.  Unbind first!");

So the third part of that is a little wrong.  See the nsGenericElement version (or https://bugzilla.mozilla.org/show_bug.cgi?id=286000#c13).

> nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
> {
>+    // Notify XBL- & nsIAnonymousContentCreator-generated
>+    // anonymous content that the document is changing.

Maybe comment that we need this so that the insertion point for |this| will get updated?  Otherwise it looks weird, since generic DOM data nodes can't have XBL bindings, anon kids, etc.

sr=bzbarsky.

I sorta wonder whether it makes sense to have an nsIContent::Slots impl that would have binding parent and child nodes and just have generic element extend it.  We might be able to have a single method (nsIContent::BindToTree ?) for BindToTree then -- the only difference between nsGenericElement and nsGenericDOMDataNode now is in walking the child nodes...  Separate bug, though.

Comment 19

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

Checked in.
Still watching whether this made any changes to tp/tdhtml/etc..

Comment 20

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

I was thinking about putting mBindingParent in nsSlots too, but since in nsGenericElement it lives inside a union I don't think it's woth the hassle