282828 - Modify repeat-index on insert and delete

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P1)

Description

[Allan Beaufour]

The repeat-index needs to be updated to point to the newly inserted node on an
insert, and also point to the correct node after a delete.

Comment 1

[Allan Beaufour]

Attachment: Testcase 1

Comment 2

[Allan Beaufour]

Attachment: Testcase 2

(filed bug 302922 for the index error in this)

Comment 3

[Allan Beaufour]

Attachment: Testcase 3

Comment 4

[Allan Beaufour]

(In reply to comment #2)
> Created an attachment (id=191197) [edit]
> Testcase 2
> 
> (filed bug 302922 for the index error in this)

argh, that is bug 302918.

Comment 5

[Peter Nunn]

Have sent fixes as patch to A.Beaufor for integration, added patches here.

Comment 6

[Peter Nunn]

Attachment: Test form to show insert/delete and setindex working

Comment 7

[Peter Nunn]

Attachment: Added constants found in files to header

Comment 8

[Peter Nunn]

Attachment: Changed common names to use constants in header, added code for 9.3.8 compliance

Comment 9

[Peter Nunn]

Attachment: Added support to set index on all repeats that have a relevant row

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Have sent fixes as patch to A.Beaufor for integration, added patches here.

Could you please create one patch and use -u8p:
cvs diff -u8p extensions/xforms/
or
diff -U 8 -p original/mozilla/code/extensions/xforms your/code/extensions/xforms

And use unix line separators '\n', not Windows '\r\n'.

Comment 11

[Peter Nunn]

Attachment: Patch to resolve issue

Comment 12

[Peter Nunn]

Comment on attachment 194751 [details] [diff] [review]
Patch to resolve issue

Index: extensions/xforms/.cvsignore
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/.cvsignore,v
retrieving revision 1.2
diff -u -8 -p -r1.2 .cvsignore
--- extensions/xforms/.cvsignore	13 Oct 2004 09:51:52 -0000	1.2
+++ extensions/xforms/.cvsignore	3 Sep 2005 21:37:01 -0000
@@ -1 +1,5 @@
 Makefile
+xforms.sln
+xforms.vcproj
+xforms.ncb
+xforms.suo
Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
retrieving revision 1.3
diff -u -8 -p -r1.3 nsXFormsInsertDeleteElement.cpp
--- extensions/xforms/nsXFormsInsertDeleteElement.cpp	2 Aug 2005 14:56:42
-0000	    1.3
+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	3 Sep 2005 21:30:23
-0000
@@ -34,23 +34,26 @@
  *
  * ***** END LICENSE BLOCK ***** */

 #include "nsIDOMEvent.h"
 #include "nsIDOMNode.h"
 #include "nsIDOMElement.h"
 #include "nsIDOMDocument.h"
 #include "nsIDOMNodeList.h"
+#include "nsIXFormsRepeatElement.h"
+#include "nsIXformsControl.h"

 #include "nsString.h"

 #include "nsIInstanceElementPrivate.h"
 #include "nsXFormsActionModuleBase.h"
 #include "nsXFormsActionElement.h"
 #include "nsXFormsUtils.h"
+#include "nsIDOM3Node.h"

 #include "math.h"

 #ifdef DEBUG
 //#define DEBUG_XF_INSERTDELETE
 #endif

 /**
@@ -58,18 +61,16 @@
  *
  * @see http://www.w3.org/TR/xforms/slice9.html#action-insert
  *
  * @todo The spec. states that the events must set their Context Info to:
  * "Path expression used for insert/delete (xsd:string)" (XXX)
  * @see http://www.w3.org/TR/xforms/slice4.html#evt-insert
  * @see https://bugzilla.mozilla.org/show_bug.cgi?id=280423
  *
- * @todo Any \<repeat\> elements need to set their repeat-indexes properly if
- * they are bound to the same nodeset. (XXX)
  */
 class nsXFormsInsertDeleteElement : public nsXFormsActionModuleBase
 {
 private:
   PRBool mIsInsert;

 public:
   NS_DECL_NSIXFORMSACTIONMODULEELEMENT
@@ -216,22 +217,130 @@ nsXFormsInsertDeleteElement::HandleActio
     NS_ENSURE_SUCCESS(rv, rv);
   }

   // Dispatch xforms-insert/delete event to the instance node we have modified
   // data for
   nsCOMPtr<nsIDOMElement> modelElem = do_QueryInterface(model);
   NS_ENSURE_STATE(modelElem);

+  // update all repeats with the inserted/deleted node
+  nsCOMPtr<nsIDOMDocument> document;
+
+  rv = mElement->GetOwnerDocument(getter_AddRefs(document));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // get all of the repeats.
+  nsCOMPtr<nsIDOMNodeList> repeatNodes;
+  document->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+    NS_LITERAL_STRING(NS_REPEAT), getter_AddRefs(repeatNodes));
+
+  // work over each node and if the node contains the deleted element,
+  // we assume that the node will no longer be in the tree, so apply the rules
+  // defined in 9.3.6
+  PRUint32 nodeCount;
+  rv = repeatNodes->GetLength(&nodeCount);
+
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  for(PRUint32 node = 0; node < nodeCount; node++)
+  {
+    nsCOMPtr<nsIDOMNode> repeatNode;
+
+    rv = repeatNodes->Item(node, getter_AddRefs(repeatNode));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // try cast
+    nsCOMPtr<nsIXFormsRepeatElement>
repeatElement(do_QueryInterface(repeatNode));
+    NS_ENSURE_STATE(repeatElement);
+
+    // each row is bound to the repeat as an anonymous child of the repeat.
+    // to find the context elements we need to get the html element
+    // then find the xforms:contextcontainer element which holds the child
+    // elements, but most importantly the context of the node that
+    // is bound.  If that context is the node being removed then we
+    // adjust the index.  When the node is refreshed, the index will then
point
+    // to the new node.
+
+    if(mIsInsert)
+    {
+      // AN issue here.  If we are inserting into the node, the nodeset will
+      // not have the new node in the list of inserted elements as the XPath
expression has
+      // not been evaluated.  We call the refresh on the repeat so we can
+      // evaluate the node binding after the xpath is reset.  This is not
really nice
+      // as it would be better if this is deferred till a model refresh.
+      nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(repeatElement));
+      NS_ENSURE_STATE(control);
+
+      rv = control->Refresh();
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    nsCOMPtr<nsIXTFVisual> visualElement(do_QueryInterface(repeatNode));
+    NS_ENSURE_STATE(repeatElement);
+
+    nsCOMPtr<nsIDOMElement> visualContext;
+    rv = visualElement->GetVisualContent(getter_AddRefs(visualContext));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // all of the child nodes of the type
+    nsCOMPtr<nsIDOMNodeList> childNodes;
+
+    rv =
visualContext->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+					       
NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
+						getter_AddRefs(childNodes));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    PRUint32 childCount;
+    rv = childNodes->GetLength(&childCount);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCOMPtr<nsIDOMNode> childNode;
+    for(PRUint32 childIndex = 0; childIndex < childCount; childIndex++)
+    {
+      rv = childNodes->Item(childIndex, getter_AddRefs(childNode));
+
+      nsCOMPtr<nsIXFormsContextControl>
riContext(do_QueryInterface(childNode));
+      NS_ENSURE_STATE(riContext);
+
+      // see what the context of the node is.	If it is our deleted node then
+      // we adjust the index so that when the control refreshes,
+      // we see the node prior or following being selected.
+      nsAutoString modelID;
+      PRInt32 position, size;
+      nsCOMPtr<nsIDOMNode> boundNode;
+
+      rv = riContext->GetContext(modelID, getter_AddRefs(boundNode),
&position, &size);
+
+      // if the node is the same node then we have found the deleted node on
this repeat.
+      nsCOMPtr<nsIDOM3Node> changedNode(do_QueryInterface(resNode));
+      NS_ENSURE_STATE(changedNode);
+
+      PRBool isSameNode;
+      changedNode->IsSameNode(boundNode, &isSameNode);
+      if(isSameNode)
+      {
+	 PRUint32 indexValue = childIndex + (mIsInsert ? 1 : 0);
+
+	 // we ask the node t refresh at this point so that the
+	 // inner indices are reset to 1.
+	 rv = repeatElement->SetIndex(&indexValue, PR_FALSE);
+	 NS_ENSURE_SUCCESS(rv, rv);
+
+	 break;        // all done on this repeat
+      }
+    }
+  }
+
   nsCOMPtr<nsIDOMNode> instNode;
   rv = nsXFormsUtils::GetInstanceNodeForData(resNode, model,
getter_AddRefs(instNode));
   NS_ENSURE_SUCCESS(rv, rv);

   rv = nsXFormsUtils::DispatchEvent(instNode,
-				     mIsInsert ? eEvent_Insert :
eEvent_Delete);
+    mIsInsert ? eEvent_Insert : eEvent_Delete);
   NS_ENSURE_SUCCESS(rv, rv);

   // Dispatch refreshing events to the model
   if (aParentAction) {
     aParentAction->SetRebuild(modelElem, PR_TRUE);
     aParentAction->SetRecalculate(modelElem, PR_TRUE);
     aParentAction->SetRevalidate(modelElem, PR_TRUE);
     aParentAction->SetRefresh(modelElem, PR_TRUE);
Index: extensions/xforms/nsXFormsRepeatElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRepeatElement.cpp,v
retrieving revision 1.22
diff -u -8 -p -r1.22 nsXFormsRepeatElement.cpp
--- extensions/xforms/nsXFormsRepeatElement.cpp 23 Aug 2005 11:00:50 -0000     
1.22
+++ extensions/xforms/nsXFormsRepeatElement.cpp 3 Sep 2005 21:30:36 -0000
@@ -448,17 +448,22 @@ nsXFormsRepeatElement::SetIndex(PRUint32
   if (mIsParent) {
     NS_ASSERTION(mCurrentRepeat, "How can we be a repeat parent without a
child?");
     // We're the parent of nested repeats, set through the correct repeat
     return mCurrentRepeat->SetIndex(aIndex, aIsRefresh);
   }

   // Do nothing if we are not showing anything
   if (mMaxIndex == 0)
+  {
+    // 9.3.6 states that he index position becomes 0 if there are
+    // no elements in the repeat.
+    mCurrentIndex = 0;
     return NS_OK;
+  }

   if (aIsRefresh && !mCurrentIndex) {
     // If we are refreshing, get existing index value from parent
     NS_ASSERTION(mParent, "SetIndex with aIsRefresh == PR_TRUE for a
non-nested repeat?!");
     rv = mParent->GetIndex(aIndex);
     NS_ENSURE_SUCCESS(rv, rv);
   }

@@ -486,17 +491,19 @@ nsXFormsRepeatElement::SetIndex(PRUint32
   // Set the repeat-index
   rv = SetChildIndex(*aIndex, PR_TRUE, aIsRefresh);
   NS_ENSURE_SUCCESS(rv, rv);

   // Unset previous repeat-index
   if (mCurrentIndex) {
     // We had the previous selection, unset directly
     SetChildIndex(mCurrentIndex, PR_FALSE, aIsRefresh);
-  } if (mParent) {
+  }
+  
+  if (mParent) {
     // Selection is in another repeat, inform parent (it will inform the
     // previous owner of its new state)
     rv = mParent->SetCurrentRepeat(this, *aIndex);
     NS_ENSURE_SUCCESS(rv, rv);
   }

   // Set current index to new value
   mCurrentIndex = *aIndex;
@@ -717,18 +724,18 @@ nsXFormsRepeatElement::Refresh()
   nsCOMPtr<nsIDOMDocument> domDoc;
   rv = mHTMLElement->GetOwnerDocument(getter_AddRefs(domDoc));
   NS_ENSURE_SUCCESS(rv, rv);

   mMaxIndex = contextSize;
   for (PRUint32 i = 1; i < mMaxIndex + 1; ++i) {
     // Create <contextcontainer>
     nsCOMPtr<nsIDOMElement> riElement;
-    rv =
domDoc->CreateElementNS(NS_LITERAL_STRING("http://www.w3.org/2002/xforms"),
-				  NS_LITERAL_STRING("contextcontainer"),
+    rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+				 
NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
				  getter_AddRefs(riElement));
     NS_ENSURE_SUCCESS(rv, rv);

     // Set model as attribute
     if (!modelID.IsEmpty()) {
       riElement->SetAttribute(NS_LITERAL_STRING("model"), modelID);
     }	

Index: extensions/xforms/nsXFormsUtils.h
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
retrieving revision 1.30
diff -u -8 -p -r1.30 nsXFormsUtils.h
--- extensions/xforms/nsXFormsUtils.h	23 Aug 2005 11:00:50 -0000	1.30
+++ extensions/xforms/nsXFormsUtils.h	3 Sep 2005 21:30:50 -0000
@@ -56,16 +56,18 @@ class nsString;
 class nsIMutableArray;
 class nsIDOMEvent;

 #define NS_NAMESPACE_XFORMS		  "http://www.w3.org/2002/xforms"
 #define NS_NAMESPACE_XHTML		  "http://www.w3.org/1999/xhtml"
 #define NS_NAMESPACE_XML_SCHEMA	  "http://www.w3.org/2001/XMLSchema"
 #define NS_NAMESPACE_XML_SCHEMA_INSTANCE
"http://www.w3.org/2001/XMLSchema-instance"
 #define NS_NAMESPACE_MOZ_XFORMS_TYPE	 
"http://www.mozilla.org/projects/xforms/2005/type"
+#define NS_REPEAT_CONTEXTCONTAINER	  "contextcontainer"
+#define NS_REPEAT			  "repeat"

 /**
  * XForms event types
  *
  * @see http://www.w3.org/TR/xforms/slice4.html#rpm-events
  *
  * @note nsXFormsModelElement::SetSingleState() assumes a specific order of
  * the events from eEvent_Valid to eEvent_Disabled.

Comment 13

[Peter Nunn]

Attachment: Converted file to be unix line endings and no tabs.

Sorry to all on the mailing list for repeated uploads.	Have got it right now
:-)

Comment 14

[Allan Beaufour]

So which one of all these is the correct patch? Could you please set the review
flag on the correct one?

Comment 15

[Allan Beaufour]

Comment on attachment 194787 [details] [diff] [review]
Converted file to be unix line endings and no tabs.

Please fix issues in:
http://lab.cph.novell.com/~abeaufour/jst-review/?patch=194787

> Index: extensions/xforms/.cvsignore
> ===================================================================
> +xforms.sln
> +xforms.vcproj
> +xforms.ncb
> +xforms.suo

I think you'll have to ignore that yourself.

> Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
> retrieving revision 1.3
> diff -u -8 -p -r1.3 nsXFormsInsertDeleteElement.cpp
> --- extensions/xforms/nsXFormsInsertDeleteElement.cpp	2 Aug 2005 14:56:42 -0000	1.3
> +++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	3 Sep 2005 21:30:23 -0000
> @@ -34,23 +34,26 @@
>   *
>   * ***** END LICENSE BLOCK ***** */

>  #include "nsIDOMEvent.h"
>  #include "nsIDOMNode.h"
>  #include "nsIDOMElement.h"
>  #include "nsIDOMDocument.h"
>  #include "nsIDOMNodeList.h"
> +#include "nsIXFormsRepeatElement.h"
> +#include "nsIXformsControl.h"

nsIXFormsControl.h, proper file systems are case-sensitive you know :)

> +  // get all of the repeats.
> +  nsCOMPtr<nsIDOMNodeList> repeatNodes;
> +  document->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +    NS_LITERAL_STRING(NS_REPEAT), getter_AddRefs(repeatNodes));

NS_REPEAT is overdoing it.

> +  for(PRUint32 node = 0; node < nodeCount; node++)
> +  {
> +    nsCOMPtr<nsIDOMNode> repeatNode;
> +
> +    rv = repeatNodes->Item(node, getter_AddRefs(repeatNode));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // try cast
> +    nsCOMPtr<nsIXFormsRepeatElement> repeatElement(do_QueryInterface(repeatNode));
> +    NS_ENSURE_STATE(repeatElement);
> +
> +    // each row is bound to the repeat as an anonymous child of the repeat.
> +    // to find the context elements we need to get the html element
> +    // then find the xforms:contextcontainer element which holds the child
> +    // elements, but most importantly the context of the node that
> +    // is bound.  If that context is the node being removed then we
> +    // adjust the index.  When the node is refreshed, the index will then point
> +    // to the new node.
> +
> +    if(mIsInsert)
> +    {

curly braces go on same line as the 'if', same goes for switch, case, etc.
Please change that in the entire patch.

> +      // AN issue here.  If we are inserting into the node, the nodeset will

AN? bad CaSiNg? :)

> +      // not have the new node in the list of inserted elements as the XPath expression has
> +      // not been evaluated.  We call the refresh on the repeat so we can
> +      // evaluate the node binding after the xpath is reset.  This is not really nice
> +      // as it would be better if this is deferred till a model refresh.

Add an XXX before the comment when it is something that should be fixed.

For an insert in an action block this is wrong. You should fix it so that you
only rebind the repeat, cache the nodeset, and test for the node in that.


> +      nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(repeatElement));
> +      NS_ENSURE_STATE(control);
> +
> +      rv = control->Refresh();
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

You refresh _all_ repeats on an insert? This is bad performance-wise.

> +
> +    nsCOMPtr<nsIXTFVisual> visualElement(do_QueryInterface(repeatNode));
> +    NS_ENSURE_STATE(repeatElement);
> +
> +    nsCOMPtr<nsIDOMElement> visualContext;
> +    rv = visualElement->GetVisualContent(getter_AddRefs(visualContext));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // all of the child nodes of the type
> +    nsCOMPtr<nsIDOMNodeList> childNodes;
> +
> +    rv = visualContext->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                               NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
> +                                               getter_AddRefs(childNodes));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    PRUint32 childCount;
> +    rv = childNodes->GetLength(&childCount);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIDOMNode> childNode;
> +    for(PRUint32 childIndex = 0; childIndex < childCount; childIndex++)
> +    {
> +      rv = childNodes->Item(childIndex, getter_AddRefs(childNode));
> +
> +      nsCOMPtr<nsIXFormsContextControl> riContext(do_QueryInterface(childNode));
> +      NS_ENSURE_STATE(riContext);
> +
> +      // see what the context of the node is.  If it is our deleted node then
> +      // we adjust the index so that when the control refreshes,
> +      // we see the node prior or following being selected.
> +      nsAutoString modelID;
> +      PRInt32 position, size;
> +      nsCOMPtr<nsIDOMNode> boundNode;
> +
> +      rv = riContext->GetContext(modelID, getter_AddRefs(boundNode), &position, &size);
> +
> +      // if the node is the same node then we have found the deleted node on this repeat.
> +      nsCOMPtr<nsIDOM3Node> changedNode(do_QueryInterface(resNode));
> +      NS_ENSURE_STATE(changedNode);
> +
> +      PRBool isSameNode;
> +      changedNode->IsSameNode(boundNode, &isSameNode);
> +      if(isSameNode)
> +      {
> +        PRUint32 indexValue = childIndex + (mIsInsert ? 1 : 0);
> +
> +        // we ask the node t refresh at this point so that the
> +        // inner indices are reset to 1.
> +        rv = repeatElement->SetIndex(&indexValue, PR_FALSE);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        break;        // all done on this repeat
> +      }
> +    }
> +  }
> +

How about nested repeats? I'm not sure that this works correctly for them...
I'm not actually sure whether the exact behvaviour is clearly defined :(

Doing this through the DOM is expensive. I opt for the above approach of
caching the nodeset from the XPath result.


>    rv = nsXFormsUtils::DispatchEvent(instNode,
> -                                    mIsInsert ? eEvent_Insert : eEvent_Delete);
> +    mIsInsert ? eEvent_Insert : eEvent_Delete);

Don't change the indent.

> Index: extensions/xforms/nsXFormsRepeatElement.cpp
> ===================================================================
>    // Do nothing if we are not showing anything
>    if (mMaxIndex == 0)
> +  {
> +    // 9.3.6 states that he index position becomes 0 if there are
> +    // no elements in the repeat.
> +    mCurrentIndex = 0;
>      return NS_OK;
> +  }

Yes, good catch. Hmmm, but are controls using indexes actually updated? I do
not think so.

>    mMaxIndex = contextSize;
>    for (PRUint32 i = 1; i < mMaxIndex + 1; ++i) {
>      // Create <contextcontainer>
>      nsCOMPtr<nsIDOMElement> riElement;
> -    rv = domDoc->CreateElementNS(NS_LITERAL_STRING("http://www.w3.org/2002/xforms"),
> -                                 NS_LITERAL_STRING("contextcontainer"),
> +    rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                 NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),

Good fix to use the namespace define, but NS_REPEAT_CONTEXTCONTAINER is
overdoing it.

Comment 16

[Matthijs Wensveen]

Don't know if this is the right place to discuss this (if not ignore me), but shouldn't the repeat-index element be selected by a pseudo-element selector (r_l1::repeat-index and r_l2::repeat-index in testcase 3)? See XForms spec appendix F.
Or is this a matter of CSS(3) support for Mozilla/Firefox in general?

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> Don't know if this is the right place to discuss this (if not ignore me), but
> shouldn't the repeat-index element be selected by a pseudo-element selector
> (r_l1::repeat-index and r_l2::repeat-index in testcase 3)? See XForms spec
> appendix F.
> Or is this a matter of CSS(3) support for Mozilla/Firefox in general?

It's a matter of general CSS support for Mozilla/Firefox. We do not have support for the pseudoelements yet. Bug 271724 is tracking that. We have temporarily implemented the functionality through classes, in bug 312980.

Comment 18

[Allan Beaufour]

Attachment: Up-to-date patch

Attachment 194787 [details] [diff] brought up-to-date, and fixed nits and stuff -- my own review comments basically :)

Comment 19

[Allan Beaufour]

Attachment: Patch

Here's a go. As opposed to Peter's approach I moved the handling responsibility of handling to the repeat element itself.

There are still optimizations and fixes that need to be done, but this works for simple use-cases, and should have been fixed ages ago (by me ....). So let's get this in, and hunt for nested repeat bugs later.

Comment 20

[Doron Rosenberg (IBM)]

Comment on attachment 215905 [details] [diff] [review]
Patch

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsInsertDeleteElement.cpp xforms.282828/nsXFormsInsertDeleteElement.cpp
>--- xforms/nsXFormsInsertDeleteElement.cpp	2006-02-23 12:19:46.000000000 +0100
>+++ xforms.282828/nsXFormsInsertDeleteElement.cpp	2006-03-22 16:26:22.000000000 +0100
>   
>   // Do nothing if we are not showing anything
>-  if (mMaxIndex == 0)
>+  if (mMaxIndex == 0) {
>+    // 9.3.6 states that he index position becomes 0 if there are
>+    // no elements in the repeat.

s/he/the

>+  SanitizeIndex(aIndex, PR_TRUE);

couldn't aIndex be an uninitialzed variable? Or do we make sure that never happens in the callers?

> 
>+NS_IMETHODIMP
>+nsXFormsRepeatElement::HandleNodeInsert(nsIDOMNode *aNode)
>+{
>+  NS_ENSURE_STATE(mHTMLElement);
>+
>+  nsCOMPtr<nsIDOM3Node> node(do_QueryInterface(aNode));
>+  NS_ENSURE_STATE(node);
>+
>+  nsresult rv;
>+  // XXX, badness^2: If it is a insert we have to refresh before we can
>+  // figure out whether the node is in our nodeset... refactor this so
>+  // repeat actually gets the nodeset in Bind() and then uses it refresh,
>+  // then we can "just" re-evaluate the nodeset, and only refresh if the
>+  // node actually hits this repeat
>+
>+  // XXX, moreover it is also wrong to refresh at this point. It will happen
>+  // in insert processing (and possibly deferred...)
>+  rv = Refresh();
>+  NS_ENSURE_SUCCESS(rv, rv);

why not nsresult rv = Refresh() rather than in 2 lines?

>+  nsCOMPtr<nsIDOMNode> child;
>+  mHTMLElement->GetFirstChild(getter_AddRefs(child));
>+
>+  PRUint32 index = 1;
>+  while (child) {
>+    nsCOMPtr<nsIXFormsContextControl> context(do_QueryInterface(child));
>+    NS_ASSERTION(context,
>+                 "repeat child not implementing nsIXFormsContextControl?!");
>+

Perhaps do a nodeType check if its a ELEMENT_NODE before doing the QI?  Might be faster?

>+    nsAutoString modelID;
>+    PRInt32 position, size;
>+    nsCOMPtr<nsIDOMNode> boundNode;
>+    rv = context->GetContext(modelID, getter_AddRefs(boundNode), &position,
>+                             &size);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    PRBool sameNode = PR_FALSE;
>+    node->IsSameNode(boundNode, &sameNode);
>+    if (sameNode) {
>+      rv = SetIndex(&index, PR_FALSE);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      break;
>+    }

nit: newline after NS_ENSURE_SUCCESS(rv, rv); makes it more readable.

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> (From update of attachment 215905 [details] [diff] [review] [edit])
> >+  SanitizeIndex(aIndex, PR_TRUE);
> 
> couldn't aIndex be an uninitialzed variable? Or do we make sure that never
> happens in the callers?

There's an ENSURE_ARG in the start of this function, SanitizeIndex() also starts by checking it.
 
> >+  PRUint32 index = 1;
> >+  while (child) {
> >+    nsCOMPtr<nsIXFormsContextControl> context(do_QueryInterface(child));
> >+    NS_ASSERTION(context,
> >+                 "repeat child not implementing nsIXFormsContextControl?!");
> >+
> 
> Perhaps do a nodeType check if its a ELEMENT_NODE before doing the QI?  Might
> be faster?

Well, it should never happen. Repeat children always implement nsIXFormsContextControl -- we manage them ourselves. So this will just catch (major) design changes to repeat during testing.

Comment 22

[Allan Beaufour]

Attachment: Patch w/Doron's comments fixed

Comment 23

[Steve Speicher]

1.0 test suite: 9.3.6.a

Comment 24

[Allan Beaufour]

Checked into trunk

Comment 25

[Steve Speicher]

I haven't dug through the code, but test case 9.3.6.a shows a index() == 1 when all repeat items have been removed (when it should be == 0).

Comment 26

[Steve Speicher]

likewise for test suite case: 7.8.5.a...should return have: index('norepeat') == NaN, instead get 0

Comment 27

[aaronr]

(In reply to comment #26)
> likewise for test suite case: 7.8.5.a...should return have: index('norepeat')
> == NaN, instead get 0
> 

That is a change in the second edition of the spec.  Used to throw a compute exception.

Comment 28

[aaronr]

(In reply to comment #25)
> I haven't dug through the code, but test case 9.3.6.a shows a index() == 1 when
> all repeat items have been removed (when it should be == 0).
> 

Looks like repeat bails when there are no items to show, so never gets to the point in the code where it sets index.