361366 - problems with xf:repeat in anonymous content

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

xforms:repeat cannot be used in anonymous content, for example:

<content>
  <children include="label"/>
  <xf:repeat xbl:inherits="model, bind, nodeset">
    <children/>
    <xf:trigger>
      <xf:label>R</xf:label>
      <xf:delete ev:event="DOMActivate" at="index()"
                 xbl:inherits="model, bind, nodeset"/>
    </xf:trigger>
  </xf:repeat>
</content>

mozType:attrBased="true" placed directly on xforms:repeat will not work right here. I guess we can do some magic like DOMi do: http://lxr.mozilla.org/mozilla/source/layout/inspector/src/inDOMView.cpp#1240

Also, this will release us from mozType:attrBased stuff.

Comment 1

[alexander :surkov (:asurkov)]

The other problem of repeat using in anonymous content is index() function. If repeat is placed inside anonymous content then how can index() function refer on that repeat. We can't use id attribute to idenfify repeat. Can we assume that index() method is referred on parent repeat if argument of index() is missed?

Comment 2

[aaronr]

(In reply to comment #1)
> The other problem of repeat using in anonymous content is index() function. If
> repeat is placed inside anonymous content then how can index() function refer
> on that repeat. We can't use id attribute to idenfify repeat. Can we assume
> that index() method is referred on parent repeat if argument of index() is
> missed?
> 

Not according to spec.  According to spec, index must have a parameter.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (In reply to comment #1)
> > The other problem of repeat using in anonymous content is index() function. If
> > repeat is placed inside anonymous content then how can index() function refer
> > on that repeat. We can't use id attribute to idenfify repeat. Can we assume
> > that index() method is referred on parent repeat if argument of index() is
> > missed?
> > 
> 
> Not according to spec.  According to spec, index must have a parameter.
> 

Then, can we assume that 'anonid' attribute that is used inside anonymous content is ID attribute? In this case we can specify 'anonid' as argument of index() method.

Comment 4

[aaronr]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > The other problem of repeat using in anonymous content is index() function. If
> > > repeat is placed inside anonymous content then how can index() function refer
> > > on that repeat. We can't use id attribute to idenfify repeat. Can we assume
> > > that index() method is referred on parent repeat if argument of index() is
> > > missed?
> > > 
> > 
> > Not according to spec.  According to spec, index must have a parameter.
> > 
> 
> Then, can we assume that 'anonid' attribute that is used inside anonymous
> content is ID attribute? In this case we can specify 'anonid' as argument of
> index() method.
> 

I don't think that I'd have a problem with that, as long as the precedence of the search would be for regular DOM id's first and then if that isn't found, then via anonid.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> > Then, can we assume that 'anonid' attribute that is used inside anonymous
> > content is ID attribute? In this case we can specify 'anonid' as argument of
> > index() method.
> > 
> 
> I don't think that I'd have a problem with that, as long as the precedence of
> the search would be for regular DOM id's first and then if that isn't found,
> then via anonid.
> 

I filed bug 361501 for this.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 8

[aaronr]

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

>Index: extensions/xforms/nsXFormsRepeatElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRepeatElement.cpp,v
>retrieving revision 1.38
>diff -u -8 -p -r1.38 nsXFormsRepeatElement.cpp
>--- extensions/xforms/nsXFormsRepeatElement.cpp	8 Oct 2006 14:15:01 -0000	1.38
>+++ extensions/xforms/nsXFormsRepeatElement.cpp	22 Nov 2006 14:03:53 -0000
>@@ -42,23 +42,22 @@
> #include "nsISchema.h"
> #include "nsIServiceManager.h"
> #include "nsMemory.h"
> #include "nsString.h"
> #include "nsSubstring.h"
> 
> #include "nsIDOM3EventTarget.h"
> #include "nsIDOM3Node.h"
>-#include "nsIDOMDOMImplementation.h"
> #include "nsIDOMDocument.h"
> #include "nsIDOMElement.h"
>-#include "nsIDOMEventTarget.h"
> #include "nsIDOMNodeList.h"
> #include "nsIDOMXPathResult.h"
>-#include "nsIDOMDocumentXBL.h"
>+#include "nsIContent.h"
>+#include "nsIBindingManager.h"
> 
> #include "nsXFormsControlStub.h"
> #include "nsIXFormsContextControl.h"
> #include "nsIXFormsRepeatElement.h"
> #include "nsIXFormsRepeatUIElement.h"
> #include "nsIXFormsRepeatItemElement.h"
> #include "nsXFormsAtoms.h"
> #include "nsXFormsModelElement.h"
>@@ -234,22 +233,16 @@ protected:
>    * Is the repeat attribute-based (ie. created by using xf:repeat-*
>    * attributes).
>    *
>    * @see http://www.w3.org/TR/xforms/slice9.html#ui.repeat.via.attrs
>    */
>   PRPackedBool mIsAttributeBased;
> 
>   /**
>-   * A clone of the parent node (used when mIsAttributeBased), that is used by
>-   * InsertTemplateContent().
>-   */
>-  nsCOMPtr<nsIDOMNode> mParentClone;
>-
>-  /**
>    * The currently selected repeat (nested repeats)
>    */
>   nsCOMPtr<nsIXFormsRepeatElement> mCurrentRepeat;
> 
>   /**
>    * Array of controls using the repeat-index
>    */
>   nsCOMArray<nsIXFormsControl>     mIndexUsers;
>@@ -321,25 +314,16 @@ protected:
> 
>   /**
>    * Insert the template content for a repeat node into the given node.
>    *
>    * @param aNode             The node to insert content into.
>    */
>   nsresult InsertTemplateContent(nsIDOMNode *aNode);
> 
>-
>-  /**
>-   * Get the XBL binding parent for a node.
>-   *
>-   * @param aNode              The node to get binding parent for
>-   * @param aParent            The returned binding parent
>-   */
>-  nsresult GetBindingParent(nsIDOMNode *aNode, nsIDOMNode **aParent);
>-
>   // nsXFormsControlStub override
>   virtual void AfterSetAttribute(nsIAtom *aName);
> 
> public:
>   NS_DECL_ISUPPORTS_INHERITED
> 
>   // nsIXTFElement overrides
>   NS_IMETHOD OnCreated(nsIXTFElementWrapper *aWrapper);
>@@ -749,91 +733,90 @@ nsXFormsRepeatElement::Bind(PRBool *aCon
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   *aContextChanged = PR_TRUE;
> 
>   return NS_OK;
> }
> 
> nsresult
>-nsXFormsRepeatElement::GetBindingParent(nsIDOMNode *aNode,
>-                                        nsIDOMNode **aParent)
>-{
>-  NS_ENSURE_ARG_POINTER(aNode);
>-  NS_ENSURE_ARG_POINTER(aParent);
>-
>-  nsCOMPtr<nsIDOMDocument> domDoc;
>-  aNode->GetOwnerDocument(getter_AddRefs(domDoc));
>-  nsCOMPtr<nsIDOMDocumentXBL> docXBL(do_QueryInterface(domDoc));
>-  NS_ENSURE_STATE(docXBL);
>-  
>-  nsCOMPtr<nsIDOMElement> parent;
>-  docXBL->GetBindingParent(aNode, getter_AddRefs(parent));
>-  NS_IF_ADDREF(*aParent = parent);
>-  return NS_OK;
>-}
>-
>-nsresult
> nsXFormsRepeatElement::InsertTemplateContent(nsIDOMNode *aNode)
> {
>   NS_ENSURE_ARG_POINTER(aNode);
> 
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
>+  NS_ENSURE_STATE(content);
>+
>   nsresult rv;
>-  nsCOMPtr<nsIDOMNode> insertionPoint;
> 
>   // If we are attribute based we need to clone our parent (the element with
>   // the repeat-* attributes), and insert that as the first child.
>   if (mIsAttributeBased) {
>-    NS_ENSURE_STATE(mParentClone);
>-    nsCOMPtr<nsIDOMNode> parentClone;
>-    rv = mParentClone->CloneNode(PR_FALSE, getter_AddRefs(parentClone));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIContent> templateContent = content->GetBindingParent();
>+    NS_ENSURE_STATE(templateContent);
> 
>-    rv = aNode->AppendChild(parentClone, getter_AddRefs(insertionPoint));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  } else {
>-    insertionPoint = aNode;
>-  }
>+    nsCOMPtr<nsIDOMNode> templateNode(do_QueryInterface(templateContent));
>+    NS_ENSURE_STATE(templateNode);
> 
>-  // Get template content
>-  nsCOMPtr<nsIDOMNode> child;
>-  if (mIsAttributeBased) {
>-    // If we are attribute based, we need to get our binding parent and get
>-    // its first child or we will be looking at the binding document.
>-    nsCOMPtr<nsIDOMNode> parent;
>-    rv = GetBindingParent(mElement, getter_AddRefs(parent));
>+    nsCOMPtr<nsIDOMNode> clonedNode;
>+    rv = CloneNode(templateNode, getter_AddRefs(clonedNode));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    rv = parent->GetFirstChild(getter_AddRefs(child));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  } else {
>-    rv = mElement->GetFirstChild(getter_AddRefs(child));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>+    nsCOMPtr<nsIDOMElement> clonedElm(do_QueryInterface(clonedNode));
>+    NS_ENSURE_STATE(clonedElm);
> 
>-  // Clone template
>-  while (child) {
>-    nsCOMPtr<nsIDOMNode> childClone;
>-    rv = CloneNode(child, getter_AddRefs(childClone));
>+    // Remove all possible repeat-* attributes
>+    clonedElm->RemoveAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("repeat-model"));
>+    clonedElm->RemoveAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("repeat-bind"));
>+    clonedElm->RemoveAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("repeat-nodeset"));
>+    clonedElm->RemoveAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("repeat-startindex"));
>+    clonedElm->RemoveAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("repeat-number"));

