284002 - NodeXBL implementation

Status: RESOLVED WONTFIX

(Core :: XBL, enhancement)

Description

[Brian Ryner (not reading)]

sXBL and XBL2 define a NodeXBL DOM interface
(http://www.w3.org/TR/sXBL/#the-nodexbl).  By implementing this, we can take a
step towards implementing these specs while also making direct use of the
binding manager unnecessary, which should allow it to be deCOMified.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'll take a look in more detail later, but we can't freeze this interface yet,
can we?  sXBL/XBL2 aren't even REC yet.

Comment 3

[Brian Ryner (not reading)]

True, though Hixie told me it was "not likely to change"... maybe that's not
sufficient.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think that's sufficient for UNDER_REVIEW, not a FROZEN.

Comment 5

[Hixie (not reading bugmail)]

I apologise if I misled you. I shouldn't make any claims about XBL being stable.
For all I know the entire interface could get dropped -- the XBL task force is a
little volatile. But it would be good for MozXBL, whatever happens to W3CXBL. :-)

Comment 6

[Brian Ryner (not reading)]

Ok, assume that I've changed this to UNDER_REVIEW locally.

Comment 7

[Brian Ryner (not reading)]

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

new patch forthcoming

Comment 8

[Brian Ryner (not reading)]

Attachment: get rid of some patch noise and fix up accessibility code

This patch moves the walk-anonymous-content flag back to
nsAccessibleTreeWalker.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note to self: use http://hixie.ch/specs/xbl/sXBL.html when reviewing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175911 [details] [diff] [review]
get rid of some patch noise and fix up accessibility code

>Index: content/base/src/nsGenericElement.cpp

>+nsNodeXBLTearoff::GetXblParentNode(nsIDOMNode **aNode)

This isn't right.  For starters, it returns the wrong thing for the root node
in a document (null instead of the document node).  It should probably just
call GetParentNode on mContent if there is no insertion parent.

Apart from that, there are some special rules in sXBL as to what the
xblParentNode should be when the node is a child of a <content> element that's
a child of the <shadowTree> element.  This patch doesn't seem to implement
those rules.

Past that I'm still trying to sort out whether GetInsertionParent actually
returns the right thing for us, but I think it probably does (and if not, we
should fix it to do so).

>+nsNodeXBLTearoff::GetXblParentNodes(nsIDOMNodeList **aNodeList)

Why are we implementing this method?  It doesn't seem to be part of the NodeXBL
interface in sXBL....

>+nsXBLChildNodeList::GetCurrentNodeList(nsIDOMNodeList **aNodeList)
>+{
>+  nsIDocument *ownerDoc = mContent->GetOwnerDoc();
>+  if (ownerDoc) {
>+    return ownerDoc->BindingManager()->GetXBLChildNodesFor(mContent,
>+                                                           aNodeList);
>+  }

I don't believe this does the same thing as what the sXBL spec says for the
xblChildNodes method... In particular, given a mix of anonymous kids and
explicit kids bound via <children> (in Mozilla XBL) it'll just return the
former; in sXBL it should be returning all nodes whose xblParentNode is |this|.

Also, if there is no binding involved, it looks to me like this will never
actually get the childNodes of mContent.  Compare the end of
nsBindingManager::GetXBLChildNodesInternal to the end of
nsBindingManager::GetContentListFor.

>+nsNodeXBLTearoff::GetXblScopedChildNodes(nsIDOMNodeList **aNodeList)

I can't tell what the spec is trying to say for this attribute... I've raised
the issue with the WG.

>+nsNodeXBLTearoff::GetXblPreviousSibling(nsIDOMNode **aSibling)
>+  nsCOMPtr<nsIDOMNodeXBL> xblParentXBL = do_QueryInterface(xblParent);
>+  NS_ENSURE_TRUE(xblParentXBL, NS_ERROR_FAILURE);

This should return NS_OK (and probably not use NS_ENSURE_TRUE), per spec (null
return if the xblParentNode is null).

>+nsNodeXBLTearoff::GetXblNextSibling(nsIDOMNode **aSibling)
>+  nsCOMPtr<nsIDOMNodeXBL> xblParentXBL = do_QueryInterface(xblParent);
>+  NS_ENSURE_TRUE(xblParentXBL, NS_ERROR_FAILURE);

Same.

>+nsNodeXBLTearoff::GetXblPreviousElementSibling(nsIDOMElement **aSibling)
>+  nsCOMPtr<nsIDOMNodeXBL> xblParentXBL = do_QueryInterface(xblParent);
>+  NS_ENSURE_TRUE(xblParentXBL, NS_ERROR_FAILURE);

Same.

>+nsNodeXBLTearoff::GetXblNextElementSibling(nsIDOMElement **aSibling)
>+  nsCOMPtr<nsIDOMNodeXBL> xblParentXBL = do_QueryInterface(xblParent);
>+  NS_ENSURE_TRUE(xblParentXBL, NS_ERROR_FAILURE);

Same.

>+nsNodeXBLTearoff::GetXblBoundElement(nsIDOMElement **aElement)
>+    nsXBLBinding *binding = ownerDoc->BindingManager()->GetBinding(mContent);
>+    if (binding) {
>+      nsIContent *boundElement = binding->GetBoundElement();

Er... This is just going to return mContent, no?  Don't you want to use
GetBindingParent here instead?

>+nsNodeXBLTearoff::GetXblShadowTree(nsIDOMElement **aElement)
>+  return NS_ERROR_NOT_IMPLEMENTED;

We could actually come pretty close to implementing this...  we have the
corresponding node; it's just that its kids don't think they're its kids.

>+nsNodeXBLTearoff::AddBinding(const nsAString &aBindingURI)

I don't see this method in sXBL...

>+nsNodeXBLTearoff::RemoveBinding(const nsAString &aBindingURI)

Nor this one.

>+nsNodeXBLTearoff::HasBinding(const nsAString &aBindingURI, PRBool *aResult)

Nor this.

>@@ -3159,6 +3676,7 @@ NS_INTERFACE_MAP_BEGIN(nsGenericElement)
>   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3Node, new nsNode3Tearoff(this))
>+  NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNodeXBL, new nsNodeXBLTearoff(this))
>   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventReceiver,
>                                  nsDOMEventRTTearoff::Create(this))
>   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventTarget,

