851217 - xul error when clicking on link that results in download of file from webapp

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

screenshot attached.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Hmm, looks like unknownContentType.xul didn't load unknownContentType.dtd, which is strange, since both should be packaged with Toolkit files, which should all be available to the desktop webapp runtime.  They are, however, packaged into different JAR archives using different manifests (toolkit/mozapps/downloads/jar.mn and toolkit/locales/jar.mn, respectively), so perhaps the localization archive isn't being loaded for some reason.

glandium did some runtime localization work over in bug 762864.  I wonder if he would know why this is happening.

Comment 2

[Dietrich Ayala (:dietrich)]

Yes, the code there looks fine - correct URL, etc. I can load the chrome URL to the dtd in the browser, and shows the right file.

Comment 3

[Dietrich Ayala (:dietrich)]

Glandium says nope, doesn't know.

Comment 5

[Marco Castelluccio [:marco]]

WARNING: Failed to open external DTD: publicId "" systemId "chrome://branding/locale/brand.dtd" base "chrome://mozapps/content/downloads/unknownContentType.xul" URL "chrome://branding/locale/brand.dtd"

So it's brand.dtd (and brand.properties) that aren't packaged for the runtime.

Comment 6

[Mike Hommey [:glandium]]

None of the entities declared in brand.dtd are used in unknownContentType.xul. Sounds like you can just remove the reference.

Comment 7

[Mike Hommey [:glandium]]

While doing this, it might be worth checking other files under toolkit/ that include brand.dtd (there are a bunch of them) to see if they actually use it.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 786733 [details] [diff] [review]
Patch

I'm not a toolkit peer.

That being said, the settingsChangePreferences and settingsChangeOptions labels are interesting, in that they may not match anything real outside Firefox (and in fact, don't in webapps). It seems to me this part of the UI should be browser-specific instead of being in toolkit.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 787208 [details] [diff] [review]
Patch v2

Review of attachment 787208 [details] [diff] [review]:
-----------------------------------------------------------------

You seem to be missing part of this patch (settingsChange.properties).

::: toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY  intro2.label                "You have chosen to open:">
>  <!ENTITY  from.label                  "from:">
> +<!ENTITY  actionQuestion.label2       "What would you like to do with this file?">

Can you get someone from UX to agree to this change?

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +474,5 @@
>        openHandler.parentNode.removeChild(openHandler);
>        var openHandlerBox = this.dialogElement("openHandlerBox");
>        openHandlerBox.appendChild(openHandler);
> +
> +      if (Services.appinfo.ID != "[e-mail]") {

I'd rather not hardcode here. Does attempting to load the bundle throw an exception?

Comment 12

[Andy Poole]

I hope I not breaking any conventions by posting this, but it may help, it certainly now allows me to get the expected behaviour until a fix is published. 

Taken from: https://developer.mozilla.org/en-US/docs/XULRunner_tips#Using_Firefox_to_run_XULRunner_applications

Whilst the error code stated below is different, with those preferences added the expected action occurs when clicking file download links from a webmail webapp.

To use the unknown-content-type and file-downloads dialogs from a <browser> element, you need to add the following prefs:

pref("browser.download.useDownloadDir", true);
pref("browser.download.folderList", 0);
pref("browser.download.manager.showAlertOnComplete", true);
pref("browser.download.manager.showAlertInterval", 2000);
pref("browser.download.manager.retention", 2);
pref("browser.download.manager.showWhenStarting", true);
pref("browser.download.manager.useWindow", true);
pref("browser.download.manager.closeWhenDone", true);
pref("browser.download.manager.openDelay", 0);
pref("browser.download.manager.focusWhenStarting", false);
pref("browser.download.manager.flashCount", 2);
//
pref("alerts.slideIncrement", 1);
pref("alerts.slideIncrementTime", 10);
pref("alerts.totalOpenTime", 4000);
pref("alerts.height", 50);

If you are missing preferences that a dialog requires, you will get the following errors:

Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]

Error: dialog has no properties
Source File: chrome://mozapps/content/downloads/u...ontentType.xul
Line: 1

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Andy Poole from comment #12)
> Whilst the error code stated below is different, with those preferences
> added the expected action occurs when clicking file download links from a
> webmail webapp.

The problem here was in the download dialog, I think with those preferences you're bypassing it and you're directly downloading the files in the default downloads directory.
I don't know if this is the expected behavior in an app, because while in Firefox users can change settings, in the Webapp Runtime they can't (so they'll be forced to always use the downloads directory).

Comment 14

[creations]

Any update please.

Kind Regards, Aarif

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch v3

This creates a webapp specific branding. We'll later decide where we want Firefox's branding.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 814240 [details] [diff] [review]
Patch v3

Review of attachment 814240 [details] [diff] [review]:
-----------------------------------------------------------------

::: webapprt/Startup.jsm
@@ +69,5 @@
> +vendorShortName=' + developer;
> +
> +  yield writeFile(OS.Path.join(PROFILE_DIR, "brand.properties"),
> +                  brandPropertiesContent);
> +}

Everything looks fine here but I have a comment about these functions. It feels a little better to me to wrap them in a Task.spawn call so they just return a promise, this makes them easier to use from other contexts. If that's not the norm for webapprt code then that's fine I'm not set on it. I have started a thread on dev-platform to discuss this pattern.

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Dave Townsend (:Mossop) from comment #16)
> Comment on attachment 814240 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 814240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: webapprt/Startup.jsm
> @@ +69,5 @@
> > +vendorShortName=' + developer;
> > +
> > +  yield writeFile(OS.Path.join(PROFILE_DIR, "brand.properties"),
> > +                  brandPropertiesContent);
> > +}
> 
> Everything looks fine here but I have a comment about these functions. It
> feels a little better to me to wrap them in a Task.spawn call so they just
> return a promise, this makes them easier to use from other contexts. If
> that's not the norm for webapprt code then that's fine I'm not set on it. I
> have started a thread on dev-platform to discuss this pattern.