Why did you move this code from UnrollRows?  Instead of removing the attributes once and cloning from that, we are doing the cloning first and then removing the attributes.  So we'll end up removing the attributes once for every row in the repeat, right?  A lot less efficient.

>+
>+    nsCOMPtr<nsIDOMNode> stubNode;
>+    return aNode->AppendChild(clonedNode, getter_AddRefs(stubNode));
>+  }
>+
>+  nsCOMPtr<nsIDOMDocument> domDocument;
>+  mElement->GetOwnerDocument(getter_AddRefs(domDocument));
>+  NS_ENSURE_STATE(domDocument);
>+
>+  nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocument));
>+  nsCOMPtr<nsIBindingManager> bindingMgr(document->BindingManager());
>+  NS_ENSURE_STATE(bindingMgr);
>+
>+  nsCOMPtr<nsIDOMNodeList> children;
>+  bindingMgr->GetContentListFor(content, getter_AddRefs(children));

You'll need comments here about why we are using this now instead of getting the children explicitly via DOM.  Is this list of children guaranteed to be in document order?

Removing the review request pending answers to questions.  The logic of the patch seems ok other than these things.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #8)

> Why did you move this code from UnrollRows?  Instead of removing the attributes
> once and cloning from that, we are doing the cloning first and then removing
> the attributes.  So we'll end up removing the attributes once for every row in
> the repeat, right?  A lot less efficient.

