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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aaronr, Assigned: aaronr)
Details
Attachments
(2 files, 4 obsolete files)
1.43 KB,
application/xhtml+xml
|
Details | |
14.46 KB,
patch
|
Details | Diff | Splinter Review |
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
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 5•19 years ago
|
||
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 6•19 years ago
|
||
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.
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)
Updated•19 years ago
|
Attachment #174718 -
Flags: review?(doronr) → 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)
Assignee | ||
Comment 10•19 years ago
|
||
Attachment #174726 -
Attachment is obsolete: true
Attachment #174727 -
Flags: review?(allan)
Comment 11•19 years ago
|
||
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+
Assignee | ||
Comment 12•19 years ago
|
||
final patch with Allan's nits.
Attachment #174727 -
Attachment is obsolete: true
Comment 13•19 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 14•19 years ago
|
||
(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.
Assignee | ||
Comment 15•19 years ago
|
||
I was going to make sure that it got checked in before I opened the bug :=) But thanks for opening the bug.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•