Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement Node.contains(node)

RESOLVED FIXED in mozilla9

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: smaug)

Tracking

({dev-doc-complete})

Trunk
mozilla9
All
Linux
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(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)?
(Assignee)

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).
(Assignee)

Comment 3

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

Comment 4

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

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

Comment 5

6 years ago
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()
(Assignee)

Comment 6

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

Comment 7

6 years ago
Actually both.
(Assignee)

Comment 8

6 years ago
Bah, new patch coming.

Comment 9

6 years ago
Does that mean you don't want review on that one?
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

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

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

Comment 14

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

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/3dc687a271e7
Status: NEW → RESOLVED
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) http://hg.mozilla.org/comm-central/rev/a5f596801e38

Updated

6 years ago
Depends on: 684458
(Reporter)

Comment 17

6 years ago
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?
(Assignee)

Comment 18

6 years ago
IsNodeOfType(nsINode::eDOCUMENT) but that is virtual call.
I guess NodeType() == nsIDOMNode::DOCUMENT_NODE is fast now.

Comment 20

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

Updated

6 years ago
Duplicate of this bug: 685086

Comment 22

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

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

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

Updated

6 years ago
Duplicate of this bug: 698304

Updated

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