I missed that optimization issue. Fixed.

> You'll need comments here about why we are using this now instead of getting
> the children explicitly via DOM.  Is this list of children guaranteed to be in
> document order?

I guess yes. That's how DOMi works :)

Comment 10

[aaronr]

Comment on attachment 247394 [details] [diff] [review]
patch2


nit: patch has window line endings.

r-'ing because nested attribute-based repeats stop working with this patch.  I'll attach a testcase.

Comment 11

[aaronr]

Attachment: testcase2

This testcase does not show the nested repeat's outputs with the current patch.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Created an attachment (id=247722) [edit]
> testcase2
> 
> This testcase does not show the nested repeat's outputs with the current patch.
> 

The same problem like in bug 356790. There is no XBL binding for nested repeats though binding style rules are there.

Comment 13

[aaronr]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=247722) [edit]
> > testcase2
> > 
> > This testcase does not show the nested repeat's outputs with the current patch.
> > 
> 
> The same problem like in bug 356790. There is no XBL binding for nested repeats
> though binding style rules are there.
> 


I tried this patch on 1.8 branch and nested repeats work, so nothing to do with this patch.  Must be something on trunk.

Comment 14

[aaronr]

Comment on attachment 247394 [details] [diff] [review]
patch2

since the inner repeat issue I noted isn't caused by this patch, I'm r+'ing the patch.

Please make sure to fix the windows line endings prior to checkin.

Comment 15

[alexander :surkov (:asurkov)]

checked in by smaug

Comment 16

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16