I usually prefer this style, because it makes the code a little more readable (it avoids indentation).
It also makes the function impossible to use outside of a Task, so people in future will not be able to just call it and forget that it's asynchronous.
Anyway I'm fine with changing it until we decide in dev-platform what to do in general.

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Carrying r+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d8109ff79880

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8109ff79880

Comment 21

[creations]

Hello,

Does this mean that this fix will be work on FireFox 27. Currently we are using FireFox 24 - it isstill giving the same error.

XML Parsing Error: undefined entity
Location: chrome://mozapps/content/downloads/unknownContentType.xul
Line Number 30, Column 18:    <description>&intro2.label;</description>
-----------------^

Please confirm.

Kind Regards, Aarif

Comment 22

[creations]

Hello Ryan,

Sorry, I can't undersatnd your reply. Can you please clarify - if the fix should work in FireFox 24 or it will be released with FireFox 27.

Kind Regards, Aarif

Comment 23

[Marco Castelluccio [:marco]]

It will be released in Firefox 27.

Comment 24

[vkeranov]

Well, I was the author of the dublicate bug (see here: https://bugzilla.mozilla.org/show_bug.cgi?id=901205) and for me there is a new problem now.
With Firefox 27 there is a dialog box, as expected. But every downloaded file is empty. I've tested this with some .pdf and .csv files. Adobe Reader (11.0.6) says the .pdf file is broken. LibreOffice (4.2.0) opens the .csv file, but there is nothing in it (empty spreadsheet). It is the same with Microsoft Office 2013.
Please note: The same files, downloaded in normal browser (Firefox 27.0) work just fine. I can open and view all of them (.pdf's and .csv's).

Comment 25

[vkeranov]

Actually this new bug is described here:
https://bugzilla.mozilla.org/show_bug.cgi?id=965414