problems with xf:repeat in anonymous content

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

1.80 KB, application/xhtml+xml
Details
8.54 KB, patch
aaronr
: review+
Details | Diff | Splinter Review
1.31 KB, application/xhtml+xml
Details
(Assignee)

Description

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

Comment 1

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

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

Comment 3

11 years ago
(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

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

Comment 5

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

Comment 6

11 years ago
Created attachment 246274 [details] [diff] [review]
patch
Assignee: xforms → surkov.alexander
Status: NEW → ASSIGNED
Attachment #246274 - Flags: review?(aaronr)
(Assignee)

Updated

11 years ago
Attachment #246274 - Flags: review?(doronr)
(Assignee)

Comment 7

11 years ago
Created attachment 246888 [details]
testcase

Updated

11 years ago
Attachment #246274 - Flags: review?(doronr) → review+

Comment 8

11 years ago
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.
Attachment #246274 - Flags: review?(aaronr)
(Assignee)

Comment 9

11 years ago
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 :)
Attachment #246274 - Attachment is obsolete: true
Attachment #247394 - Flags: review?(aaronr)

Comment 10

11 years ago
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.
Attachment #247394 - Flags: review?(aaronr) → review-

Comment 11

11 years ago
Created attachment 247722 [details]
testcase2

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

Comment 12

11 years ago
(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

11 years ago
(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

11 years ago
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.
Attachment #247394 - Flags: review- → review+
(Assignee)

Comment 15

11 years ago
checked in by smaug
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 16

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.