User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1 If the submission method is post, the form isn't sent until all required fields are filled. If the submission method is get or urlencoded-post, the form is sent even if a required field is empty. Reproducible: Always Steps to Reproduce:

Unless I'm sleeping or something this is quite amazing :) Because it seems to be correct. Amazing amazing. NB. It does also work for put and multipart-post. Not that it makes it so much better though...

sounds like a good starter bug. Assigning to Steve.

Oddly post,multipart,put work and get,url-post,form-data don't

After some debugging, here's the issue: Since post,multipart,put cause nsXFormsSubmissionElement::SerializeDataXML() to be invoked, which indirectly invokes nsXFormsModelElement::HandleInstanceDataNode() (which actually does the validity, relevant and requiredness checking). HandleInstanceDataNode() is not invoked with the other submission method types. I'll investigate a way to iterate over the instance nodes for the other method types.

Notes to reviewers: - Should the arg to CanSubmit() be const? - Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal approach? - Should the test be moved to SerializeData* methods? I liked it in in Submit() as it prevents further processing and can be used for other tests (like cross domain?)

Comment on attachment 214330 [details] [diff] [review] proposed patch to nsXFormsSubmitElement.* Your comments are too long, and maybe use a slightly short variable name. Check: http://beaufour.dk/jst-review/?patch=214330 (In reply to comment #6) > Notes to reviewers: > - Should the arg to CanSubmit() be const? People (incl. me) are very bad at using const where it should be used. It would be nice if it was, yes. > - Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal > approach? It's ok to keep the style of the code you are changing. I'm not sure we are heading the right path though... more about that. > - Should the test be moved to SerializeData* methods? I liked it in in Submit() > as it prevents further processing and can be used for other tests (like cross > domain?) It should be moved. You end up doing the same work twice for XML submissions. >+/* >+ * This walks the DOM tree, starting at topNode and invokes model->HandleInstanceDataNode() >+ * that ensures that the instance is valid, required conditions met...if so, then can submit. >+ */ Good style with a comment! But, please move it to the header file, and use "/**", and comment the argument, just like EndSubmit() is. >+nsresult >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *topNode) Arguments should start with an "a", so: nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) >+{ >+ nsCOMPtr<nsIDOMNode> currentNode(topNode), node; >+ nsCOMPtr<nsIModelElementPrivate> model = GetModel(); You need to make sure that you actually got a model here. It could be null NS_ENSURE_STATE(model); >+ >+ while (currentNode) >+ { (I can see that this file has it's own "style", compared to the rest of XForms code.... hmmm, but:) please move the "{" up on the prev. line. >+ PRUint16 handleNodeResult; >+ model->HandleInstanceDataNode(currentNode, &handleNodeResult); >+ >+ /* >+ * SUBMIT_SERIALIZE_NODE - node is to be serialized >+ * SUBMIT_SKIP_NODE - node is not to be serialized >+ * SUBMIT_ABORT_SUBMISSION - abort submission (invalid node or empty required node) >+ */ >+ if (handleNodeResult == nsIModelElementPrivate::SUBMIT_SKIP_NODE) { >+ // skip node and subtree >+ currentNode->GetNextSibling(getter_AddRefs(node)); >+ currentNode.swap(node); >+ continue; >+ } else if (handleNodeResult == nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) { >+ // abort >+ return NS_ERROR_ILLEGAL_VALUE; Since you are aborting, please use NS_ERROR_ABORT. >+ } If you want to be really nice :), you can do a: NS_ASSERTION(handleNodeResult == SUBMIT_SERIALIZE_NODE) here, just in case. >+ // recurse >+ nsCOMPtr<nsIDOMNode> startNode; I know this is copy'n'paste, but please call it "firstChild", since that is what it is. >Index: nsXFormsSubmissionElement.h >=================================================================== >+ nsresult CanSubmit(nsIDOMNode *source); Here you call the argument something else than in the cpp file. Something is wrong in the submission handling. I think we need a common "CreateInstancePurgedForNonRelevantNodes" function, that is called for all serialization possibilities and that will also check for required nodes. We need that to do a full schema-check before submission. BUT, that is a different bug.

