Last Comment Bug 361366 - problems with xf:repeat in anonymous content
: problems with xf:repeat in anonymous content
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-11-21 01:38 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (9.22 KB, patch)
2006-11-22 06:05 PST, alexander :surkov
doronr: review+
Details | Diff | Splinter Review
testcase (1.80 KB, application/xhtml+xml)
2006-11-28 22:01 PST, alexander :surkov
no flags Details
patch2 (8.54 KB, patch)
2006-12-04 00:52 PST, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
testcase2 (1.31 KB, application/xhtml+xml)
2006-12-06 13:27 PST, aaronr
no flags Details

Description alexander :surkov 2006-11-21 01:38:20 PST
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 2006-11-21 02:31:41 PST
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 2006-11-21 10:23:06 PST
(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 2006-11-21 18:31:04 PST
(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 2006-11-22 00:58:10 PST
(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 2006-11-22 04:17:23 PST
(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 2006-11-22 06:05:38 PST
Created attachment 246274 [details] [diff] [review]
patch
Comment 7 alexander :surkov 2006-11-28 22:01:41 PST
Created attachment 246888 [details]
testcase
Comment 8 aaronr 2006-11-30 17:25:51 PST
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 2006-12-04 00:52:05 PST
Created attachment 247394 [details] [diff] [review]
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 2006-12-06 13:20:56 PST
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 2006-12-06 13:27:26 PST
Created attachment 247722 [details]
testcase2

This testcase does not show the nested repeat's outputs with the current patch.
Comment 12 alexander :surkov 2006-12-09 05:25:28 PST
(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 2006-12-10 15:57:37 PST
(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 2006-12-10 16:21:28 PST
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 2006-12-11 03:15:26 PST
checked in by smaug
Comment 16 aaronr 2007-04-23 15:54:01 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.