The default bug view has changed. See this FAQ.

Implement Node.contains(node)

RESOLVED FIXED in mozilla9



DOM: Core & HTML
6 years ago
6 years ago


(Reporter: mounir, Assigned: smaug)




Firefox Tracking Flags

(Not tracked)




(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
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

6 years ago
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

6 years ago
Sorry Alex. I was in the train and started to write patch for this...
Assignee: nobody → Olli.Pettay

Comment 4

6 years ago
Created attachment 557890 [details] [diff] [review]

Trying to avoid virtual calls, but GetBindingParent is there, and in
some cases IsNodeOfType.
Attachment #557890 - Flags: review?(bzbarsky)

Comment 5

6 years ago
Comment on attachment 557890 [details] [diff] [review]

>+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

6 years ago
Er, GetCurrentDoc != aOther->GetCurrentDoc()

Comment 7

6 years ago
Actually both.

Comment 8

6 years ago
Bah, new patch coming.
Does that mean you don't want review on that one?

Comment 10

6 years ago
Created attachment 557912 [details] [diff] [review]
Attachment #557890 - Attachment is obsolete: true
Attachment #557890 - Flags: review?(bzbarsky)
Attachment #557912 - Flags: review?(bzbarsky)
Comment on attachment 557912 [details] [diff] [review]

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.
Attachment #557912 - Flags: review?(bzbarsky) → review+

Comment 12

6 years ago
Ah, I could reuse nsContentUtils::ContentIsDescendantOf, but 
nsContentUtils::IsInSameAnonymousTree would be bit slower.
Yeah, ok.  Let's just reuse nsContentUtils::ContentIsDescendantOf.

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

Comment 14

6 years ago
Created attachment 557929 [details] [diff] [review]
Keywords: dev-doc-needed

Comment 15

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
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)


6 years ago
Depends on: 684458

Comment 17

6 years ago
Comment on attachment 557929 [details] [diff] [review]

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

6 years ago
IsNodeOfType(nsINode::eDOCUMENT) but that is virtual call.
I guess NodeType() == nsIDOMNode::DOCUMENT_NODE is fast now.
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.


6 years ago
Duplicate of this bug: 685086

Comment 22

6 years ago
Not fixed,
Pleasre REOPEN, because:

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
Duplicate of this bug: 685086
Docs updated:

Mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete


6 years ago
Duplicate of this bug: 698304


6 years ago
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.