Closed Bug 1327784 Opened 5 years ago Closed 4 years ago

When I save JSON files, Firefox doesn't autofills file name into save dialog

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: arni2033, Assigned: Oriol, Mentored, NeedInfo)

References

Details

Attachments

(1 file, 4 obsolete files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url [1] or [2] or [3]
2. Click "Save" button in JSON viewer

AR:  Browser opens native save dialog, but there's no file name in field "File name"
ER:  Browser should autofill file name, just like it always does when I save file

It's already too hard to save JSON files, so I'm forced to click "Save" button instead of Ctrl+S
But now it turns out that "Save" button doesn't work as well, and now I have to close modal save
dialog, copy file name or some part of url (if file doesn't have a name), then [probably make
the same mistake - Ctrl+S instead of "Save"] paste file name in modal save dialog

> [1] http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.map
> [2] https://mail.yandex.ru/neo2/handlers/handlers3.jsx?_h=do-settings,settings
> [3] https://www.dropbox.com/subscribe?host_int=0&trace=%2Fhome&user_id=112673760&nid=0&ns_map=182477297_13678%2C214117373_1205
No longer blocks: 1277113
Component: Untriaged → Developer Tools: JSON Viewer
Hi,

I would like to work on this bug but is it possible to provide some information on where to start.

Thanks,
Ankita
Flags: needinfo?(dakatsuka)
Flags: needinfo?(odvarko)
I talked with Ankita on irc.
I'm not familiar with JSON Viewer.
Please ask to triage owner.
Flags: needinfo?(dakatsuka)
(In reply to Ankita Tanwar from comment #1)
> Hi,
> 
> I would like to work on this bug but is it possible to provide some
> information on where to start.
Sounds great!

1) Here is the place where onSave handler is implemented. It's executed when the user clicks on the 'Save' button. 
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/devtools/client/jsonview/main.js#52

2) `getTargetFile` is implemented here:
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/devtools/client/jsonview/utils.js#25

It open and initializes the 'Save as' dialog. It should also be responsible for setting the file name.

See this MDN page:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFilePicker

It should be possible to set the file name through `defaultString` attribute.

3) Also, I noticed that `show()` is deprecated and we should probably use `open()` instead.

Should I assign the bug to you?

Honza
Flags: needinfo?(odvarko) → needinfo?(ankita_tanwar)
Hello Jan.

You can assign this bug to me. I would love to work on it.
Ah, forgot to do that, done now!

Honza
Assignee: nobody → ankita_tanwar
Flags: needinfo?(ankita_tanwar)
Hi Jan,
 I talked to Ankita, we are working on this together. I put console.log() in main.js and utils.js on the lines you have mentioned. It does not get displayed in console. The file json-viewer.js seems to be involved. onSaveJson() is called from there. This calls window.dispatchEvent() with custom event. I couldn't find which file contains the definition of this method. The comment in file says "It is handled in chrome scope". What does this mean? Is there a systematic way to see call hierarchy of javascript files? 
-Thanks in advance.
Flags: needinfo?(odvarko)
(In reply to Saghan from comment #6)
> Hi Jan,
>  I talked to Ankita, we are working on this together.
We were discussing similar issues on IRC today with Anktita so, all questions might be answered now.

Honza
Flags: needinfo?(odvarko)
Attached patch Bug_1327784_1.patch (obsolete) — Splinter Review
Assigning the document title to "defaultString" parameter of "nsIFilePicker" solves the problem.
Comment on attachment 8848282 [details] [diff] [review]
Bug_1327784_1.patch

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

I was testing the patch on my machine and when saving JSON
from this url: https://api.github.com/ the default file name is:
https---api.github.com- - Nightly

When using `File -> Save Page As...` the default file name is: 
https _api.github.com_.htm


Could we make it the same?

You can search the code base and find how Firefox Save As dialog does it.
See http://searchfox.org/

Thanks!

Honza
Attachment #8848282 - Flags: review-
Any progress on this bug?
It would be so great to have this landed!

Honza
Flags: needinfo?(ankita_tanwar)
Hi Honza,

Apologies for the late response.
Lately, I haven't been able to work on this bug due to workload. I will start working on it again and submit a fix asap.
Flags: needinfo?(ankita_tanwar)
(In reply to Ankita Tanwar from comment #11)
> Hi Honza,
> 
> Apologies for the late response.
> Lately, I haven't been able to work on this bug due to workload. I will
> start working on it again and submit a fix asap.
Thanks for the update!

Thanks for working on this, I am looking forward
to check out the patch.
Honza
Instead of producing a custom save dialog, which has various problems:
 - Name not autofilled
 - Save location is globaly remembered, not by domain
 - Downloaded file does not appear in Firefox's Downloads

maybe the save button should just trigger native save (bug 1349437)?

I think it would be something like

    XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
      let ContentAreaUtils = {};
      Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", ContentAreaUtils);
      return ContentAreaUtils;
    });
    // ...
    ContentAreaUtils.saveBrowser(gBrowser.selectedBrowser);

