683852 - Implement Node.contains(node)

Status: RESOLVED FIXED

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

Comment 1

[Alex Vincent [:WeirdAl]]

This is the kind of bug I could do pretty quickly.  Would we want anonymous content to show up as true?  What about native anonymous content (a la the video element's controls)?

Comment 2

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

Per spec anonymous content shows up as true only within the same anonymous content (since the spec
relies on parent-to-child chain, not child-to-parent chain).

Comment 3

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

Sorry Alex. I was in the train and started to write patch for this...
Taking.

Comment 4

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

Attachment: patch

Trying to avoid virtual calls, but GetBindingParent is there, and in
some cases IsNodeOfType.

Comment 5

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

Comment on attachment 557890 [details] [diff] [review]
patch


>+nsINode::Contains(const nsINode* aOther) const
>+{
>+  if (!aOther ||
>+      aOther == this ||
>+      !(aOther->IsElement() ||
>+        aOther->IsNodeOfType(nsINode::eCONTENT)) ||
>+      !GetFirstChild()) {
This is missing still one thing
.. ||
GetOwnerDoc != aOther->GetOwnerDoc()

Comment 6

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

Er, GetCurrentDoc != aOther->GetCurrentDoc()

Comment 7

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

Actually both.

Comment 8

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

Bah, new patch coming.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Does that mean you don't want review on that one?

Comment 10

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

Attachment: patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 557912 [details] [diff] [review]
patch

Once you do the document check, is there a reason to not just use a combination of ContentIsDescendantOf and nsContentUtils::IsInSameAnonymousTree?

Certainly reimplementing ContentIsDescendantOf here is not something we want, I think.

Other than that, looks good.

r=me with the above issue addressed.

Comment 12

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

Ah, I could reuse nsContentUtils::ContentIsDescendantOf, but 
nsContentUtils::IsInSameAnonymousTree would be bit slower.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  Let's just reuse nsContentUtils::ContentIsDescendantOf.

And maybe we should add more booleans like IsElement() on nsINode...

Comment 14

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

Attachment: patch

Comment 15

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

http://hg.mozilla.org/mozilla-central/rev/3dc687a271e7

Comment 16

[Justin Wood (:Callek)]

FYI, I landed a followup fix [on comm-central] to break bustage for Thunderbird and SeaMonkey.

(Admitedly the comm-* code that depends on this stuff is very very ugly) http://hg.mozilla.org/comm-central/rev/a5f596801e38

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 557929 [details] [diff] [review]
patch

Review of attachment 557929 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +5544,5 @@
> +    return PR_FALSE;
> +  }
> +
> +  const nsIContent* other = static_cast<const nsIContent*>(aOther);
> +  if (this == GetOwnerDoc()) {

This is *very* confusing I think. If I understand it correctly, we want to check if |this| is a document. right? Isn't there more explicit way to do so?

Comment 18

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

IsNodeOfType(nsINode::eDOCUMENT) but that is virtual call.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I guess NodeType() == nsIDOMNode::DOCUMENT_NODE is fast now.

Comment 20

[Boris Zbarsky [:bzbarsky]]

We should consider adding IsDocument() on nsINode and backing it with either a boolean flag (we have a bunch of free ones) or the NodeType() check Ms2ger suggests.

Comment 22

[bugzilla33]

Not fixed,
Pleasre REOPEN, because:

http://host0001.webd.pl/bugs/firefox/contains.html?

theSameElem.contains(theSameElem) should returns TRUE

IE 9, Opera 11.5, Konqueror 4.5, Chrome 13, Safari 5.1 return TRUE
FF 9a1 returns FALSE

Comment 24

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/DOM/Node.contains

Mentioned on Firefox 9 for developers.