Implement <xforms:upload>

OK, here's a v1 patch that implements partial support for <xforms:upload>. There's currently no support for the mediatype attribute or the <filename> and <mediatype> child elements. Care is taken to not allow script to modify the path of the file to be uploaded. A content property is used to store the nsILocalFile to be uploaded. I think it might be useful to land this patch as is, and then go back and complete the rest of the features of <xforms:upload>.

Comment on attachment 169246 [details] [diff] [review] v1 patch (checked in) This looks ok for the initial patch. But add comments about missing features. And don't mark this bug fixed ;)

v1 patch checked in on trunk. leaving this bug open for further work.

Work-in-progress, with some minor bugs still left to work out. This patch XBLizes <upload>, making it extend "xformswidget-input". Sets <filename> and <mediatype> depending on the file chosen in the file picker. I made the "mediatype" attribute on <upload> use <html:input>'s "accept" attribute; unfortunately, that isn't implemented yet (bug 83749), but if we want to fix it, the work should take place in that bug, so both HTML and XForms can make use of it. This patch doesn't have support for "base64Binary" or "hexBinary". I am using |EvaluateNodeBinding()| in order to get the bindings for the <filename> and <mediatype> child elements. If it looks like this, |<filename ref="@name">|, and the instance data node has an attribute called "name", then my code works just as expected. However, if the bound instance data node doesn't have an attribute by that name, then |EvaluateNodeBinding()| returns an empty list. If I understand correctly, then a non-existant attribute on an existing node should be created when setting, but if we bind directly to a node (not its attr) and it doesn't exist, then we shouldn't create the instance node (unless we are lazy authoring). Correct? So how do I go about creating the attribute if it doesn't exist?

