if a piece of instance data is bound to with a "required" MIP that evaluates to false, then we should NOT submit. Yet, we do. We also need a visual indicator that an element is required so that the user knows why the submit isn't happening. Perhaps a red astrisk appended at the end of the label? Also, according to file:///c:/xforms/spec/index-all.html#submit-event item #2, if any invalid items are encountered, then submit should also not proceed. Look in the URL field for a testcase that shows off the problem. And keep in mind if you try to run this on your own server, that the submission action must live on the same server as the original document or submit won't work in mozilla due to security.

Please also generate the xforms-submit-error as spelled out in the spec at 11.1 #2 while you are at it.

Basically adds nsXFormsModelElement::ValidateInstanceDataForSubmission, which validates the passed in instancedata if it can be submitted, which nsXFormsSubmissionElement::Submit() calls.

Comment on attachment 172752 [details] [diff] [review] attempt #2 Need more comments. Makes sense if you really read the spec first, but someone coming into the code for the first time won't understand what you are doing (in ValidateInstanceDataForSubmission) need to initialize *aResult ValidateInstanceDataForSubmission returns nsresult, but in 3 different places you return booleans. Logic seems good.

Comment on attachment 172839 [details] [diff] [review] brain fart regarding returns. Also add documentation. + // if not relevant, don't handle subtree + if (!isNodeRelevant) { + *aResult = PR_TRUE; + return NS_OK; + } nit: couple of places the closing bracket isn't properly aligned. + + /** + * Validates the instance node and its children if it can be submitted. + */ *to see* if it can be submitted, or something a little more accurate, please :=) with those changes, r=aaronr

aaron: do I need other review to check this in?

probably doesn't need a sr, but get beaufour to review it, too.

Comment on attachment 172839 [details] [diff] [review] brain fart regarding returns. Also add documentation. Disclaimer: I have yet to study submission. Even yet to try to actually submit anything :) >--- extensions/xforms/nsXFormsSubmissionElement.cpp 26 Jan 2005 12:20:21 -0000 1.19 >nsXFormsSubmissionElement::Submit() ... >+ // XXX seems to be required by >+ // http://www.w3.org/TR/2003/REC-xforms-20031014/slice4.html#evt-revalidate >+ // but is it really needed for us? Yes, indeed. >+ nsCOMPtr<nsIModelElementPrivate> model = GetModel(); do model->Recalculate() first. >+ model->Revalidate(); > // XXX call nsISchemaValidator::validate on each node That's exactly what you do next is it not? >+ PRBool isValid = CheckChildren(data); >+ if (!isValid) >+ return NS_ERROR_FAILURE; >+ Hmmm, shouldn't you dispatch xforms-submit-error pr.: http://www.w3.org/TR/xforms/slice11.html#submit-event (item 2) >+/* >+ Validates a instance data subtree for submission by recursively walking it. "*an* instance data", no? >+ It makes sure that all nodes are valid, and that all required nodes have an >+ XForms value. It skips non-relevant nodes and their children, since those >+ won't be submitted per the spec. >+ */ How about complex type checking? You should probably prune the instance of any non-relevant nodes before (complex) type checking, so you could start by doing that? >+nsXFormsModelElement::ValidateInstanceDataForSubmission(nsIDOMNode *aInstanceDataNode, PRBool *aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aResult); check aInstanceDataNode too. >+ ns = mMDG.GetNodeState(aInstanceDataNode); NS_ENSURE_STATE(ns); >+ while (!done && (run < length)) { A for-loop would be easier to understand. >+ if (!foundElementNode) { >+ nsAutoString value; >+ nsXFormsUtils::GetNodeValue(node, value); >+ >+ if (value.IsEmpty() && isRequired) >+ isValid = PR_FALSE; >+ else >+ isValid = PR_TRUE; >+ } I had to read that a couple of times, how about if (!foundElementNode && isRequired) { nsAutoString value; nsXFormsUtils::GetNodeValue(node, value); isValid = !value.IsEmpty(); } else { isValid = PR_TRUE; } Skips GetNodeValid() for non-required fields, and makes it a lot easier to understand IMHO. >+++ extensions/xforms/nsIModelElementPrivate.idl 30 Jan 2005 00:00:36 -0000 ... >+ PRBool ValidateInstanceDataForSubmission(in nsIDOMNode aInstanceDataNode); IDL: validateInstanceDataForSubmission With the above r=me. I wasn't actively asked to review this, but I took Aaron's comment as a hint :)

(In reply to comment #10) > (From update of attachment 172839 [details] [diff] [review] [edit]) > Disclaimer: I have yet to study submission. Even yet to try to actually submit > anything :) > > >--- extensions/xforms/nsXFormsSubmissionElement.cpp 26 Jan 2005 12:20:21 -0000 1.19 > >nsXFormsSubmissionElement::Submit() > ... > >+ // XXX seems to be required by > >+ // http://www.w3.org/TR/2003/REC-xforms-20031014/slice4.html#evt-revalidate > >+ // but is it really needed for us? > > Yes, indeed. > It doesn't seem to make a difference though. > > > // XXX call nsISchemaValidator::validate on each node > > That's exactly what you do next is it not? Actually no. All I do is check if the node is valid or not. We probably need to validate nodes again due to complex types, so I left this comment in. > > >+ PRBool isValid = CheckChildren(data); > >+ if (!isValid) > >+ return NS_ERROR_FAILURE; > >+ > > Hmmm, shouldn't you dispatch xforms-submit-error pr.: > http://www.w3.org/TR/xforms/slice11.html#submit-event (item 2) if Submit() returns a failure, the event is sent by existing code. > > How about complex type checking? You should probably prune the instance of any > non-relevant nodes before (complex) type checking, so you could start by doing > that? I opened a bug about noone pruning non-relevant nodes, I think that is beyond the scope of this bug.

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 172839 [details] [diff] [review] [edit] [edit]) > > >nsXFormsSubmissionElement::Submit() > > ... > > >+ // XXX seems to be required by > > >+ // http://www.w3.org/TR/2003/REC-xforms-20031014/slice4.html#evt-revalidate > > >+ // but is it really needed for us? > > > > Yes, indeed. > > > It doesn't seem to make a difference though. Could if somebody does some deferred actions or messes with the instance data behind our back. > > > // XXX call nsISchemaValidator::validate on each node > > > > That's exactly what you do next is it not? > > Actually no. All I do is check if the node is valid or not. We probably need > to validate nodes again due to complex types, so I left this comment in. Ah. Check. > > > > >+ PRBool isValid = CheckChildren(data); > > >+ if (!isValid) > > >+ return NS_ERROR_FAILURE; > > >+ > > > > Hmmm, shouldn't you dispatch xforms-submit-error pr.: > > http://www.w3.org/TR/xforms/slice11.html#submit-event (item 2) > if Submit() returns a failure, the event is sent by existing code. Where is that? It is not in the patch, and http://lxr.mozilla.org/seamonkey/search?string=eEvent_SubmitError only finds it in nsXFormsUtils.h only. > > How about complex type checking? You should probably prune the instance of any > > non-relevant nodes before (complex) type checking, so you could start by doing > > that? > > I opened a bug about noone pruning non-relevant nodes, I think that is beyond > the scope of this bug. Fine.

checked in