Last Comment Bug 302496 - Add instance attribute to submission element
: Add instance attribute to submission element
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: jhp (no longer active)
: Stephen Pride
Mentors:
http://www.w3.org/MarkUp/Forms/Group/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-28 06:33 PDT by Allan Beaufour
Modified: 2005-09-28 13:55 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
data for testcase (79 bytes, text/xml)
2005-07-28 08:58 PDT, Allan Beaufour
no flags Details
Testcase (1.40 KB, application/xhtml+xml)
2005-07-28 09:00 PDT, Allan Beaufour
no flags Details
Testcase v2 (1.40 KB, application/xhtml+xml)
2005-07-28 23:29 PDT, Allan Beaufour
no flags Details
patch (5.20 KB, patch)
2005-08-01 15:09 PDT, jhp (no longer active)
no flags Details | Diff | Review
patch v2 (7.12 KB, patch)
2005-08-04 09:40 PDT, jhp (no longer active)
no flags Details | Diff | Review
patch v3 (7.57 KB, patch)
2005-08-05 12:14 PDT, jhp (no longer active)
aaronr: review+
Details | Diff | Review
patch v4 (diff -w) (7.31 KB, patch)
2005-08-11 10:45 PDT, jhp (no longer active)
aaronr: review+
bugs: review+
Details | Diff | Review

Description Allan Beaufour 2005-07-28 06:33:40 PDT
The current errata (E69a) adds an instance attribute to the submission element,
specifying which instance should be replaced on replace="instance"
Comment 1 Allan Beaufour 2005-07-28 08:58:50 PDT
Created attachment 190850 [details]
data for testcase
Comment 2 Allan Beaufour 2005-07-28 09:00:46 PDT
Created attachment 190851 [details]
Testcase
Comment 3 Allan Beaufour 2005-07-28 09:01:52 PDT
(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 aaronr 2005-07-28 10:37:49 PDT
(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 :(
Comment 5 Allan Beaufour 2005-07-28 23:29:51 PDT
Created attachment 190927 [details]
Testcase v2

This uses an external document instead.
Comment 6 jhp (no longer active) 2005-08-01 15:09:46 PDT
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.
Comment 7 aaronr 2005-08-02 09:35:56 PDT
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.
Comment 8 jhp (no longer active) 2005-08-04 09:40:17 PDT
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.
Comment 9 aaronr 2005-08-04 22:57:01 PDT
(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.
Comment 10 jhp (no longer active) 2005-08-05 12:14:31 PDT
Created attachment 191725 [details] [diff] [review]
patch v3

Patch that uses |nsXFormsUtilityService::GetInstanceDocumentRoot()|.
Comment 11 aaronr 2005-08-10 16:03:07 PDT
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
Comment 12 jhp (no longer active) 2005-08-11 10:45:19 PDT
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.
Comment 13 aaronr 2005-08-11 13:14:42 PDT
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
Comment 14 jhp (no longer active) 2005-08-23 14:23:51 PDT
checked in to trunk.
Comment 15 Olli Pettay [:smaug] 2005-09-28 13:55:17 PDT
Removing xf-to-branch

Note You need to log in before you can comment on or make changes to this bug.