I think QI to nsIDOMEventTarget and nsIDOMEventReceiver is a lot more likely
than to nsIDOMNodeXBL, at least for now....

>Index: dom/public/idl/xbl/nsIDOMNodeXBL.idl

This doesn't match the interface in the sXBL draft (has three extra methods at
the end, and the extra xblParentNodes property).

>Index: extensions/inspector/base/src/inDOMUtils.cpp
>@@ -131,14 +132,10 @@ inDOMUtils::GetParentForNode(nsIDOMNode*
>-    if (bindingManager) {
>-      bindingManager->GetInsertionParent(content, getter_AddRefs(bparent));
>+    nsCOMPtr<nsIDOMNodeXBL> xblNode = do_QueryInterface(aNode);

I don't think that's equivalent, given our NodeXBL implementation, if aNode is
a Text node...

>Index: extensions/inspector/base/src/inDeepTreeWalker.cpp
>+      nsCOMPtr<nsIDOMNodeXBL> xblNode = do_QueryInterface(aNode);
>+      if (xblNode) {
>+	xblNode->GetXblChildNodes(getter_AddRefs(kids));
>       } else {
>         aNode->GetChildNodes(getter_AddRefs(kids));

Weird indent (tabs?).  And I don't think we need the |else| here, really... any
time that QI fails either we can't have child nodes or we need to throw
NS_ERROR_OUT_OF_MEMORY or something.

Comment 11

[Hixie (not reading bugmail)]

Note that a proposal I have on the table for sXBL redefines some of these
methods a little:
   http://hixie.ch/specs/xbl/issues/shadow-css.html#sec-proposals-Combo-Method

I think we should be implementing those proposals.

Comment 12

[Brian Ryner (not reading)]

I can address a number of the comments at once by noting that what I was
actually implementing is http://www.hixie.ch/specs/xbl/XBL2.html#the-nodexbl,
which is a superset of sXBL.

Also, I had no intent at this time to implement anything dealing with <shadowTree>.

On to some more specific things:

(In reply to comment #10)
> (From update of attachment 175911 [details] [diff] [review] [edit])
> >Index: content/base/src/nsGenericElement.cpp
> 
> >+nsNodeXBLTearoff::GetXblParentNode(nsIDOMNode **aNode)
> 
> This isn't right.  For starters, it returns the wrong thing for the root node
> in a document (null instead of the document node).  It should probably just
> call GetParentNode on mContent if there is no insertion parent.

Agreed.

> >+nsXBLChildNodeList::GetCurrentNodeList(nsIDOMNodeList **aNodeList)
> >+{
> >+  nsIDocument *ownerDoc = mContent->GetOwnerDoc();
> >+  if (ownerDoc) {
> >+    return ownerDoc->BindingManager()->GetXBLChildNodesFor(mContent,
> >+                                                           aNodeList);
> >+  }
> 
> I don't believe this does the same thing as what the sXBL spec says for the
> xblChildNodes method... In particular, given a mix of anonymous kids and
> explicit kids bound via <children> (in Mozilla XBL) it'll just return the
> former; in sXBL it should be returning all nodes whose xblParentNode is |this|.
> 
> Also, if there is no binding involved, it looks to me like this will never
> actually get the childNodes of mContent.  Compare the end of
> nsBindingManager::GetXBLChildNodesInternal to the end of
> nsBindingManager::GetContentListFor.
> 

To solve both of these, I think I will change this to make use of
nsChildIterator, which has this exact purpose.

> >+nsNodeXBLTearoff::GetXblScopedChildNodes(nsIDOMNodeList **aNodeList)
> 
> I can't tell what the spec is trying to say for this attribute... I've raised
> the issue with the WG.
> 

I came away with a rough understanding after talking about it with hixie on irc
for some time.  The basic idea is just that you ignore any content that is
inserted before/after child nodes via their bindings.


> >+nsNodeXBLTearoff::GetXblPreviousSibling(nsIDOMNode **aSibling)
> >+  nsCOMPtr<nsIDOMNodeXBL> xblParentXBL = do_QueryInterface(xblParent);
> >+  NS_ENSURE_TRUE(xblParentXBL, NS_ERROR_FAILURE);
> 
> This should return NS_OK (and probably not use NS_ENSURE_TRUE), per spec (null
> return if the xblParentNode is null).

OK (to all of these).

> >+nsNodeXBLTearoff::GetXblBoundElement(nsIDOMElement **aElement)
> >+    nsXBLBinding *binding = ownerDoc->BindingManager()->GetBinding(mContent);
> >+    if (binding) {
> >+      nsIContent *boundElement = binding->GetBoundElement();
> 
> Er... This is just going to return mContent, no?  Don't you want to use
> GetBindingParent here instead?
> 

I think so, yes.

> >@@ -3159,6 +3676,7 @@ NS_INTERFACE_MAP_BEGIN(nsGenericElement)
> >   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3Node, new nsNode3Tearoff(this))
> >+  NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNodeXBL, new nsNodeXBLTearoff(this))
> >   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventReceiver,
> >                                  nsDOMEventRTTearoff::Create(this))
> >   NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventTarget,
> 
> I think QI to nsIDOMEventTarget and nsIDOMEventReceiver is a lot more likely
> than to nsIDOMNodeXBL, at least for now....
> 

Probably; how likely is QI to nsIDOM3Node?

> >@@ -131,14 +132,10 @@ inDOMUtils::GetParentForNode(nsIDOMNode*
> >-    if (bindingManager) {
> >-      bindingManager->GetInsertionParent(content, getter_AddRefs(bparent));
> >+    nsCOMPtr<nsIDOMNodeXBL> xblNode = do_QueryInterface(aNode);
> 
> I don't think that's equivalent, given our NodeXBL implementation, if aNode is
> a Text node...

Can you elaborate?  One of the main reasons for this addition is to get
extensions off of depending on interfaces like nsIBindingManager.

> 
> >Index: extensions/inspector/base/src/inDeepTreeWalker.cpp
> >+      nsCOMPtr<nsIDOMNodeXBL> xblNode = do_QueryInterface(aNode);
> >+      if (xblNode) {
> >+	xblNode->GetXblChildNodes(getter_AddRefs(kids));
> >       } else {
> >         aNode->GetChildNodes(getter_AddRefs(kids));
> 
> Weird indent (tabs?).  And I don't think we need the |else| here, really... any
> time that QI fails either we can't have child nodes or we need to throw
> NS_ERROR_OUT_OF_MEMORY or something.
> 

Agreed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Brian, I was using the spec that Ian pointed me to as being the stable version
of this interface, with stable descriptions of what the properties do -- see
comment 9.  I realize that sXBL and XBL2 differ as far as this interface goes...
and that's a problem if we're considering doing anything like freezing it.

> The basic idea is just that you ignore any content that is inserted
> before/after child nodes via their bindings.

But the bindings of child nodes can't affect what their siblings are in the
fully flattened tree.  From looking at the example in the sXBL specification, it
looks like the intent here is that this is the child list _before_ any
relocation of content via <content> nodes for the element's binding... so
basically a list of the element's non-anonymous kids combined with the list of
the kids of the element's shadowTree root.  If that's the case, great, but then
the spec doesn't seem to clearly define what order the two lists should be
interleaved in.  The incomprehensible definition of this attribute was one of
the questions I raised on www-svg while reviewing this patch.

> how likely is QI to nsIDOM3Node?

Not that common, I think...  Certainly not common for web content, since IE
doesn't support DOM3.

>> >-    if (bindingManager) {
>> >-      bindingManager->GetInsertionParent(content, getter_AddRefs(bparent));
>> >+    nsCOMPtr<nsIDOMNodeXBL> xblNode = do_QueryInterface(aNode);
>> 
>> I don't think that's equivalent, given our NodeXBL implementation, if aNode
>> is a Text node...
> Can you elaborate?

Sure.  Your patch makes only GenericElement nodes implement NodeXBL.  So if
aNode is a Text node, that QI will always fail.  On the other hand, the binding
manager deals with insertion parents for all sorts of nsIContent nodes
(including text nodes, comment nodes, etc)...

Comment 14

[Hixie (not reading bugmail)]

sXBL and XBL2 are still in WD status. _Whatever_ you implement, it could change.
And almost certainly will. (In fact, XBL2 isn't even WD yet.)

Comment 15

[Brian Ryner (not reading)]

One idea, since XBL2 is a superset, would be:

interface nsIDOMNodeXBL : nsISupports
{
  // sXBL methods and properties
};

interface nsIDOMNodeXBL2 : nsIDOMNodeXBL
{
  // XBL2-specific methods and properties
};

That way, the sXBL interface can be frozen when the spec is, even if XBL2 is not
frozen... assuming that XBL2 only adds to sXBL and does not redefine any
existing behavior.

Comment 16

[Hixie (not reading bugmail)]

sXBL will indeed be a strict subset of XBL2. At least if I have anything to do
with it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

XBL is going away, and shadow DOM has other APIs for this.