Closed Bug 273428 Opened 20 years ago Closed 19 years ago

xforms input element checkbox behavior w.r.t. instance data

Categories

(Core Graveyard :: XForms, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: aaronr)

Details

Attachments

(2 files, 4 obsolete files)

During code review I noticed that if we are handling an input field bound to a
node of boolean type, that during Refresh() we treat it as a checkbox and
determine whether the value is "true" or "1" and if either, we'll check it,
otherwise we won't.  Fine.  But during "::Blur", we will set the value to 1 or 0
based on whether it is checked or not.  So with nothing more than a focus change
happening, the instance value could go from "true" to "1".

This needs to be investigated and if necessary, fixed.  Especially what the
appropriate behavior should be.  For example, if I use a "true" instance data
node value to make the checkbox selected, if the checkbox is then manually
deselected,  should the instance value become "false", or is "0" ok?  What do
other processors (esp. the Novell one) do?
I'm looking at this, now.  Looks like Novell and formsPlayer will update
boolean-type instance data using 'true' and 'false' if the user selects or
unselects the 'checkbox' no matter if the instance data started with 0 or 1.  So
if the data is '1' to start with and the user de-selects the checkbox, then the
instance data will now contain 'false'.  However, if the user never touches the
bound control and then submits the form, 1 will be submitted.  If the user
starts with '1' and de-selects and then selects the checbox, then 'true' will be
the instance value.
Status: NEW → ASSIGNED
Attached file testcase
testcase works, but submit won't work, of course.
Attached patch first fix attempt (obsolete) — Splinter Review
probably will need a few changes after comments, but it works just dandy.
Attachment #174638 - Flags: review?(doronr)
main things I'd like critiques on are my schema changes, whether you think I
should move the function ::GetTypeForControl out to nsXFormsUtils.cpp, and
whether just looking for "click" in nsXFormsInputElement::HandleDefault is
enough or if I should also check if it is a checkbox.  I didn't think it was
that big of a deal if a click also caused an update in cases where input wasn't
a checkbox.  Click could end up playing a role with other types, like using date
type with at date picker control and could just end up with ugly tests.
Comment on attachment 174638 [details] [diff] [review]
first fix attempt

> NS_IMETHODIMP
> nsXFormsModelElement::GetTypeForControl(nsIXFormsControl  *aControl,
>                                         nsISchemaType    **aType)
> {
>+  NS_ENSURE_ARG_POINTER(aType);
>   *aType = nsnull;
>-  return NS_OK;
>+
>+  nsCOMPtr<nsIDOMNode> boundNode;
>+  nsresult rv;
Nit: define it the first time you need it.

>+
>+  aControl->GetBoundNode(getter_AddRefs(boundNode));
>+  nsCOMPtr<nsIDOMElement> nodeElem = do_QueryInterface(boundNode, &rv);
>+  NS_ENSURE_TRUE(nodeElem, NS_ERROR_FAILURE);
>+
>+  nsAutoString typeAttribute;
>+  nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE),
>+                           NS_LITERAL_STRING("type"), typeAttribute);
>+
>+  // split type (ns:type) into namespace and type.
>+  PRInt32 separator = typeAttribute.FindChar(':');
>+  if ((separator == kNotFound) || ((PRUint32) separator == typeAttribute.Length()))
>+    return NS_ERROR_UNEXPECTED;
>+  // xxx send error to console
>+
>+  nsAutoString schemaTypeName;
>+  nsAutoString schemaTypePrefix;
>+  nsAutoString schemaTypeNamespace;
>+
>+  schemaTypePrefix.Assign(Substring(typeAttribute, 0, separator));
>+  schemaTypeName.Assign(Substring(typeAttribute, ++separator, typeAttribute.Length()));
>+
>+  // get the namespace url from the prefix
>+  nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = domNode3->LookupNamespaceURI(schemaTypePrefix, schemaTypeNamespace);
>+  NS_ENSURE_SUCCESS(rv, rv);

This is code you stole from my existing code right?  Should be factored out
into a helper method in nsXFormsUtils or even just in nsXFormsSchemaValidator.

>+
>+  nsXFormsSchemaValidator validator;
>+  nsCOMPtr<nsISchema> schema = do_QueryInterface(mSchemas);
>+  validator.LoadSchema(schema);
>+  return validator.GetType(schemaTypeName, schemaTypeNamespace, aType);
> }
> 
> NS_IMETHODIMP
> nsXFormsModelElement::InstanceLoadStarted()
> {
>   ++mPendingInstanceCount;
>   return NS_OK;
> }
>Index: xforms/nsXFormsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSchemaValidator.cpp,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsSchemaValidator.cpp
>--- xforms/nsXFormsSchemaValidator.cpp	19 Jan 2005 16:05:31 -0000	1.3
>+++ xforms/nsXFormsSchemaValidator.cpp	18 Feb 2005 02:17:45 -0000
>@@ -72,8 +72,18 @@ PRBool nsXFormsSchemaValidator::Validate
>   PRBool isValid = PR_FALSE;
> 
>   NS_ENSURE_TRUE(mSchemaValidator, isValid);
>   mSchemaValidator->Validate(aElement, &isValid);
> 
>   return isValid;
> }
> 
>+PRBool nsXFormsSchemaValidator::GetType(const nsAString & aType,
>+  const nsAString & aNamespace, nsISchemaType **aSchemaType)
>+{
>+  PRBool succeeds = PR_FALSE;
>+
>+  NS_ENSURE_TRUE(mSchemaValidator, succeeds);
>+  mSchemaValidator->GetType(aType, aNamespace, aSchemaType);
>+
>+  return succeeds;
>+}

