Patch coming.

This XBLizes <repeat> in a bit similar way, which was done for <itemset>. I haven't tested this properly yet, especially nested repeats need testing. Allan, could you take a look at this patch, does it look somewhat right to you? I hope the patch applies properly.

Comment on attachment 194106 [details] [diff] [review] v1 bah, doesn't work properly with nested repeats.

This doesn't crash ;). And creates right kind of UI also when nested repeats are used. But still problems with repeat-index :( Allan, maybe you have some comments?

Comment on attachment 201284 [details] [diff] [review] a better patch And repeat must be optimized a bit :(

*** Bug 317518 has been marked as a duplicate of this bug. ***

(In reply to comment #1 bug 317518) > XBLizing repeat would solve that. See bug 306247 > Yes, I guess it should. But it's needed to remember about it. So the patch should let to specify desired layout.

(In reply to comment #6) > (In reply to comment #1 bug 317518) > > XBLizing repeat would solve that. See bug 306247 > > > > Yes, I guess it should. But it's needed to remember about it. So the patch > should let to specify desired layout. We could implement an appearance="vertical"/minimal, whatever. But that's a detail, as long as we have it defined in XBL the form author can define his own custom control for it.

(In reply to comment #3) > Created an attachment (id=201284) [edit] > a better patch > > This doesn't crash ;). And creates right kind of UI also when > nested repeats are used. But still problems with repeat-index :( > Allan, maybe you have some comments? I looked briefly on this. Except for the fact that you forgot to include the IDL file in the second patch :), then the problem is the nested repeat handling of indexes, right?

(In reply to comment #8) > (In reply to comment #3) > > Created an attachment (id=201284) [edit] > > a better patch > > > > This doesn't crash ;). And creates right kind of UI also when > > nested repeats are used. But still problems with repeat-index :( > > Allan, maybe you have some comments? > > I looked briefly on this. Except for the fact that you forgot to include the > IDL file in the second patch :), then the problem is the nested repeat > handling of indexes, right? That might actually be bug 331081...

It's rougth patch. I have a unclear feel that it's not very good (I'm about bind() method). I should get more expierence with repeat to do a more finest patch :).

(In reply to comment #10) > Created an attachment (id=218672) [edit] > rough patch > > It's rougth patch. I have a unclear feel that it's not very good (I'm about > bind() method). I should get more expierence with repeat to do a more finest > patch :). What's the difference between and the previous patches?

(In reply to comment #12) > (In reply to comment #10) > > Created an attachment (id=218672) [edit] > > rough patch > > > > It's rougth patch. I have a unclear feel that it's not very good (I'm about > > bind() method). I should get more expierence with repeat to do a more finest > > patch :). > > What's the difference between and the previous patches? > The difference is that there are no changes in xforms model part (in nsPostRefresh) and I changed nsRepeatElement::Bind() method. Now repeat element is deffered if model is not ready. But I doubt that it is proper way to change Bind() method. Therefore I called the patch as a rough. I have one more question why is repeat element inherited from nsXFormsDelegateStub class (not just from nsXFormsBindableControlStub)?

(In reply to comment #12) > > What's the difference between and the previous patches? > For sure I added into the patch repeat for xul context support.

(In reply to comment #13) > I have one more question why is repeat element inherited from > nsXFormsDelegateStub class (not just from nsXFormsBindableControlStub)? > That way this.delegate.widgetAttached() can be called in constructor.

Comment on attachment 218672 [details] [diff] [review] rough patch You need smaug's change to the nsPostRefresh() handling, or you will crash on nested repeats. >+ /** >+ * Returns either the anonymous content of the repeat or null; >+ */ >+ already_AddRefed<nsIDOMElement> AnonymousContent(); I think it would be better if it was called GetAnonymousContent(). 1) It actually tells what the function does, and 2) it is a getter so it should be called that :) >+already_AddRefed<nsIDOMElement> >+nsXFormsRepeatElement::AnonymousContent() >+{ >+ nsIDOMElement* anon = nsnull; >+ nsCOMPtr<nsIXFormsRepeatUIElement> uiElement(do_QueryInterface(mElement)); >+ if (uiElement) { >+ // AddRefs >+ uiElement->GetAnonymousRepeatContent(&anon); Please add "// addrefs" to this line.

I ran quickly thought tests from http://beaufour.dk/xftst/Repeat and I couldn't find problems that aren't presented without patch.

*** Bug 334789 has been marked as a duplicate of this bug. ***

Comment on attachment 219567 [details] [diff] [review] patch2 > --- mozilla.orig/extensions/xforms/nsIXFormsRepeatUIElement.idl 1970-01-01 08:00:00.000000000 +0800 > @@ -0,0 +1,53 @@ > + * The Initial Developer of the Original Code is > + * Olli Pettay. > + * Portions created by the Initial Developer are Copyright (C) 2005 2006 > + /** > + * anonymousRepeatContent is used as a parent element for > + * the generated \<contextcontainer\> elements. Generated content will be inserted as children of this element. > --- mozilla.orig/extensions/xforms/nsXFormsRepeatElement.cpp 2006-03-30 14:45:57.000000000 +0900 > @@ -821,13 +812,36 @@ nsXFormsRepeatElement::Refresh() > // refresh (either using delete or through script, so check the index > // value > SanitizeIndex(&mCurrentIndex); > - } else if (!mParent && mMaxIndex) { > + } else if (mMaxIndex) { > // repeat-index has not been initialized, set it. > - GetStartingIndex(&mCurrentIndex); > + if (!mParent) { > + GetStartingIndex(&mCurrentIndex); > + } else { This is only needed for nested repeats, so: > + } else if (mLevel > 1) { should avoid all this if it is not necessary. You also need a comment explaining what you do here. > + nsCOMPtr<nsIXFormsRepeatItemElement> context = nsnull; > + nsCOMPtr<nsIDOMNode> parent = nsnull; You do not need to initialize nsCOMPtrs. > + nsCOMPtr<nsIDOMNode> temp = do_QueryInterface(mElement); > + NS_ENSURE_STATE(temp); > + > + while (!context) { > + temp.swap(parent); I would rather have parent initialized to mElement, and then temp.swap(parent) at the end of the loop. Easier to read. > + rv = parent->GetParentNode(getter_AddRefs(temp)); This can set temp to null, but still return NS_OK. So you need to check that temp has a value, or you might end up deref'ing a null. > + NS_ENSURE_SUCCESS(rv, rv); > + context = do_QueryInterface(parent); Shouldn't you be QI'ing temp, and not parent? > + } > + > + PRBool hasIndex = PR_FALSE; > + context->GetIndexState(&hasIndex); > + if (hasIndex) { > + PRUint32 index = 0; > + GetStartingIndex(&index); > + SetIndex(&index, PR_FALSE); > + } > + return NS_OK; > + } > } I'm wondering why this is suddenly necessary though. But repeat is a big mess, and I been meaning to refactor it for a long time. But this bug should not be delayed, so doing this better can wait. > --- mozilla.orig/extensions/xforms/resources/content/xforms-xul.xml 2006-04-04 17:11:26.000000000 +0900 > + > + <!-- REPEAT --> > + <binding id="xformswidget-repeat" > + extends="chrome://xforms/content/xforms.xml#xformswidget-repeat-base"> > + <content> > + <xul:box hidden="true"> > + <children/> > + </xul:box> > + <xul:vbox flex="1" xbl:inherits="orient" anonid="insertion"/> I do not like exposing non-standard attributes on XForms elements. Is there a CSS way of controling the orient? If there is, then that should be used, or we should have two appearance implementations.

(In reply to comment #20) > > I do not like exposing non-standard attributes on XForms elements. Is there a > CSS way of controling the orient? If there is, then that should be used, or we > should have two appearance implementations. > -moz-box-orient

I removed the supporting of repeat aligning. I prefer to reopen the existing bug 317518.

Comment on attachment 219585 [details] [diff] [review] patch3 >+ } else if (mLevel > 1) { >+ // Set repeat-index for inner repeats. If parent <contextcontainer/> >+ // element is selected then mCurrentIndex is setted on starting index. >+ >+ nsCOMPtr<nsIDOMNode> temp; >+ nsCOMPtr<nsIDOMNode> parent = do_QueryInterface(mElement); >+ NS_ENSURE_STATE(parent); >+ nsCOMPtr<nsIXFormsRepeatItemElement> context; >+ >+ while (!context) { >+ temp->GetParentNode(getter_AddRefs(parent)); >+ context = do_QueryInterface(parent); >+ temp.swap(parent); >+ } This is sloppy. What will temp always be in the first call? Initialize temp instead of parent, do not ignore the return value, check the temp value in the loop condition, and assert that context is set on loop exit.

> This is sloppy. What will temp always be in the first call? > > Initialize temp instead of parent, do not ignore the return value, check the > temp value in the loop condition, and assert that context is set on loop exit. > I'm sorry for thoughtlessness.

Comment on attachment 219707 [details] [diff] [review] patch4 >+ nsCOMPtr<nsIDOMNode> temp = do_QueryInterface(mElement); >+ NS_ENSURE_STATE(temp); nsIDOMElement is (C++) inherited from nsIDOMNode, so you can assign directly. r=me with that fix. But let's run it through smaug first.

Comment on attachment 219707 [details] [diff] [review] patch4 Looks good to me

Fixed on trunk