Add instance attribute to submission element

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: Allan Beaufour, Assigned: jhp (no longer active))

Tracking

({fixed1.8})

Trunk
fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
The current errata (E69a) adds an instance attribute to the submission element,
specifying which instance should be replaced on replace="instance"
(Reporter)

Comment 1

12 years ago
Created attachment 190850 [details]
data for testcase
(Reporter)

Comment 2

12 years ago
Created attachment 190851 [details]
Testcase
(Reporter)

Comment 3

12 years ago
(In reply to comment #2)
> Created an attachment (id=190851) [edit]
> Testcase

Hmmm, I get: ERR: XForms Error (13): Could not parse new instance data ... is
the mime type wrong for the data?

Comment 4

12 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=190851) [edit] [edit]
> > Testcase
> 
> Hmmm, I get: ERR: XForms Error (13): Could not parse new instance data ... is
> the mime type wrong for the data?

I only took a second to look at this, but it looks like what is coming back from
our submission is an HTML page, not the page that you are expecting.  Since it
is HTML, I assume that is why you get the error output to the JS console.  I
haven't personally tried to do what you are doing....submitting to a XML file 
instead of a cgi-bin perl script or servlet.  If you can't host one, maybe worth
asking Leigh if he has one you can bounce off on xformstest.org.  I'd point you
to one of mine, but they are all behind a firewall :(
(Reporter)

Comment 5

12 years ago
Created attachment 190927 [details]
Testcase v2

This uses an external document instead.
Attachment #190850 - Attachment is obsolete: true
Attachment #190851 - Attachment is obsolete: true
(Assignee)

Comment 6

12 years ago
Created attachment 191272 [details] [diff] [review]
patch

I wasn't too sure about the best way to handle the logic in
|GetSelectedInstanceData()|.  I needed a binding attr that would always be
ignored, such that it would always use the default expression; so I made it
pass an empty string as the binding attr.
Attachment #191272 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Assignee: allan → jhpedemonte

Comment 7

12 years ago
Your GetSelectedInstanceData changes seems to be kinda hacky.  Did you give any
thought to just doing the xpath evaluations yourself?  Using EvaluateNodeBinding
was already overkill since submissions shouldn't need all the find context and
figuring out the model type of logic, for example.
(Assignee)

Comment 8

12 years ago
Created attachment 191599 [details] [diff] [review]
patch v2

Previous patch was completely wrong.  By going through |EvaluateNodeBinding()|,
this allowed a "bind" attr to override the "instance" attr.  So I did what
Aaron suggested, and call |EvaluateXPath()| myself.

I changed |GetSelectedInstanceData()| to |GetBoundInstanceData()|, since that
seems to better describe what the function does;  I then took the
|GetSelectedInstanceData()| name for my function.
Attachment #191272 - Attachment is obsolete: true
Attachment #191599 - Flags: review?(aaronr)

Comment 9

12 years ago
(In reply to comment #8)
> Created an attachment (id=191599) [edit]
> patch v2
> 
> Previous patch was completely wrong.  By going through |EvaluateNodeBinding()|,
> this allowed a "bind" attr to override the "instance" attr.  So I did what
> Aaron suggested, and call |EvaluateXPath()| myself.
> 
> I changed |GetSelectedInstanceData()| to |GetBoundInstanceData()|, since that
> seems to better describe what the function does;  I then took the
> |GetSelectedInstanceData()| name for my function.

I am soooo sorry.  I just now realized that @instance on submission replaces the
WHOLE instance document.  I didn't catch that the first time I read those
sections in the errata.  I thought that it specified which instance document to
use with the @ref (though looking back on that, it is such a stupid mistake on
my part because you can already do that).  I think that
nsXFormsUtilityService::GetInstanceDocumentRoot will give you everything that
you need without you having to dip into XPath.  See, that is what you get when
you ask me questions after I just get up (at noon :=)  Sorry again.
(Assignee)

Updated

12 years ago
Attachment #191272 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #191599 - Flags: review?(aaronr)
(Assignee)

Comment 10

12 years ago
Created attachment 191725 [details] [diff] [review]
patch v3

Patch that uses |nsXFormsUtilityService::GetInstanceDocumentRoot()|.
Attachment #191599 - Attachment is obsolete: true
Attachment #191725 - Flags: review?(aaronr)

Comment 11

12 years ago
Comment on attachment 191725 [details] [diff] [review]
patch v3

please change the error message.  Doesn't seem like if you saw the message that
you'd immediately know what the problem was with your form.  Doesn't have to be
exactly this, but something along the lines of: "Submission failed trying to
replace instance document '%S'.  Instance document doesn't exist in same model
as the submission element.

otherwise it looks good.  With new error message, r=me
Attachment #191725 - Flags: review?(aaronr) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 192401 [details] [diff] [review]
patch v4 (diff -w)

Talked to Aaron about this, and I was doing too much work in the latest patch. 
This one is better.
Attachment #191725 - Attachment is obsolete: true
Attachment #192401 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #192401 - Flags: review?(smaug)

Comment 13

12 years ago
Comment on attachment 192401 [details] [diff] [review]
patch v4 (diff -w)

>Index: nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.37
>diff -u -6 -p -w -r1.37 nsXFormsSubmissionElement.cpp
>--- nsXFormsSubmissionElement.cpp	3 Aug 2005 14:30:54 -0000	1.37
>+++ nsXFormsSubmissionElement.cpp	11 Aug 2005 17:43:01 -0000
>@@ -486,39 +486,67 @@ nsXFormsSubmissionElement::LoadReplaceIn
>       nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceParseError"),
>                                  mElement);
>       return NS_ERROR_UNEXPECTED;
>     }
>   }
> 
>-  // get the nodeset that we are currently bound to
>-  nsCOMPtr<nsIDOMNode> data;
>+  // Get the appropriate instance node.  If the "instance" attribute is set,
>+  // then get that instance node.  Otherwise, get the one we are bound to.
>+  nsresult rv;
>   nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>   NS_ENSURE_STATE(model);
>-  nsresult rv = GetSelectedInstanceData(getter_AddRefs(data));
>+  nsCOMPtr<nsIInstanceElementPrivate> instanceElement;
>+  nsAutoString value;
>+  mElement->GetAttribute(NS_LITERAL_STRING("instance"), value);
>+  if (!value.IsEmpty()){

nit: space before '{'

>+    rv = GetSelectedInstanceElement(value, model,
>+                                    getter_AddRefs(instanceElement));
>+  } else {
>+    nsCOMPtr<nsIDOMNode> data;
>+    rv = GetBoundInstanceData(getter_AddRefs(data));
>   if (NS_SUCCEEDED(rv)) {
>     nsCOMPtr<nsIDOMNode> instanceNode;
>     rv = nsXFormsUtils::GetInstanceNodeForData(data, model, 
>                                                getter_AddRefs(instanceNode));
>     NS_ENSURE_SUCCESS(rv, rv);
> 

nit: please fix indentation inside the else section

>+nsresult
>+nsXFormsSubmissionElement::GetSelectedInstanceElement(
>+                                            const nsString &aInstanceID,
>+                                            nsIModelElementPrivate *aModel,
>+                                            nsIInstanceElementPrivate **aResult)
>+{

nit: aInstanceID parameter should be nsAString like other functions.

>Index: nsXFormsSubmissionElement.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.h,v
>retrieving revision 1.17
>diff -u -6 -p -w -r1.17 nsXFormsSubmissionElement.h
>--- nsXFormsSubmissionElement.h	30 Jun 2005 22:32:44 -0000	1.17
>+++ nsXFormsSubmissionElement.h	11 Aug 2005 17:43:01 -0000
>@@ -95,13 +95,14 @@ public:
> 
>   NS_HIDDEN_(nsresult) LoadReplaceInstance(nsIChannel *);
>   NS_HIDDEN_(nsresult) LoadReplaceAll(nsIChannel *);
>   NS_HIDDEN_(nsresult) Submit();
>   NS_HIDDEN_(PRBool)   GetBooleanAttr(const nsAString &attrName, PRBool defaultVal = PR_FALSE);
>   NS_HIDDEN_(void)     GetDefaultInstanceData(nsIDOMNode **result);
>-  NS_HIDDEN_(nsresult) GetSelectedInstanceData(nsIDOMNode **result);
>+  NS_HIDDEN_(nsresult) GetBoundInstanceData(nsIDOMNode **result);
>+  NS_HIDDEN_(nsresult) GetSelectedInstanceElement(const nsString &aInstance, nsIModelElementPrivate *aModel, nsIInstanceElementPrivate **result);

nit: aInstance parameter should be nsAString like other functions.

With those, r=me
Attachment #192401 - Flags: review?(aaronr) → review+

Updated

12 years ago
Attachment #192401 - Flags: review?(smaug) → review+
(Assignee)

Comment 14

12 years ago
checked in to trunk.
Whiteboard: xf-to-branch
(Reporter)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Comment 15

12 years ago
Removing xf-to-branch
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.