why bother with |succeeds|?  Just use PR_*.  Also, doesn't NS_ENSURE_TRUE need
an NS_* not a PR_*?
Also please check rv of mSchemaValidator->GetType.
Attachment #174638 - Flags: review?(doronr) → review-
Comment on attachment 174638 [details] [diff] [review]
first fix attempt

Just a quick review from me.

General issues

1) Remember to get rid of your Windows cr+lf!!

2) Regarding your comments about updating and blur. We should not update the
instance data if the instance data we retrieve is invalid/out-of-range. Feel
free to include a todo and create a new bug for it.

3) Code comments:

>+++ xforms/nsXFormsInputElement.cpp	18 Feb 2005 02:17:45 -0000
...
>+  // XXX is it a big deal if we also update on clicks, too?  Seems like too
>+  // big of a hassle for too little gain to also check if we are a checkbox 
>+  // in addition to checking for the click.

Hmmm, well using incremental you should know you are asking for trouble, so
fine with me (but change the comment to a statement then).

>@@ -340,16 +349,20 @@ nsXFormsInputElement::Refresh()
>+
>+        // other xforms processors default to incremental update in this case,
>+        // so we should, too.
>+        mIncremental = PR_TRUE;

Makes sense.

For now, I'll leave the schema stuff to you and Doron.
Attached patch 2nd attempt (obsolete) — Splinter Review
took in Allan's and Doron's suggestions.  Also included a one line fix to
nsXFormsInputElement::AttributeSet since I was already in the code.  It was
calling nsXFormsControlStub::WillSetAttribute instead of
nsXFormsControlStub::AttributeSet
Attachment #174638 - Attachment is obsolete: true
Attachment #174718 - Flags: review?(doronr)
Attachment #174718 - Flags: review?(doronr) → review+
Attached patch same with nit fix (obsolete) — Splinter Review
added nit fix for doron.  Allan, please be my second review?
Attachment #174718 - Attachment is obsolete: true
Attachment #174726 - Flags: review?(allan)
Comment on attachment 174726 [details] [diff] [review]
same with nit fix

forgot to dos2unix patch
Attachment #174726 - Flags: review?(allan)
Attached patch same after dos2unix run (obsolete) — Splinter Review
Attachment #174726 - Attachment is obsolete: true
Attachment #174727 - Flags: review?(allan)
Comment on attachment 174727 [details] [diff] [review]
same after dos2unix run

>Index: xforms/nsXFormsInputElement.cpp
>===================================================================
>@@ -277,17 +282,25 @@ nsXFormsInputElement::UpdateInstanceData
...
>+      // XXX we've got a problem here.  Since UpdateInstanceData can be called
>+      // due to a blur event, as it stands now we could be changing the instance
>+      // data values even if the user didn't click on the checkbox, but instead
>+      // was just tabbing through the form.

Remember to create a new bug.

>Index: xforms/nsXFormsUtils.cpp
>===================================================================
>+
>+/* This function takes an instance data node, finds the type bound to it, and
>+ * returns the seperated out type (integer) and namespace prefix (xsd).
>+ */

You have that in the .h-file, once is enough :)

>+/* static */ nsresult
>+nsXFormsUtils::ParseTypeFromNode(nsIDOMNode *aInstanceData, 
>+                                 nsAString & aType, nsAString & aNSPrefix)

Keep the &'s next to the name.

With the above nits, r=me.
Attachment #174727 - Flags: review?(allan) → review+
Attached patch final patchSplinter Review
final patch with Allan's nits.
Attachment #174727 - Attachment is obsolete: true
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> (From update of attachment 174727 [details] [diff] [review] [edit])
> >Index: xforms/nsXFormsInputElement.cpp
> >===================================================================
> >@@ -277,17 +282,25 @@ nsXFormsInputElement::UpdateInstanceData
> ...
> >+      // XXX we've got a problem here.  Since UpdateInstanceData can be called
> >+      // due to a blur event, as it stands now we could be changing the instance
> >+      // data values even if the user didn't click on the checkbox, but instead
> >+      // was just tabbing through the form.
> 
> Remember to create a new bug.

And you didn't, so now I've created bug 283278 for it.
I was going to make sure that it got checked in before I opened the bug :=)  But
thanks for opening the bug.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: