279449 - Submission not stopped for invalid/required data

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

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.

Comment 1

[aaronr]

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

Comment 2

[Doron Rosenberg (IBM)]

Attachment: first attempt

Comment 3

[Doron Rosenberg (IBM)]

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

Comment 4

[Doron Rosenberg (IBM)]

Attachment: attempt #2

Comment 5

[aaronr]

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 6

[Doron Rosenberg (IBM)]

Attachment: brain fart regarding returns. Also add documentation.

Comment 7

[aaronr]

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

Comment 8

[Doron Rosenberg (IBM)]

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

Comment 9

[aaronr]

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

Comment 10

[Allan Beaufour]

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 :)

Comment 11

[Doron Rosenberg (IBM)]

(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.

Comment 12

[Allan Beaufour]

(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.

Comment 13

[Doron Rosenberg (IBM)]

checked in