Bug 1349437 comment #12 would need to be fixed but I did not reproduce that.
(In reply to Oriol from comment #14)
> Instead of producing a custom save dialog, which has various problems:
>  - Name not autofilled
>  - Save location is globaly remembered, not by domain
>  - Downloaded file does not appear in Firefox's Downloads
> 
> maybe the save button should just trigger native save (bug 1349437)?
> 
> I think it would be something like
> 
>     XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
>       let ContentAreaUtils = {};
>      
> Services.scriptloader.loadSubScript("chrome://global/content/
> contentAreaUtils.js", ContentAreaUtils);
>       return ContentAreaUtils;
>     });
>     // ...
>     ContentAreaUtils.saveBrowser(gBrowser.selectedBrowser);
> 
> Bug 1349437 comment #12 would need to be fixed but I did not reproduce that.

Sounds good to me!

Honza
Attached patch json-native-save.patch (obsolete) — Splinter Review
I meant something like this. The problem is that it always saves the original JSON, never the prettified one.

Ankita, Jan, any idea? I thought about opening a new tab, load the desired contents in there, save that tab, and close it before the dialog appears (hopefully it will be asynchroneous). But seems too hacky.
Attached patch json-native-save-v2.patch (obsolete) — Splinter Review
Ankita, I hope you don't mind if I steal this bug :)

Handling prettified JSON was a bit messy. I managed to do it by storing the data in a blob url and downloading that. The blob is created in the content process so that when you close the tab with the json, the url is revoked and the blob can be garbage collected. The filename is obtained from the document, not the blob.
Assignee: ankita_tanwar → oriol-bugzilla
Attachment #8848282 - Attachment is obsolete: true
Attachment #8864715 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8865309 - Flags: review?(odvarko)
Comment on attachment 8865309 [details] [diff] [review]
json-native-save-v2.patch

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

Thanks for the patch!

Please see my inline comments

Honza

::: devtools/client/jsonview/main.js
@@ +65,5 @@
> +      // - Uses the given blob URL containing the custom contents to save.
> +      // - Obtains the file name from the URL of the document, not the blob.
> +      let persistable = browser.QueryInterface(Ci.nsIFrameLoaderOwner)
> +        .frameLoader.QueryInterface(Ci.nsIWebBrowserPersistable);
> +      let stack = components.stack.caller;

Please move the 'stack' variable into `onError` handler. Also the, error handling feels a bit hacky. Could we just use: new Error() ?

@@ +69,5 @@
> +      let stack = components.stack.caller;
> +      persistable.startPersistence(message.data.windowID, {
> +        onDocumentReady(doc) {
> +          let uri = chrome.makeURI(doc.documentURI, doc.characterSet);
> +          let filename = chrome.getDefaultFileName(undefined, uri, doc, null);

When I switch off the JSON View and press Ctrl+S (Win10) to save the page I am seeing that the default file name is 'api.github.com'.

In case of the JSON Viewer enabled, I am seeing: 'https _api.github.com_'. Why the difference?
Attachment #8865309 - Flags: review?(odvarko)
@Ankita: what do you think about the patch?

Honza
Flags: needinfo?(ankita_tanwar)
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> Please move the 'stack' variable into `onError` handler. Also the, error
> handling feels a bit hacky. Could we just use: new Error() ?

I just copied the code from saveBrowser. I also think Error makes more sense.
Maybe Error is not defined in that scope, I will check later, but chrome.Error should work.

> When I switch off the JSON View and press Ctrl+S (Win10) to save the page I
> am seeing that the default file name is 'api.github.com'.
> 
> In case of the JSON Viewer enabled, I am seeing: 'https _api.github.com_'.
> Why the difference?

That's because bug 1362268 hadn't landed yet :)
Attached patch json-native-save-v3.patch (obsolete) — Splinter Review
Removed stack and used Error.
Attachment #8865477 - Flags: review?(odvarko)
Comment on attachment 8865477 [details] [diff] [review]
json-native-save-v3.patch

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

Looks great, thanks for working on this!

R+

Honza
Attachment #8865477 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Attachment #8865309 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd31968cb8d7
Let JSON Viewer save using standard saving methods from contentAreaUtils.js. r=honza
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c30e21ceb7
Backed out changeset dd31968cb8d7 for eslint failure
Replacing Blob with window.Blob
Damn linter...
Flags: needinfo?(oriol-bugzilla)
Attachment #8866417 - Flags: review?(odvarko)
Attachment #8865477 - Attachment is obsolete: true
Comment on attachment 8866417 [details] [diff] [review]
json-native-save-v4.patch

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

Yep, fixes the eslint error for me.

Thanks,
Honza
Attachment #8866417 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c43489fc05f
Let JSON Viewer save using standard saving methods from contentAreaUtils.js. r=honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9c43489fc05f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have successfully reproduced the bug in firefox Nightly 53.0a1 (2017-01-01) (32-bit) with windows 10 (32 bit) 

Verified as fixed with latest nightly 55.0a1 (2017-05-31) (32-bit)

Build ID:   20170531030204
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [Bugday-20170531]
I have reproduced this bug with nightly 53.0a1 (2017-01-01) (64-bit) on "Linux Mint 18.1 Serena"(64 Bit).

The bug's fix is now verified on Latest nightly 55.0a1.

Build ID 	20170530100155
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[Bugday-20170531]
As per Comment 30 & Comment 31, I am marking this bug as verified fixed
Status: RESOLVED → VERIFIED
Depends on: 1369159
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.