Prevent closing WebIDE if any file hasn't been saved

VERIFIED FIXED in Firefox 34

Status

VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: paul, Assigned: bgrins)

Tracking

Trunk
Firefox 34
x86_64
All
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
It's too easy to forget to save a file.
(Reporter)

Updated

4 years ago
Blocks: 1011026
(Assignee)

Comment 1

4 years ago
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #0)
> It's too easy to forget to save a file.

Sounds like we need to have a projecteditor.promptUnsaved() function that:
* returns true if either there are no unsaved changes
* If there are unsaved changes, prompts the user and returns the outcome of the prompt

This will be called at different times than menuEnabled (b/c we wouldn't want to prompt when switching to the preferences page, for instance).  It does get a bit weird if it is not visible when the close happens (like, make some changes, switch to preference page, close WebIDE).  I guess in this case it should switch back to projecteditor then show the prompt, or maybe not show it at all.  Will be left up to webide.js for that.
(Assignee)

Comment 2

4 years ago
Created attachment 8469305 [details] [diff] [review]
confirm-unsaved-WIP.patch

Ryan, this adds a function called projecteditor.confirmUnsaved() that will prompt the user to save their stuff if it is unsaved, then return a bool indicating whether it is OK to quit.

I tried adding a call to this in Cmds.quite in webide.js, but that didn't seem to work when closing the WebIDE window with the X in the corner.  Any ideas how to hook this up with the WebIDE.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8469305 - Flags: feedback?(jryans)
Comment on attachment 8469305 [details] [diff] [review]
confirm-unsaved-WIP.patch

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

Hmm, I wasn't able to get it working either.  I also tried listening for "beforeunload", but my listener wasn't called.

Paul, what's the right way to hook into the closing window process here?
Attachment #8469305 - Flags: feedback?(jryans) → feedback?(paul)
(Reporter)

Comment 4

4 years ago
> <window onclose="return canWeClose();">

if canWeClose returns false, I think it won't close the window.
(Reporter)

Comment 5

4 years ago
Created attachment 8469732 [details] [diff] [review]
onclose addendum
(Reporter)

Comment 6

4 years ago
Comment on attachment 8469305 [details] [diff] [review]
confirm-unsaved-WIP.patch

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

::: browser/locales/en-US/chrome/browser/devtools/projecteditor.properties
@@ +13,5 @@
>  
>  # LOCALIZATION NOTE (projecteditor.deleteLabel):
>  # This string is displayed as a context menu item for allowing the selected
>  # file / folder to be deleted.
> +projecteditor.confirmUnsavedLabel=Are you sure you want to continue?  Your unsaved changes will be lost.

double space. Also, you're not mentioning there actually is unsaved changes.
Attachment #8469305 - Flags: feedback?(paul) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8470035 [details] [diff] [review]
confirm-unsaved.patch

Changed text to:

You have unsaved changes that will be lost if you exit. Are you sure you want to continue?

Happy to modify it further if you have any suggestions.  Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cc91835f765a
Attachment #8469305 - Attachment is obsolete: true
Attachment #8469732 - Attachment is obsolete: true
Attachment #8470035 - Flags: review+
(Reporter)

Comment 8

4 years ago
Works for me. Let's land that.
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 11

4 years ago
Created attachment 8470520 [details] [diff] [review]
confirm-unsaved.patch

Rebased - https://tbpl.mozilla.org/?tree=Try&rev=3aad3c4586d7
Attachment #8470035 - Attachment is obsolete: true
Attachment #8470520 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/19719fd38363
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/19719fd38363
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Unfortunately I had to back this out for intermittent failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45708896&tree=Mozilla-Inbound

17:37:24     INFO -  6340 INFO TEST-START | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js
17:37:24     INFO -  6341 INFO ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
17:37:25     INFO -  6342 INFO dumping last 17 message(s)
17:37:25     INFO -  6343 INFO if you need more context, please use SimpleTest.requestCompleteLog() in your test
17:37:25     INFO -  6344 INFO checking window state
17:37:25     INFO -  6345 INFO Console message: [JavaScript Error: "chrome://browser/content/browser.xul : Unable to run script because scripts are blocked internally."]
17:37:25     INFO -  6346 INFO Building a temporary directory at ProjectEditor1407803844781
17:37:25     INFO -  6347 INFO Adding a project editor tab for editing at: /Users/cltbld/Library/Caches/TemporaryItems/ProjectEditor1407803844781
17:37:25     INFO -  6348 INFO Adding a new tab with URL: 'chrome://browser/content/devtools/projecteditor-test.xul'
17:37:25     INFO -  6349 INFO Console message: [JavaScript Error: "chrome://browser/content/browser.xul : Unable to run script because scripts are blocked internally."]
17:37:25     INFO -  6350 INFO URL 'chrome://browser/content/devtools/projecteditor-test.xul' loading complete
17:37:25     INFO -  6351 INFO must wait for load
17:37:25     INFO -  6352 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | Tab has placeholder iframe for projecteditor - Tab has placeholder iframe for projecteditor
17:37:25     INFO -  6353 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | ProjectEditor has been initialized - ProjectEditor has been initialized
17:37:25     INFO -  6354 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | ProjectEditor has loaded - ProjectEditor has loaded
17:37:25     INFO -  6355 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | A valid resource has been passed in for selection /Users/cltbld/Library/Caches/TemporaryItems/ProjectEditor1407803844781/css/styles.css - A valid resource has been passed in for selection /Users/cltbld/Library/Caches/TemporaryItems/ProjectEditor1407803844781/css/styles.css
17:37:25     INFO -  6356 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | Editor has been activated for /Users/cltbld/Library/Caches/TemporaryItems/ProjectEditor1407803844781/css/styles.css - Editor has been activated for /Users/cltbld/Library/Caches/TemporaryItems/ProjectEditor1407803844781/css/styles.css
17:37:25     INFO -  6357 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | When there are no unsaved changes, confirmUnsaved() is true - When there are no unsaved changes, confirmUnsaved() is true
17:37:25     INFO -  6358 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | When an editor has changed but is still the original text, confirmUnsaved() is true - When an editor has changed but is still the original text, confirmUnsaved() is true
17:37:25     INFO -  6359 INFO confirm dialog observed as expected, going to click OK
17:37:25     INFO -  6360 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | When there are no unsaved changes, clicking OK makes confirmUnsaved() true - When there are no unsaved changes, clicking OK makes confirmUnsaved() true
17:37:25     INFO -  6361 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | When there are no unsaved changes, clicking cancel makes confirmUnsaved() false
17:37:25     INFO -  Stack trace:
17:37:25     INFO -  chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js:checkConfirm:52
17:37:25     INFO -  chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js:test<:26
17:37:25     INFO -  resource://gre/modules/Task.jsm:TaskImpl_run:314
17:37:25     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Handler.prototype.process:866
17:37:25     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.walkerLoop:745
17:37:25     INFO -  null:null:0 - When there are no unsaved changes, clicking cancel makes confirmUnsaved() false
17:37:25     INFO -  Stack trace:
17:37:25     INFO -  chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js:checkConfirm:52
17:37:25     INFO -  chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js:test<:26
17:37:25     INFO -  resource://gre/modules/Task.jsm:TaskImpl_run:314
17:37:25     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Handler.prototype.process:866
17:37:25     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.walkerLoop:745
17:37:25     INFO -  null:null:0
17:37:25     INFO -  TEST-INFO | expected PASS
17:37:25     INFO -  6362 INFO TEST-OK | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_confirm_unsaved.js | took 1179ms

remote:   https://hg.mozilla.org/integration/fx-team/rev/0e5d6cfe6538
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

4 years ago
Seems like the failure is during the test in which the expected sequence is:

INFO confirm dialog observed as expected, going to click OK
INFO TEST-PASS | When there are unsaved changes, clicking OK makes confirmUnsaved() true - When there are unsaved changes, clicking OK makes confirmUnsaved() true
INFO confirm dialog observed as expected, going to click cancel
INFO TEST-PASS | When there are unsaved changes, clicking cancel makes confirmUnsaved() false - When there are unsaved changes, clicking cancel makes confirmUnsaved() false

But instead it looks like this:

INFO confirm dialog observed as expected, going to click OK
INFO TEST-PASS | When there are unsaved changes, clicking OK makes confirmUnsaved() true - When there are unsaved changes, clicking OK makes confirmUnsaved() true
INFO TEST-UNEXPECTED-FAIL | When there are no unsaved changes, clicking cancel makes confirmUnsaved() false

So basically the dialog is not being observed.  It could be some weirdness with the "common-dialog-loaded" / "tabmodal-dialog-loaded" calls, but that it is returning instead of timing out on the call to confirmUnsaved() makes me wonder if that condition is failing for some reason.  Going to add more logging and push to try.
(Assignee)

Comment 17

4 years ago
Created attachment 8472583 [details] [diff] [review]
confirm-unsaved-test-changes.patch

Have a bunch of tests triggered on this push, and haven't seen the error show up yet: https://tbpl.mozilla.org/?tree=Try&rev=9855b9ac507f
Attachment #8470520 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8472583 - Flags: review+
(Assignee)

Comment 19

4 years ago
I've not seen the error on the ~100 or so dt test runs with the new patch at https://tbpl.mozilla.org/?tree=Try&rev=9855b9ac507f.  I'd say lets land it and see if it comes up again, in which case there will be more logging to help track it down.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/81ec2010b71b
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/81ec2010b71b
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
QA Whiteboard: [qa+]
Using Aurora 34.0a2 (2014-10-08) I can see the confirm exit without saving dialog only when I choose to close the WebIDE window.
If I modify a packaged or template app and then select another app, the modification is not kept and the dialog doesn't appear. I could reproduce this under all platforms on Aurora and Nightly too.

Should this bug cover this too or I have to open a new bug for it? Thanks
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 23

4 years ago
(In reply to Petruta Rasa [QA] [:petruta] from comment #22)
> Using Aurora 34.0a2 (2014-10-08) I can see the confirm exit without saving
> dialog only when I choose to close the WebIDE window.
> If I modify a packaged or template app and then select another app, the
> modification is not kept and the dialog doesn't appear. I could reproduce
> this under all platforms on Aurora and Nightly too.
> 
> Should this bug cover this too or I have to open a new bug for it? Thanks

Could you please file a separate bug for this?  This one is only meant for covering the window close,
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #23)
> (In reply to Petruta Rasa [QA] [:petruta] from comment #22)
> > Using Aurora 34.0a2 (2014-10-08) I can see the confirm exit without saving
> > dialog only when I choose to close the WebIDE window.
> > If I modify a packaged or template app and then select another app, the
> > modification is not kept and the dialog doesn't appear. I could reproduce
> > this under all platforms on Aurora and Nightly too.
> > 
> > Should this bug cover this too or I have to open a new bug for it? Thanks
> 
> Could you please file a separate bug for this?  This one is only meant for
> covering the window close,

I believe the other cases are covered by bug 1073935.
Great, thanks guys.

Marking this as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
QA Contact: petruta.rasa
Summary: Prevent closing WebIDE or switch to a new project if any file hasn't been saved → Prevent closing WebIDE if any file hasn't been saved

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.