Can't find a good solution to const args (at least in this case), as I get into all sorts of nsCOMPtr contructor type conflicts (they want non-const *). So, not fixing that. Regarding the comment: we need a common "CreateInstancePurgedForNonRelevantNodes" function I assume that wasn't directed at this patch but towards the forward referenced full schema check bug.

Comment on attachment 214460 [details] [diff] [review] patch take 2 >Index: nsXFormsSubmissionElement.cpp >=================================================================== >- // XXX call nsISchemaValidator::validate on each node >- >- >+ // XXX This is handled by the SerializeData() method >+ No need for XXX here, if there's no bug. >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) >+ } else if (handleNodeResult == >+ nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) { >+ // abort >+ return NS_ERROR_ILLEGAL_VALUE; Ahem. I remember saying something about this ;-) >Index: nsXFormsSubmissionElement.h >=================================================================== > /** >+ * This walks the DOM tree, starting at aTopNode and invokes >+ * model->HandleInstanceDataNode()that ensures that the instance is valid, >+ * required conditions met...if so, then can submit. >+ * >+ * @param aTopNode Root instance node of tree to be evaluated >+ * @return NS_ERROR_ABORT if instance is not valid and empty required nodes, >+ * otherwise return NS_OK. >+ * >+ */ >+ nsresult CanSubmit(nsIDOMNode *aTopNode); Please indent the comment properly, that is, something like this: >+ * @param aTopNode Root instance node of tree to be evaluated >+ * @return NS_ERROR_ABORT if instance is not valid and empty required >+ * nodes, otherwise return NS_OK. With the above changes, r=me

Comment on attachment 214896 [details] [diff] [review] updated with Allan's review comments Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2 spaces only.

(In reply to comment #11) > (From update of attachment 214896 [details] [diff] [review] [edit]) > Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode > *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2 > spaces only. Not only that, it also has Windows line-endings. Steve: Please convert file to Unix line-endings before submitting.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 214896 [details] [diff] [review] [edit] [edit]) > > Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode > > *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2 > > spaces only. > > Not only that, it also has Windows line-endings. Steve: Please convert file to > Unix line-endings before submitting. I also changes the return value in CopyChildren() and not in CanSubmit()...

Comment on attachment 214896 [details] [diff] [review] updated with Allan's review comments Please fix: * return value in correct function * indentation * unix line endings

Comment on attachment 215005 [details] [diff] [review] fixed: return value, indents, eol *sigh* still, Windows line-endings... Whoever checks this in, be SURE to change this.

(In reply to comment #16) > (From update of attachment 215005 [details] [diff] [review] [edit]) > *sigh* still, Windows line-endings... > > Whoever checks this in, be SURE to change this. jst-review v2 now warns about this too...

fixed line endings. Checked into trunk for sspeiche.

Comment on attachment 215005 [details] [diff] [review] fixed: return value, indents, eol >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) >+{ >+ nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node; >+ nsCOMPtr<nsIModelElementPrivate> model = GetModel(); >+ NS_ENSURE_STATE(model); Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on each call. Very expensive. We need to change this. I'm suspecting something else is wrong too...

(In reply to comment #19) > (From update of attachment 215005 [details] [diff] [review] [edit]) > >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) > >+{ > >+ nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node; > >+ nsCOMPtr<nsIModelElementPrivate> model = GetModel(); > >+ NS_ENSURE_STATE(model); > > Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on > each call. Very expensive. We need to change this. CopyChildren does the same... :( > I'm suspecting something else is wrong too... Still trying.

(In reply to comment #19) > (From update of attachment 215005 [details] [diff] [review] [edit]) > >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) > >+{ > >+ nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node; > >+ nsCOMPtr<nsIModelElementPrivate> model = GetModel(); > >+ NS_ENSURE_STATE(model); > > Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on > each call. Very expensive. We need to change this. > > I'm suspecting something else is wrong too... > Not THAT expensive. Gets the parent node and QI's it. But you are right. No reason we can't pass model through as a parameter since it is recursive.

(In reply to comment #21) > Not THAT expensive. Gets the parent node and QI's it. But you are right. No > reason we can't pass model through as a parameter since it is recursive. Well, it's unnecessary and being done for every tree level in an instance document... that's expensive in my world.