(In reply to comment #4) >...if the bound instance data node doesn't have an attribute by that name, then >|EvaluateNodeBinding()| returns an empty list. If I understand correctly, then >a non-existant attribute on an existing node should be created when setting, but >if we bind directly to a node (not its attr) and it doesn't exist, then we >shouldn't create the instance node (unless we are lazy authoring). Correct? So >how do I go about creating the attribute if it doesn't exist? In general (except as you point out for 'lazy authoring'), in XForms, nodes must exist before they can be bound to. If the bound node does not exist, then the form control is treated as not relevant. The case of attributes in the child element semi-widgets of upload raises an interesting question, which has perhaps already been answered in the XForms WG and you may know more than I do, but my naive assumption would be that the filename or mediatype would fail to bind, and either the entire upload would be not relevant, or the filename / mediatype would silently fail. As an author, I'd prefer the former, it's difficult to debug the latter, but absent any WG opinion, it's dealer's choice.

The magic 8 ball says filename and mediatype are not form controls and so in the case of failed @ref of single-node-binding attributes, the filename or mediatype element is ignored and the information is not put into the instance, but other operation of the upload is unaffected (in particular, the relevant property is not affected). In the case of failed @model or @bind, an error is signalled, as described in 3.2.3 single-node binding attributes.

Still work-in-progress. This patch fixes several bugs, as well as adding base64 & hex encoding. Hex encoding doesn't yet validate, but Doron promised to write that code! Have several questions, though: - From the spec: "... on activation upload places a URI in the content of the node." Currently, this code runs on "onblur", which isn't correct. My interpretation of "on activation" is: (1) if the user clicks the "Browse" button, then the file is handled when the filepicker returns, and (2) if the user types in the file path, then the file is handled when focus leaves the text field ("onblur"; should it also work on keypress=Enter?). I thought of using "onchange" on the <html:input>, but that gets called too often to be useful. So I was leaning towards implementing my own textfield and button (rather than depending on <html:input type="text">), and assigning separate functions to each. Anyone have a better idea? - From spec 8.3.1 <filename>: "For security reasons, upload must not take action due to any existing value of the node." I assume this means that the <upload> code should treat the <filename> node as write-only. Correct?

(In reply to comment #7) > Created an attachment (id=195870) [edit] > second patch v1.1 Hmm, you expose a "file" attribute, which stores the file content on the node. What is preventing someone using script to set this to some arbitry file? Why is it called UpdateElement btw.? Do you have bigger plans for it? If not, please call it Upload... > Still work-in-progress. This patch fixes several bugs, as well as adding > base64 & hex encoding. Hex encoding doesn't yet validate, but Doron promised > to write that code! Is there a bug on that? If not, please create it. Have several questions, though: > > - From the spec: "... on activation upload places a URI in the content of the > node." Currently, this code runs on "onblur", which isn't correct. My > interpretation of "on activation" is: (1) if the user clicks the "Browse" > button, then the file is handled when the filepicker returns, and (2) if the > user types in the file path, then the file is handled when focus leaves the > text field ("onblur"; should it also work on keypress=Enter?). I thought of > using "onchange" on the <html:input>, but that gets called too often to be > useful. So I was leaning towards implementing my own textfield and button > (rather than depending on <html:input type="text">), and assigning separate > functions to each. Anyone have a better idea? Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll write a message to the WG. But with the current wording, onblur is wrong, yes. > - From spec 8.3.1 <filename>: "For security reasons, upload must not take > action due to any existing value of the node." I assume this means that the > <upload> code should treat the <filename> node as write-only. Correct? Something like that, yes.

The spec also states: "Implementations which cannot support upload for the given mediatype must make this apparent to the user." We need to handle that. Will you include that Javier, or create a seperate bug?

(In reply to comment #8) > Hmm, you expose a "file" attribute, which stores the file content on the node. > What is preventing someone using script to set this to some arbitry file? According to Doron, doing QI in JS is a priviledged operation, and would result in a security error in non-chrome JS. So it shouldn't be a problem. I'll try to create a testcase to check this out, though. > Why is it called UpdateElement btw.? Ha! Every so often I caught myself writing "update" instead of "upload". Guess I let this one slip through. Thanks... > Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll > write a message to the WG. But with the current wording, onblur is wrong, yes. Well, I got "onchange" to work. I guess I was doing something wrong the first time that I evaluated it. But now, it's working much better. (In reply to comment #9) > The spec also states: "Implementations which cannot support upload for the given > mediatype must make this apparent to the user." > We need to handle that. Will you include that Javier, or create a seperate bug? Well, at the very least, we'll allow the user to upload a file that matches that mediatype, so there's nothing for us to warn the user about. So, for example, if the mediatype is set to "audio/*", one implementation may choose to allow the user to open a recording dialog, whereas we'll just let the user upload a file matching that mimetype. Are you interpreting that line differently?

(In reply to comment #10) > (In reply to comment #8) > > Hmm, you expose a "file" attribute, which stores the file content on the node. > > What is preventing someone using script to set this to some arbitry file? > > According to Doron, doing QI in JS is a priviledged operation, and would result > in a security error in non-chrome JS. So it shouldn't be a problem. Forgot the non-automatic QI. > I'll try to create a testcase to check this out, though. Yes please. We'll better be sure :-) > > Hmmmm, "on activation" *sigh*. That's loosely defined (or not at all...). I'll > > write a message to the WG. But with the current wording, onblur is wrong, yes. > > Well, I got "onchange" to work. I guess I was doing something wrong the first > time that I evaluated it. But now, it's working much better. I still thinks the "on activation" is a bit vague. But, when you have a patch ready I'll evaluated the UI "experience". > (In reply to comment #9) > > The spec also states: "Implementations which cannot support upload for the given > > mediatype must make this apparent to the user." > > We need to handle that. Will you include that Javier, or create a seperate bug? > > Well, at the very least, we'll allow the user to upload a file that matches that > mediatype, so there's nothing for us to warn the user about. So, for example, > if the mediatype is set to "audio/*", one implementation may choose to allow the > user to open a recording dialog, whereas we'll just let the user upload a file > matching that mimetype. Are you interpreting that line differently? I was referring to the setting of the mediatype element, but I did not notice the "and a mediatype is available" the first time. Just forget my comment.

(In reply to comment #7) > - From spec 8.3.1 <filename>: "For security reasons, upload must not take > action due to any existing value of the node." I assume this means that the > <upload> code should treat the <filename> node as write-only. Correct? Yes. Presumably it also excludes reading the mediatype binding as well. The language is written broadly to encourage the XForms processor developer to think broadly about the security aspects of this control, which is what's happening in this bug, so it's working ;-) As for medaitype, there's a mediatype attribute on upload. For security reasons, there's no corresponding way to suggest filenames, just as you suspect.

> I still thinks the "on activation" is a bit vague. But, when you have a patch > ready I'll evaluated the UI "experience". The only help I can offer is http://www.w3.org/TR/xforms/slice4.html#sequence-for-input which describes input event sequencing though you may wish to check for published and unpublished errata.

Ready for review. This patch has the following changes: - Using the "onchange" attribute on <html:input> in order to kick of the file path processing. - Moved the <upload> element initialization code form the "refresh" method to the "constructor". - So Allan was right: a Javascript on the webpage can QI and set the "file" attribute. So I changed that to a "fileUpdated" method, that lets the element know when the user has changed the file path. I also exposed a getter, so I can retrieve the value from the <html:input> element. - 'Multipart-post' was broken in several places, but it now works. One of the things I did was make |ParseTypeFromNode()| return the actual namespace URI, rather than just returning the prefix used. - Fixed a crash in the submission code: we weren't checking the return value of |SerializeDataXML()|. I looked over the spec one final time, and I think I've covered everything. But as mentioned earlier, for the "mediatype" attribute on <upload>, I am making use of <html:input>'s "accept" attr, which hasn't been implemented, yet.

(In reply to comment #14) > Created an attachment (id=196249) [edit] > second patch v1.2 Please correct the nits: http://lab.cph.novell.com/~abeaufour/jst-review/?patch=196249 I'll start looking at the rest now. Sorry for the delay.

Comment on attachment 196249 [details] [diff] [review] second patch v1.2 > Index: nsIXFormsInputUIElement.idl > =================================================================== > +interface nsIDOMElement; > + > +/** > + * Interface implemented by the XBL part of the input element. > + */ > +[scriptable, uuid(82c82c46-f111-416c-a3e6-ce9a0864d00f)] > +interface nsIXFormsInputUIElement : nsISupports > +{ > + /** > + * Returns the underlying <html:input> element. > + */ > + readonly attribute nsIDOMElement inputField; > +}; > Index: nsIXFormsUploadElement.idl > =================================================================== > +[scriptable, uuid(6dc00fd0-674c-41b3-8b0b-ba6c02aa769a)] > +interface nsIXFormsUploadElement : nsISupports > +{ > + /** > + * Let the upload element know that the user has selected or entered > + * a file path. > + */ > + void fileUpdated(); > +}; An "evil" upload control could return whatever as the inputField (ie, an input control with the value always set to /etc/passwd...) This leads to something I should have mentioned some time ago (sorry, I forgot about it). It should not be possible to "skin" an upload control. As far as I can see this will never be safe. The file name should never come from a control/code that is user-changeable. This means that the upload should either be done in XTF, or (if it's possible) only be bindable to chrome content. I think you should split the submission fixes, etc. to another bug and only have "pure upload" stuff in this one. It is crucial that we get the upload part right.

Comment on attachment 196249 [details] [diff] [review] second patch v1.2 will review when allan's comments are addressed.

(In reply to comment #16) > I think you should split the submission fixes, etc. to another bug and only > have "pure upload" stuff in this one. -> bug 309546

(In reply to comment #16) > This means that the upload should either be done in XTF... I thought we wanted the controls to be XBL. This prevents the out-of-order issue that was fixed on other controls by going to XBL. I was talking to Doron about how best to provide security, but also have the control defined in XBL, but didn't come up with anything definitive. What's your take? > ... or (if it's possible) > only be bindable to chrome content. Can you explain this a little better?

(In reply to comment #19) > (In reply to comment #16) > > This means that the upload should either be done in XTF... > > I thought we wanted the controls to be XBL. This prevents the out-of-order > issue that was fixed on other controls by going to XBL. I was talking to Doron > about how best to provide security, but also have the control defined in XBL, > but didn't come up with anything definitive. What's your take? My best shot at it right now: Let "file picking" happen in XTF and _only_ the UI in XBL. That is, expose a .pickFile() command on the XTF upload element. So the XBL binding can create "beautiful" buttons/icons, and then on activation they just call .pickFile() which (in XTF) opens the file picker dialog, get's the file name, stores the file, etc. etc. Of course, we limit ourselves to picking files, but I'm fine with that for now. > > ... or (if it's possible) only be bindable to chrome content. > > Can you explain this a little better? Assuming that chrome content is trusted, then if we can restrict bindings to the upload control to chrome:// then we could implement everything in XBL. Another note: You need to run the "value changed" sequence when you set mediatype and filename, do you do that?

(In reply to comment #20) > My best shot at it right now: Let "file picking" happen in XTF and _only_ the UI > in XBL. What's good about the approach I took in the last patch is that I got to piggyback on the <html:input type="file"> code. In your approach, I'd have to duplicate a lot of that code. But we may have no choice...

Also, using Allan's approach, we would have to remove the ability for the user to manually input the path of the file, since a script could change the input field value. So here are our choices: 1) Go along the lines of the existing upload code (returning the upload element through |nsIXTFVisual::GetVisualContent()|, etc). From what I understand, scripts wouldn't have access to the <html:input>. However, it wouldn't be skinnable. 2) Go with Allan's suggestion of putting the UI in XBL, but the actual code in XTF. This has the advantage of being skinnable, but we would have to duplicate some of the <html:input> code, and we would have to make the text field readonly, for security reasons. Or we could remove the text field altogether and just have a button that brings up the filepicker. This is what the other XForms engines do. Personally, I prefer keeping the text field; gives feedback on which file was chosen. What do you guys think?

So this patch goes with Allan's suggestion. Clicking the "Browse..." button calls a method on the XTF side. That method takes care of opening the file picker and setting the file path/contents into the instance data. As discussed above, I added a readonly text field that holds the path of the selected file. Since the user can no longer select the text and clear it out, should we add a "Clear"/"Reset" button, which would remove the currently selected file? In this patch, the "Browse..." text of the button is hard coded in 'xforms.xml'. How would I go about getting the localized string (available in HtmlForm.properites)? This patch does not handle mediatype filters on the filepicker.

I've been thinking about this some more. The <html:input type="file"> element already comes with some pretty good security. A script cannot set its value, and if it tries to convert a regular <html:input> to a 'type="file"', its value gets reset. So looking back at 'second patch v1.2' (attachment 196249 [details] [diff] [review]), in the function |GetHTMLInputValue()|, once I've know I've got an nsIDOMHTMLInputElement, I need to check to see if it is a 'type="file"'. Now the question is, could a malicious script somehow fake that, and say that it it 'type="file"', when it is really not?

(In reply to comment #24) > I've been thinking about this some more. The <html:input type="file"> element > already comes with some pretty good security. A script cannot set its value, > and if it tries to convert a regular <html:input> to a 'type="file"', its value > gets reset. > > So looking back at 'second patch v1.2' (attachment 196249 [details] [diff] [review] [edit]), in the function > |GetHTMLInputValue()|, once I've know I've got an nsIDOMHTMLInputElement, I need > to check to see if it is a 'type="file"'. > > Now the question is, could a malicious script somehow fake that, and say that it > it 'type="file"', when it is really not? If someone skins upload, how can you be sure it'll have an input field for you to talk to?

> Now the question is, could a malicious script somehow fake that, and say that it > it 'type="file"', when it is really not? Probably. Perhaps a QI to an interface would work, but that could be faked too. We do have a way of making sure we QI to an interface that isn't being spoofed, NativeWrapper or such. Asking on IRC would work.

(In reply to comment #25) > If someone skins upload, how can you be sure it'll have an input field for you > to talk to? This should't matter. When a user selects a file (by typing, or through picker), the value of the element gets set, whether or not there is an input field.

So from talking to Mozilla devs on IRC, it looks like there may still be security issues with using <html:input>. This patch, then, continues on the last patch, where I created my own <upload> widget, rather than depending on "xformswidget-input". Changes: - Added a "Clear" button and supporting code. - Set localized button text from XTF, on |WidgetAttached()|. - Cleaned up and merged some code. - Properly send model update events.

Upload example from the XForms specification

Comment on attachment 197760 [details] [diff] [review] second patch v1.4 When I try the simple example from the spec. and pick a random file from my disk, I get this: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'Permission denied to get property Object.file' when calling method: [nsIFilePicker::file]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" data: no] ************************************************************ JavaScript error: , line 0: uncaught exception: [Exception... "'Permission denied to get property Object.file' when calling method: [nsIFilePicker::file]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" data: no]

Hmm, that's very strange. It works fine for me on the latest trunk build. Can anyone else try out this patch?

Supporting @mediatype now that you have your own custom control and aren't depending on xhtml input/@type='file' would be nice. The request for mediatype control over the file types presented pre-dates the creation of the XForms WG, and the has been a hot button for years. A simple mechanism would just use default file extensions looked up from the MIME type list.

Yes, I was going to look into supporting 'mediatype' soon, but I wanted to get this patch up and reviewed. I'll either do that here in another patch, or open a new bug.

(In reply to comment #31) > Hmm, that's very strange. It works fine for me on the latest trunk build. Can > anyone else try out this patch? Works fine for me on a Windows trunk pull that I did last night. Was yours a branch build, Allan? I assume Linux?

With the latest patch the test case doesn't do anything (Fedora 3). I can open and select a file, but the data model is not updated. I don't see the errors Allan has. ...or am I missing something here...

The problem with the attached testcase is that the <attachment> node has no data type. From the spec: "Data Binding Restrictions: This form control can only be bound to datatypes xsd:anyURI, xsd:base64Binary or xsd:hexBinary, or types derived by restriction from these." So the question is what should happen? FormsPlayer updates the <filename> and <mediatype> nodes, but does nothing with <attachment>. The Novell plugin disables the <upload> element altogether. Aaron pointed out this sentence in 4.2.2: "The binding expression is evaluated to ensure that it points to a node that exists. If this is not the case then the form control should behave in the same manner as if it had bound to a model item with the relevant model item property resolved to false." He says that the Data Binding Restriction should be stated more clearly, like this last sentence. Allan, can you bring this up with the WG, to ask what should happen if <upload> is not bound to one of those datatypes?

Added a working testcase to the URL field at top of bug.

Ok, better testcase

(In reply to comment #34) > (In reply to comment #31) > > Hmm, that's very strange. It works fine for me on the latest trunk build. Can > > anyone else try out this patch? > > Works fine for me on a Windows trunk pull that I did last night. Was yours a > branch build, Allan? I assume Linux? It was a Linux trunk build... a new build this morning worked. Weird.

(In reply to comment #36) > The problem with the attached testcase is that the <attachment> node has no data > type. From the spec: > > "Data Binding Restrictions: This form control can only be bound to datatypes > xsd:anyURI, xsd:base64Binary or xsd:hexBinary, or types derived by restriction > from these." > > So the question is what should happen? FormsPlayer updates the <filename> and > <mediatype> nodes, but does nothing with <attachment>. The Novell plugin > disables the <upload> element altogether. > > Aaron pointed out this sentence in 4.2.2: > "The binding expression is evaluated to ensure that it points to a node that > exists. If this is not the case then the form control should behave in the same > manner as if it had bound to a model item with the relevant model item property > resolved to false." > > He says that the Data Binding Restriction should be stated more clearly, like > this last sentence. Allan, can you bring this up with the WG, to ask what > should happen if <upload> is not bound to one of those datatypes? Hmmm, well it's not just upload, f.x. range also has type restrictions. I think the Novell plugin does the right thing. It should be altogether disabled.

Comment on attachment 197760 [details] [diff] [review] second patch v1.4 * Could you include a note somewhere about why "@incremental" is ignored? * Include an XXX about mediatype filtering and a new bug? > Index: nsIXFormsUploadElement.idl > =================================================================== Please include a comment for the interface > + > + > +[scriptable, uuid(6dc00fd0-674c-41b3-8b0b-ba6c02aa769a)] > +interface nsIXFormsUploadElement : nsISupports > +{ > + /** > + * Tells <upload> element to display file picker dialog so user can > + * select a file. > + */ > + void pickFile(); Please use \<upload\>, that will make Doxygen oh so happy :) Also in the rest of the patch. > + > + /** > + * Tells <upload> element to clear file. > + */ > + void clearFile(); That's not the most descriptive comment. What does "clear" mean? "Clear" it from the disk? :) > Index: nsXFormsSubmissionElement.cpp > =================================================================== > - nsILocalFile *file = > - NS_STATIC_CAST(nsILocalFile *, > + nsIFile *file = > + NS_STATIC_CAST(nsIFile *, > content->GetProperty(nsXFormsAtoms::uploadFileProperty)); Why this change? Because we do not need the extra functionality in nsILocalFile? > Index: nsXFormsUploadElement.cpp > =================================================================== > + /** > + * Sets the value of the instance data node bound to <filename> or > + * <mediatype> child elements. > + **/ > + nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue, > + PRBool* aChanged); Comment not entirely correct, it can set an arbitrary node value, right? Which makes me think that it should 1) use nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to nsXFormsUtils as a generic function. > + > + /** > + * Read the contents of the file and encode in Base64 or Hex. |aResult| must > + * be freed by nsMemory::Free(). > + **/ > + nsresult EncodeFileContents(nsIFile* aFile, nsEncodingType aType, > + PRUnichar** aResult); > + > + void BinaryToHex(const char* aBuffer, PRUint32 aCount, > + PRUnichar** aHexString); > + > -NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsUploadElement, > - nsXFormsControlStub, > - nsIDOMFocusListener, > - nsIDOMEventListener) > + nsString mFilePickerTitle; > + nsString mClearButtonText; You do not use this do you? > + > + nsCOMPtr<nsIDOMNode> mFilenameNode; > + nsCOMPtr<nsIDOMNode> mMediatypeNode; > +}; > -// nsIXTFXMLVisual > +NS_IMPL_ISUPPORTS_INHERITED1(nsXFormsUploadElement, > + nsXFormsDelegateStub, > + nsIXFormsUploadElement) > NS_IMETHODIMP > -nsXFormsUploadElement::OnCreated(nsIXTFXMLVisualWrapper *aWrapper) > +nsXFormsUploadElement::WidgetAttached() > { > - nsresult rv = nsXFormsControlStub::OnCreated(aWrapper); > + nsresult rv = nsXFormsDelegateStub::WidgetAttached(); > NS_ENSURE_SUCCESS(rv, rv); > - // Our anonymous content structure will look like this: > - // > - // <label> (mLabel) > - // <span/> (insertion point) > - // <input/> (mInput) > - // </label> > - > - nsCOMPtr<nsIDOMDocument> domDoc; > - mElement->GetOwnerDocument(getter_AddRefs(domDoc)); > - > - domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML), > - NS_LITERAL_STRING("label"), > - getter_AddRefs(mLabel)); > - NS_ENSURE_STATE(mLabel); > - > - nsCOMPtr<nsIDOMElement> element; > - nsCOMPtr<nsIDOMNode> childReturn; > - > - domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML), > - NS_LITERAL_STRING("span"), > - getter_AddRefs(element)); > - NS_ENSURE_STATE(element); > - > - mLabel->AppendChild(element, getter_AddRefs(childReturn)); > - > - domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML), > - NS_LITERAL_STRING("input"), > - getter_AddRefs(element)); > - mInput = do_QueryInterface(element); > - NS_ENSURE_STATE(mInput); > - > - mInput->SetType(NS_LITERAL_STRING("file")); > + // get string bundle > + nsCOMPtr<nsIStringBundleService> bundleService = > + do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIStringBundle> bundle; > + rv = bundleService->CreateBundle(NS_HTMLFORM_BUNDLE_URL, > + getter_AddRefs(bundle)); > + NS_ENSURE_SUCCESS(rv, rv); > - mLabel->AppendChild(mInput, getter_AddRefs(childReturn)); > + // get localized "Browse..." text and set into button > + nsXPIDLString value; > + rv = bundle->GetStringFromName(NS_LITERAL_STRING("Browse").get(), > + getter_Copies(value)); > + if (NS_FAILED(rv)) { > + // fall back to English text > + value.AssignASCII("Browse..."); > + } > + nsCOMPtr<nsIXFormsUploadUIElement> uiUpload = do_QueryInterface(mElement); > + if (uiUpload) { > + uiUpload->SetBrowseButtonText(value); > + } Get mFilePickerTitle before this QI, and then exit if the QI fails. No reason to fetch strings if you cannot set them. > - // We can't use xtf handleDefault here because editor stops blur events from > - // bubbling, and we can't use a system event group handler because blur > - // events don't go to the system event group (bug 263240). So, just install > - // a bubbling listener in the normal event group. This works out because > - // content can't get at the anonymous content to install an event handler, > - // and the event doesn't bubble up. > + // get localized file picker title text > + rv = bundle->GetStringFromName(NS_LITERAL_STRING("FileUpload").get(), > + getter_Copies(value)); > + if (NS_SUCCEEDED(rv)) { > + mFilePickerTitle.Assign(value); > + } else { > + // fall back to English text > + mFilePickerTitle.AssignASCII("File Upload"); > + } > - nsCOMPtr<nsIDOMEventTarget> targ = do_QueryInterface(mInput); > - NS_ASSERTION(targ, "input must be an event target!"); > + // get localized text for clear button > + rv = bundleService->CreateBundle(NS_XFORMS_BUNDLE_URL, > + getter_AddRefs(bundle)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = bundle->GetStringFromName(NS_LITERAL_STRING("clearButtonText").get(), > + getter_Copies(value)); > + if (NS_FAILED(rv)) { > + // fall back to English text > + value.AssignASCII("Clear"); > + } > + if (uiUpload) { > + uiUpload->SetClearButtonText(value); > + } > - targ->AddEventListener(NS_LITERAL_STRING("blur"), this, PR_FALSE); > return NS_OK; > } > -NS_IMETHODIMP > -nsXFormsUploadElement::AttributeSet(nsIAtom *aName, const nsAString &aValue) > +static void > +ReleaseObject(void *aObject, > + nsIAtom *aPropertyName, > + void *aPropertyValue, > + void *aData) I know you just moved this, but for consistency in the file please move the "*" next to the type. (I'd rather have them next to the name for all functions, but as long as it's consitant in the individual files...) > { > - if (aName == nsXFormsAtoms::accesskey) { > - // accesskey > - mInput->SetAttribute(NS_LITERAL_STRING("accesskey"), aValue); > - } > - > - return NS_OK; > + NS_STATIC_CAST(nsISupports *, aPropertyValue)->Release(); > } > NS_IMETHODIMP > -nsXFormsUploadElement::AttributeRemoved(nsIAtom *aName) > +nsXFormsUploadElement::PickFile() > { > - if (aName == nsXFormsAtoms::accesskey) { > - // accesskey > - mInput->RemoveAttribute(NS_LITERAL_STRING("accesskey")); > - } > + nsresult rv; > - return NS_OK; > -} > + // get nsIDOMWindowInternal > + nsCOMPtr<nsIDOMDocument> doc; > + mElement->GetOwnerDocument(getter_AddRefs(doc)); null-check mElement > + PRBool changedM = PR_FALSE; changedM? More descriptive name. > + if (!aFile) { > + // clear instance data > + content->DeleteProperty(nsXFormsAtoms::uploadFileProperty); > + rv = mModel->SetNodeValue(mBoundNode, EmptyString(), &changedM); > + } else { > + // set file into instance data > - // We need to special case an empty value. If the input field is blank, > - // then I assume that the intention is that all bound nodes will become > - // (or stay) empty and that there shouldn't be any file kept in the property. > - if (!value.IsEmpty()) { > - NS_NewLocalFile(value, PR_FALSE, &file); > - NS_ENSURE_STATE(file); > - NS_GetURLSpecFromFile(file, spec); > - content->SetProperty(nsXFormsAtoms::uploadFileProperty, file, > + nsIFile* fileCopy = nsnull; > + rv = aFile->Clone(&fileCopy); > + NS_ENSURE_SUCCESS(rv, rv); > + content->SetProperty(nsXFormsAtoms::uploadFileProperty, fileCopy, > ReleaseObject); Check that this succeeds? And how about moving this down below the type checking. If the type's unknown you shouldn't store the property I guess. > - } else { > - content->DeleteProperty(nsXFormsAtoms::uploadFileProperty); > - } > + nsAutoString type, prefix, nsuri; > + rv = nsXFormsUtils::ParseTypeFromNode(mBoundNode, type, prefix); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // get the namespace url from the prefix > + nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = domNode3->LookupNamespaceURI(prefix, nsuri); > + > + if (NS_SUCCEEDED(rv) && nsuri.EqualsLiteral(NS_NAMESPACE_XML_SCHEMA)) > + { > + if (type.EqualsLiteral("anyURI")) { > + // set fully qualified path as value in instance data node > + nsCAutoString spec; > + NS_GetURLSpecFromFile(aFile, spec); > + rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), > + &changedM); > + } > + else if (type.EqualsLiteral("base64Binary") || move else up > + type.EqualsLiteral("hexBinary")) Use named string, or somewhat ugly |(isBinary = type.EqualsLiteral("hexBinary"))| > + { > + // encode file contents in base64/hex and set into instance data node > + nsEncodingType encoding; > + if (type.EqualsLiteral("base64Binary")) > + encoding = ENCODE_BASE64; > + else > + encoding = ENCODE_HEX; > + > + PRUnichar* fileData; > + rv = EncodeFileContents(aFile, encoding, &fileData); > + if (NS_SUCCEEDED(rv)) { > + rv = mModel->SetNodeValue(mBoundNode, nsDependentString(fileData), > + &changedM); > + nsMemory::Free(fileData); > + } > + } > + else { move else up > + NS_ERROR("Unknown type"); Please ReportError() for both unknown type and no type. > + } > + > + // XXX need to handle derived types Please create a bug for that > + } > + } > + NS_ENSURE_SUCCESS(rv, rv); > - // update the instance data node. this value is never used by the submission > - // code. instead, it exists so that the instance data will appear to be in- > - // sync with what is actually submitted. > - > - PRBool changed; > - nsresult rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), > - &changed); > + PRBool changedH; more descriptive naming > + rv = HandleChildElements(aFile, &changedH); > NS_ENSURE_SUCCESS(rv, rv); > - if (changed) { > + > + if (changedM || changedH) { > nsCOMPtr<nsIDOMNode> model = do_QueryInterface(mModel); > + > if (model) { > rv = nsXFormsUtils::DispatchEvent(model, eEvent_Recalculate); > NS_ENSURE_SUCCESS(rv, rv); > rv = nsXFormsUtils::DispatchEvent(model, eEvent_Revalidate); > NS_ENSURE_SUCCESS(rv, rv); > rv = nsXFormsUtils::DispatchEvent(model, eEvent_Refresh); > NS_ENSURE_SUCCESS(rv, rv); > } > } > + > return NS_OK; > } > -// nsIXFormsControl > +nsresult > +nsXFormsUploadElement::HandleChildElements(nsILocalFile* aFile, > + PRBool* aChanged) > +{ > + *aChanged = PR_FALSE; > + > + // return immediately if we have no children > + PRBool hasNodes; > + mElement->HasChildNodes(&hasNodes); > + if (!hasNodes) > + return NS_OK; > + > + nsresult rv = NS_OK; > -NS_IMETHODIMP > -nsXFormsUploadElement::Refresh() > + // look for the <filename> & <mediatype> elements in the <upload> children > + if (!mFilenameNode || !mMediatypeNode) { If the instance data is changed, these could point to the wrong nodes. > + nsCOMPtr<nsIDOMNode> child, temp; > + mElement->GetFirstChild(getter_AddRefs(child)); > + while (child && (!mFilenameNode || !mMediatypeNode)) { > + if (!mFilenameNode && > + nsXFormsUtils::IsXFormsElement(child, > + NS_LITERAL_STRING("filename"))) > + { > + mFilenameNode = child; > + } > + > + if (!mMediatypeNode && > + nsXFormsUtils::IsXFormsElement(child, > + NS_LITERAL_STRING("mediatype"))) > + { > + mMediatypeNode = child; > + } > + > + temp.swap(child); > + temp->GetNextSibling(getter_AddRefs(child)); > + } > + } > + > + // handle "filename" > + PRBool changedF = PR_FALSE; > + if (mFilenameNode) { > + if (aFile) { > + nsAutoString filename; > + rv = aFile->GetLeafName(filename); > + if (!filename.IsEmpty()) { > + rv = SetChildNodeValue(mFilenameNode, filename, &changedF); > + } > + } else { > + rv = SetChildNodeValue(mFilenameNode, EmptyString(), &changedF); > + } > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + // handle "mediatype" > + PRBool changedM = PR_FALSE; > + if (mMediatypeNode) { > + if (aFile) { > + nsCOMPtr<nsIMIMEService> mimeService = > + do_GetService(NS_MIMESERVICE_CONTRACTID, &rv); > + if (NS_SUCCEEDED(rv)) { > + nsCAutoString contentType; > + rv = mimeService->GetTypeFromFile(aFile, contentType); > + if (NS_FAILED(rv)) { > + contentType.AssignLiteral("application/octet-stream"); Where is this specified? > + } > + rv = SetChildNodeValue(mMediatypeNode, > + NS_ConvertUTF8toUTF16(contentType), &changedM); > + } > + } else { > + rv = SetChildNodeValue(mMediatypeNode, EmptyString(), &changedM); > + } > + } > + > + *aChanged = changedF || changedM; > + return rv; > +} > + > +nsresult > +nsXFormsUploadElement::EncodeFileContents(nsIFile* aFile, nsEncodingType aType, > + PRUnichar** aResult) > { > - if (!mInput || !mBoundNode) > - return NS_OK; > + nsresult rv; > - nsCOMPtr<nsIContent> content = do_QueryInterface(mBoundNode); > - NS_ENSURE_STATE(content); > + // get file contents > + nsCOMPtr<nsIInputStream> fileStream; > + rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), aFile, > + PR_RDONLY, -1, -1? Please comment. > + nsIFileInputStream::CLOSE_ON_EOF); > + NS_ENSURE_SUCCESS(rv, rv); > - nsILocalFile *file = > - NS_STATIC_CAST(nsILocalFile *, > - content->GetProperty(nsXFormsAtoms::uploadFileProperty)); > - > - // get the text value for the control > - nsAutoString value; > - if (file) > - file->GetPath(value); > + PRUint32 size; > + rv = fileStream->Available(&size); > + NS_ENSURE_SUCCESS(rv, rv); Possibly warn user when trying to upload BIG file... hmm, if html:upload does it. > - mInput->SetValue(value); > - mInput->SetReadOnly(GetReadOnlyState()); > + char* fileData = new char[size + 1]; Could we use a smart pointer for this? I would feel safer. > + if (!fileData) { > + return NS_ERROR_OUT_OF_MEMORY; > + } Please ReportError() about it. > - return NS_OK; > + PRUint32 bytesRead; > + rv = fileStream->Read(fileData, size, &bytesRead); > + NS_ASSERTION(NS_SUCCEEDED(rv) && bytesRead == size, > + "fileStream->Read failed"); > + > + if (NS_SUCCEEDED(rv)) { > + > + if (aType == ENCODE_BASE64) { > + // encode file contents > + *aResult = nsnull; > + char* buffer = PL_Base64Encode(fileData, bytesRead, nsnull); > + if (buffer) { > + *aResult = ToNewUnicode(nsDependentCString(buffer)); > + } is buffer automatically deleted? > + if (!*aResult) { > + rv = NS_ERROR_OUT_OF_MEMORY; > + } > + } move else up > + else if (aType == ENCODE_HEX) { > + // create buffer for hex encoded data > + PRUint32 length = bytesRead * 2 + 1; > + PRUnichar* fileDataHex = > + NS_STATIC_CAST(PRUnichar*, nsMemory::Alloc(length * sizeof(PRUnichar))); > + if (!fileDataHex) { > + rv = NS_ERROR_OUT_OF_MEMORY; > + } else { > + // encode file contents > + BinaryToHex(fileData, bytesRead, &fileDataHex); > + fileDataHex[bytesRead * 2] = 0; > + *aResult = fileDataHex; > + } > + } > + > + else { move else up > + NS_ERROR("Unknown encoding type for <upload> element"); > + rv = NS_ERROR_INVALID_ARG; > + } > + } > + > + delete [] fileData; > + > + return rv; > } I did not get further that this, gotta go :(

(In reply to comment #41) > * Could you include a note somewhere about why "@incremental" is ignored? Hmm, you are right. I thought this was working (maybe with old patch). For <upload>, when "true", this will "generate additional xforms-value-changed events". Not sure what this means exactly. But with this patch, I see 'xforms-value-changed' events sent whether "@incremental" is true or false. So I guess when false, I shouldn't send those event? > * Include an XXX about mediatype filtering and a new bug? Since this is a generic but for <upload>, I'll just handle it in another patch here, rather than creating a new bug. > > - nsILocalFile *file = > > - NS_STATIC_CAST(nsILocalFile *, > > + nsIFile *file = > > + NS_STATIC_CAST(nsIFile *, > > content->GetProperty(nsXFormsAtoms::uploadFileProperty)); > > Why this change? Because we do not need the extra functionality in > nsILocalFile? Correct. Plus, when setting property in 'nsXFormsUploadElement.cpp', I clone the file, which returns an nsIFile. > > + nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue, > > + PRBool* aChanged); > > Comment not entirely correct, it can set an arbitrary node value, right? Which > makes me think that it should 1) use > nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to > nsXFormsUtils as a generic function. I don't know about _using_ |GetSingleNodeBindingValue|, but I could definitely make a |Set...| function that looks very much like it, in nsXFormsUtils. > I know you just moved this, but for consistency in the file please move the > "*" next to the type. (I'd rather have them next to the name for all > functions, but as long as it's consitant in the individual files...) Well, I decided to be consistent with the rest of XForms, and put "*" next to the name. > > + rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), > > + &changedM); > > + } > > + else if (type.EqualsLiteral("base64Binary") || > > move else up I usually do this to break up the blocks and make it easier to read at a glance. But I can move it up if you really want. > > + nsCOMPtr<nsIMIMEService> mimeService = > > + do_GetService(NS_MIMESERVICE_CONTRACTID, &rv); > > + if (NS_SUCCEEDED(rv)) { > > + nsCAutoString contentType; > > + rv = mimeService->GetTypeFromFile(aFile, contentType); > > + if (NS_FAILED(rv)) { > > + contentType.AssignLiteral("application/octet-stream"); > > Where is this specified? What are you referring to? I took the mimeService code from nsHTMLInputElement.cpp. > Possibly warn user when trying to upload BIG file... hmm, if html:upload does > it. No, <html:input type="file"> doesn't warn the user, but it doesn't encode the file contents. I don't think we should warn the user. It should be up to the form developer to choose the appropriate way to transmit the file. The form developer should know that if there is the possibility of sending large files, they should be transmitted as "multipart-post". > > + if (!fileData) { > > + return NS_ERROR_OUT_OF_MEMORY; > > + } > > Please ReportError() about it. For an out of memory error? Why?

(In reply to comment #42) > (In reply to comment #41) > > * Could you include a note somewhere about why "@incremental" is ignored? > > Hmm, you are right. I thought this was working (maybe with old patch). For > <upload>, when "true", this will "generate additional xforms-value-changed > events". Not sure what this means exactly. But with this patch, I see > 'xforms-value-changed' events sent whether "@incremental" is true or false. So > I guess when false, I shouldn't send those event? For this type of control, I believe there should be no difference. Just note that. But if it where a text field the user typed into, it should behave as input (sending events for each keypress). > > > + nsresult SetChildNodeValue(nsIDOMNode* aDOMNode, const nsAString &aValue, > > > + PRBool* aChanged); > > > > Comment not entirely correct, it can set an arbitrary node value, right? Which > > makes me think that it should 1) use > > nsXFormsUtils::GetSingleNodeBindingValue() and 2) possibly be moved to > > nsXFormsUtils as a generic function. > > I don't know about _using_ |GetSingleNodeBindingValue|, but I could definitely > make a |Set...| function that looks very much like it, in nsXFormsUtils. Why can you not use GetSingleNodeBindingValue() do you not do the same? > > > + rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), > > > + &changedM); > > > + } > > > + else if (type.EqualsLiteral("base64Binary") || > > > > move else up > > I usually do this to break up the blocks and make it easier to read at a glance. > But I can move it up if you really want. I _really_ want it :) It's what we do in the rest of the code (or most of it anyway). > > > + nsCOMPtr<nsIMIMEService> mimeService = > > > + do_GetService(NS_MIMESERVICE_CONTRACTID, &rv); > > > + if (NS_SUCCEEDED(rv)) { > > > + nsCAutoString contentType; > > > + rv = mimeService->GetTypeFromFile(aFile, contentType); > > > + if (NS_FAILED(rv)) { > > > + contentType.AssignLiteral("application/octet-stream"); > > > > Where is this specified? > > What are you referring to? That contentType defaults to "application/octet-stream". But if that's how html:input handles it, let's just follow that. > > Possibly warn user when trying to upload BIG file... hmm, if html:upload does > > it. > > No, <html:input type="file"> doesn't warn the user, but it doesn't encode the > file contents. I don't think we should warn the user. It should be up to the > form developer to choose the appropriate way to transmit the file. The form > developer should know that if there is the possibility of sending large files, > they should be transmitted as "multipart-post". > > > > + if (!fileData) { > > > + return NS_ERROR_OUT_OF_MEMORY; > > > + } > > > > Please ReportError() about it. > > For an out of memory error? Why? I might be overfocused on OOM errors, but this is just _the_ easy place to generate that for XForms (except for _big_ instances), because it lets the user choose an arbitrary large file. The form author has no clue to whether the user chooses a 400M core dump file, and tries to upload it. It's not a must for me to report this to the user, but I think it would be useful. At least we should emit some debug messages. What do the rest of you think?

(In reply to comment #41) > (From update of attachment 197760 [details] [diff] [review] [edit]) > I did not get further that this, gotta go :( I just checked the rest. No further comments from me.

(In reply to comment #43) >The form author has no clue to whether the user > chooses a 400M core dump file, and tries to upload it. > > It's not a must for me to report this to the user, but I think it would be > useful. At least we should emit some debug messages. What do the rest of you think? I have two suggestions: 1. The look and feel of the xf:upload control is not specified by XForms, nor does it *have* to look like the <input type="file"> cousin, so there's no reason you can't include the file size in the display; you could even property-ize a minimum size for this display to happen. 2. The upload control can populate the instance with filename and mediatype. There's no reason it couldn't also populate a size for use by form authors. You could suggest this to the WG as a reasonable enhancement for a future version of XForms. (If you don't use method='multipart-post' then it's possible that you would run out of memory during the population of the base64 or hexbinary encoding, but that will rapidly teach authors not to do that.)

> (In reply to comment #43) > >The form author has no clue to whether the user > > chooses a 400M core dump file, and tries to upload it. > > > > It's not a must for me to report this to the user, but I think it would be > > useful. At least we should emit some debug messages. What do the rest of you > think? > > I have two suggestions: > 1. The look and feel of the xf:upload control is not specified by XForms, nor > does it *have* to look like the <input type="file"> cousin, so there's no reason > you can't include the file size in the display; you could even property-ize a > minimum size for this display to happen. > > 2. The upload control can populate the instance with filename and mediatype. > There's no reason it couldn't also populate a size for use by form authors. > You could suggest this to the WG as a reasonable enhancement for a future > version of XForms. (If you don't use method='multipart-post' then it's possible > that you would run out of memory during the population of the base64 or > hexbinary encoding, but that will rapidly teach authors not to do that.) > > Could also make the error style-able. Of course, if you do this, you'd want to report the error. If reporting the error, please include the size of the allocation that failed.

Hopefully I covered all of Allan's comments with this patch. I also made it so that if the element is not bound to the corrent datatype, then the control is marked as not-relevant, and the buttons are disabled.

> Created an attachment (id=199993) [edit] Something I forgot: since |refresh()| in xforms.xml can get called quite a bit, I should be caching the refs to the buttons.

(In reply to comment #47) > Created an attachment (id=199993) [edit] > second patch v1.5 > > Hopefully I covered all of Allan's comments with this patch. I also made it so > that if the element is not bound to the corrent datatype, then the control is > marked as not-relevant, and the buttons are disabled. &%¤&%¤, now I get that permission denied stuff again (comment 30) when testing attachment 197983 [details] or attachment 197763 [details]. Why is this happening to me...

Comment on attachment 199993 [details] [diff] [review] second patch v1.5 > Index: nsXFormsUploadElement.cpp > =================================================================== > +nsXFormsUploadElement::Refresh() ... > + if (NS_SUCCEEDED(rv) && nsuri.EqualsLiteral(NS_NAMESPACE_XML_SCHEMA)) { > + NS_NAMED_LITERAL_STRING(anyURIStr, "anyURI"); > + NS_NAMED_LITERAL_STRING(base64BinStr, "base64Binary"); > + NS_NAMED_LITERAL_STRING(hexBinStr, "hexBinary"); Why? You only use them once. > + // If it is not bound to 'anyURI', 'base64Binary', or 'hexBinary', then > + // mark as not relevant. > + if (mType == TYPE_DEFAULT) { > + /// @bug Set via attributes right now. Bug 271720. (XXX) > + mElement->SetAttribute(NS_LITERAL_STRING("disabled"), > + NS_LITERAL_STRING("1")); > + mElement->RemoveAttribute(NS_LITERAL_STRING("enabled")); > + nsXFormsUtils::ReportError(NS_LITERAL_STRING("uploadBoundTypeError"), > + mElement); > + } else { > + /// @bug Set via attributes right now. Bug 271720. (XXX) > + mElement->SetAttribute(NS_LITERAL_STRING("enabled"), > + NS_LITERAL_STRING("1")); > + mElement->RemoveAttribute(NS_LITERAL_STRING("disabled")); > + } > } Some jerk changed this :), so that would be: nsCOMPtr<nsIXTFElementWrapper> xtfWrap(do_QueryInterface(mElement)); NS_ENSURE_STATE(xtfWrap); xtfWrap->SetIntrinsicState(mType == TYPE_DEFAULT ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED); 1) I feel there's something fishy about this setting the event state, even though there's a bound node... but that may just be me, and definately another bug. 2) Hmmm, that's how we (I) do it in ResetBoundNode(), but that's actually wrong. Argh. Bug 313118 created. Do the above and "// XXX bug 313118 should fix this" > +nsresult > +nsXFormsUploadElement::SetFile(nsILocalFile *aFile) > { > - if (!mInput || !mBoundNode || !mModel || > - !nsXFormsUtils::EventHandlingAllowed(aEvent, mElement)) > + if (!mBoundNode || !mModel) > return NS_OK; > - nsAutoString value; > - mInput->GetValue(value); > - > - // store the file as a property on the selected content node. the submission > - // code will read this value. > + nsresult rv; > nsCOMPtr<nsIContent> content = do_QueryInterface(mBoundNode); > NS_ENSURE_STATE(content); > - nsILocalFile *file = nsnull; > - nsCAutoString spec; > - > - // We need to special case an empty value. If the input field is blank, > - // then I assume that the intention is that all bound nodes will become > - // (or stay) empty and that there shouldn't be any file kept in the property. > - if (!value.IsEmpty()) { > - NS_NewLocalFile(value, PR_FALSE, &file); > - NS_ENSURE_STATE(file); > - NS_GetURLSpecFromFile(file, spec); > - content->SetProperty(nsXFormsAtoms::uploadFileProperty, file, > - ReleaseObject); > - } else { > + PRBool dataChanged = PR_FALSE; > + if (!aFile) { > + // clear instance data > content->DeleteProperty(nsXFormsAtoms::uploadFileProperty); > - } > + rv = mModel->SetNodeValue(mBoundNode, EmptyString(), &dataChanged); > + } else { > + // set file into instance data > + > + if (mType == TYPE_ANYURI) { > + // set fully qualified path as value in instance data node > + nsCAutoString spec; > + NS_GetURLSpecFromFile(aFile, spec); > + rv = mModel->SetNodeValue(mBoundNode, NS_ConvertUTF8toUTF16(spec), > + &dataChanged); > + } else if (mType == TYPE_BASE64 || mType == TYPE_HEX) { > + // encode file contents in base64/hex and set into instance data node > + PRUnichar *fileData; > + rv = EncodeFileContents(aFile, mType, &fileData); > + if (NS_SUCCEEDED(rv)) { > + rv = mModel->SetNodeValue(mBoundNode, nsDependentString(fileData), > + &dataChanged); > + nsMemory::Free(fileData); > + } > + } > + // XXX need to handle derived types > + > + // Set nsIFile object as property on instance data node, so submission > + // code can use it. > + if (NS_SUCCEEDED(rv)) { If (aFile) and the type is not one of the three known ones, rv is undefined here. > + nsIFile *fileCopy = nsnull; > + rv = aFile->Clone(&fileCopy); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = content->SetProperty(nsXFormsAtoms::uploadFileProperty, fileCopy, > + ReleaseObject); > + } > + } > + NS_ENSURE_SUCCESS(rv, rv); > Index: nsXFormsUtils.cpp > =================================================================== > @@ -791,16 +790,50 @@ nsXFormsUtils::GetSingleNodeBindingValue > - nsXFormsUtils::GetNodeValue(singleNode, aValue); > + singleNode.swap(*aNode); > + if (aModel) > + model.swap(*aModel); could you add "// transfers ref" to the swap lines? > Index: resources/content/xforms.xml > =================================================================== > + <method name="refresh"> > + <body> > + // disable buttons if control is not relevant > + var browseButton = > + document.getAnonymousElementByAttribute(this, "anonid", > + "upload_browse_button"); > + var clearButton = > + document.getAnonymousElementByAttribute(this, "anonid", > + "upload_clear_button"); cache them, as you suggest in comment 48. > + if (this.delegate.isEnabled) { > + browseButton.disabled = false; > + clearButton.disabled = false; > + } else { > + browseButton.disabled = true; > + clearButton.disabled = true; > + } browseButton.disabled = clearButton.disabled = !this.delegate.isEnabled; ... hmmm, which is actually wrong in the case where the type's wrong, because this.delegate.isEnabled returns whether mBoundNode is enabled. You want to know if the upload control is enabled. You might have to use same mechanism as we use to disable the submit button during submission. > Index: resources/locale/en-US/xforms.properties > =================================================================== > +# XForms upload element > +clearButtonText = Clear I wonder, since upload is XBL, can it not use entity references and the dtd? Or does it have to be pure XUL for that? To summarize, I need the nits above and : * to figure out why I get the security error (and run some testcases) * a resolution of the undefined rv * "proper disabling" * an answer to whether we could use entity refs instead With those solved, you have r=me

Comment on attachment 199993 [details] [diff] [review] second patch v1.5 >Index: nsXFormsUploadElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v >retrieving revision 1.10 >diff -u -6 -p -r1.10 nsXFormsUploadElement.cpp >--- nsXFormsUploadElement.cpp 22 Jul 2005 18:47:11 -0000 1.10 >+++ nsXFormsUploadElement.cpp 18 Oct 2005 22:04:17 -0000 > NS_IMETHODIMP >-nsXFormsUploadElement::AttributeRemoved(nsIAtom *aName) >+nsXFormsUploadElement::Refresh() > { >- if (aName == nsXFormsAtoms::accesskey) { >- // accesskey >- mInput->RemoveAttribute(NS_LITERAL_STRING("accesskey")); >+ // This is called when the 'bind', 'ref' or 'model' attribute has changed. >+ // So we need to make sure that the element is still bound to a node of >+ // type 'anyURI', 'base64Binary', or 'hexBinary'. >+ >+ mType = TYPE_DEFAULT; >+ >+ if (mBoundNode) { >+ // get type bound to node >+ nsAutoString type, prefix, nsuri; >+ nsresult rv = nsXFormsUtils::ParseTypeFromNode(mBoundNode, type, prefix); > This is what Allan usually catches me on :=) Don't put all this work in a big 'if' statement if you can help it. Use if (!mBoundNode) { return return nsXFormsDelegateStub::Refresh(); } > NS_IMETHODIMP >-nsXFormsUploadElement::GetInsertionPoint(nsIDOMElement **aPoint) >+nsXFormsUploadElement::PickFile() >-// nsIDOMEventListener >+ // set filter "All Files" >+ // XXX implement "@mediatype" file filters >+ filePicker->AppendFilters(nsIFilePicker::filterAll); Make sure that you open a bug on this and attach a testcase while you are thinking about it. Pausing for now. More review to come...

This patch does "proper disabling" (I think). The only trick is that I want to disable the buttons if mBound == null or if the control is not bound to a node of the right type. I'm also using an entity ref for the "Clear" text. Not sure if you also want me to do the "Browse" text the same way.

> Created an attachment (id=200316) [edit] Ah, crap! Didn't check the console output closely. This patch spits out several warnings that " NS_ENSURE_TRUE(widget) failed", since the first few times that |Refresh()| is called, there is no mElement. I guess I could just remove the " NS_ENSURE_TRUE(widget)" and add "if (widget)" in front of ever ref to widget. But in general, I dislike how I coded that method. I'll have to think on it some more tomorrow. (In reply to comment #30) > ************************************************************ > * Call to xpconnect wrapped JSObject produced this error: * > [Exception... "'Permission denied to get property Object.file' when calling > method: [nsIFilePicker::file]" nsresult: "0x8057001e > (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: > http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" > data: no] > ************************************************************ I have yet to see this error. From what you describe, it seems like you press the "Browse" button, which brings up the file dialog, allowing you to select a file. And when you click OK in the file dialog, you get this error message, correct? It looks to me like something internal to the file picker is producing this error. What platform are you on? Does this happen consistently enough that you could debug it?

(In reply to comment #53) > (In reply to comment #30) > > ************************************************************ > > * Call to xpconnect wrapped JSObject produced this error: * > > [Exception... "'Permission denied to get property Object.file' when calling > > method: [nsIFilePicker::file]" nsresult: "0x8057001e > > (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: > > http://localhost/xftst/Upload/index.php?show=specex.xml :: onclick :: line 1" > > data: no] > > ************************************************************ > > I have yet to see this error. From what you describe, it seems like you press > the "Browse" button, which brings up the file dialog, allowing you to select a > file. And when you click OK in the file dialog, you get this error message, > correct? Yes > It looks to me like something internal to the file picker is producing > this error. What platform are you on? Does this happen consistently enough > that you could debug it? Hmmm, this is the same behaviour as last time. This only happens on my home computer (same Linux as at work...). And it happens all the time at home. This could be a setup/profile problem?

(In reply to comment #52) > Created an attachment (id=200316) [edit] > second patch v1.6 > > This patch does "proper disabling" (I think). The only trick is that I want to > disable the buttons if mBound == null or if the control is not bound to a node > of the right type. I'm also using an entity ref for the "Clear" text. Not > sure if you also want me to do the "Browse" text the same way. Wouldn't that be the better approach? Then all strings are stored in the DTD.

One last go, hopefully. Includes "if (widget)" checks in |Refresh()|, and puts the "Browse..." button text in DTD file (which allows me to get rid of some code).

I'll try to get to the latest patch today. Just a note: I think upload was the only user of nsXFormsControlStubBase::GetReadOnlyState(), and you do not use it now do you? If true, please kill it on check in.

Allan gave me the idea of using CSS to do the disabling of the buttons: if the element is bound to the right type, it uses the "xformswidget-upload" binding which works as expected; otherwise, it binds to "xforswidget-upload-disabled", which does nothing but show the UI, disabled. This patch should also fix Allan's second testcase in this bug. I just needed to check the bound type in |SetFile()|, also. If bug 313313 gets fixed, I can get rid of the |Refresh()| method altogether, and just set the 'relevant' state in XBL code.

Comment on attachment 200378 [details] [diff] [review] second patch v1.8 >Index: nsXFormsUploadElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v >retrieving revision 1.10 >diff -u -6 -p -r1.10 nsXFormsUploadElement.cpp >--- nsXFormsUploadElement.cpp 22 Jul 2005 18:47:11 -0000 1.10 >+++ nsXFormsUploadElement.cpp 21 Oct 2005 20:40:47 -0000 >+ // If it is not bound to 'anyURI', 'base64Binary', or 'hexBinary', then >+ // mark as not relevant. >+ // XXX Bug 313313 - the 'relevant' state should be handled in XBL constructor. >+ nsCOMPtr<nsIXTFElementWrapper> xtfWrap(do_QueryInterface(mElement)); >+ NS_ENSURE_STATE(xtfWrap); >+ if (GetBoundType() == TYPE_DEFAULT) { >+ xtfWrap->SetIntrinsicState(NS_EVENT_STATE_DISABLED); >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("uploadBoundTypeError"), >+ mElement); >+ } else { >+ xtfWrap->SetIntrinsicState(NS_EVENT_STATE_ENABLED); > } > > return NS_OK; > } You shouldn't be setting intrinsic state to disabled, right? But rather adding the disabled state and removing the enabled state? See comment #50. >-// nsIXFormsControl >+nsresult >+nsXFormsUploadElement::HandleChildElements(nsILocalFile *aFile, >+ PRBool *aChanged) >+{ >+ *aChanged = PR_FALSE; >+ Shouldn't you check to see if aChanged exists first? >+ >+static void >+ReportEncodingMemoryError(nsIDOMElement* aElement, nsIFile *aFile) > { >- if (!mInput || !mBoundNode) >- return NS_OK; >+ nsAutoString filename; >+ if (NS_SUCCEEDED(aFile->GetLeafName(filename))) { >+ const PRUnichar *strings[] = { filename.get() }; >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("encodingMemoryError"), >+ strings, 1, aElement, aElement); >+ } >+} Can you report the allocation size that failed, please? Otherwise seems ok to me.

(In reply to comment #59) > You shouldn't be setting intrinsic state to disabled, right? But rather adding > the disabled state and removing the enabled state? See comment #50. As Allan mentions, that will be fixed by Bug 313118. Of course, that whole function in Upload should disappear if Bug 313313 is fixed.

Comment on attachment 200378 [details] [diff] [review] second patch v1.8 right on, r=me

checked in to trunk.

checked in to branch. ->FIXED