Closed Bug 569552 Opened 10 years ago Closed 9 years ago

Fennec: onbeforeunload event is not handled properly

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: p.chwiej, Assigned: vingtetun)

References

()

Details

(Whiteboard: [at-risk] [fennec-softblocker])

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.3a5pre) Gecko/20100601 Namoroka/3.7a5pre Fennec/2.0a1pre

Maemo 5
Gecko/20100601 Namoroka/3.7a5pre Fennec/2.0a1pre


Reproducible: Always

Steps to Reproduce:
1. Open any site
2. Open new tab and load attached TC there
3. Try to close the tab with a TC

Actual Results:  
Tab is closed without any prompt


Expected Results:  
There should be a confirmation dialog displayed which allows the user to choose if he really wants to close the tab or to keep it opened


Such mechanism can be seen for example on docs.google.com while closing tab with edited, unsaved document.
Attached file TC
Whiteboard: mobile-triage
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: mobile-triage
Component: Linux/Maemo → General
OS: Linux → Linux (embedded)
Hardware: Other → ARM
Attached patch Patch (obsolete) — Splinter Review
The patch add a message to call permitUnload() which itself call the onbeforeunload alert. The patch works this way but it makes closeTab a bit more complicated since it is Async and use a timeout to ensure we can close the tab even if the content hang.
Attachment #467419 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (obsolete) — Splinter Review
fix a small error
Attachment #467419 - Attachment is obsolete: true
Attachment #467548 - Flags: review?(mark.finkle)
Attachment #467419 - Flags: review?(mark.finkle)
Summary: onbeforeunload event is not handled properly → Fennec: onbeforeunload event is not handled properly
tracking-fennec: --- → 2.0+
Comment on attachment 467548 [details] [diff] [review]
Patch v0.2

>diff -r c195d2ca37e3 chrome/content/bindings/browser.js

>+let WindowUnloadListener = {
>+  init: function() {
>+    addMessageListener("Browser:CanUnload", this);
>+  },
>+
>+  receiveMessage: function(aMessage) {
>+    let canUnload = docShell.contentViewer.permitUnload();
>+    sendSyncMessage("Browser:CanUnload", { permit: canUnload });

"Browser:CanUnload:Return"  is our convention

>+    // Listen for modal dialog when closing window as the result of permitUnload()
>+    function waitForUserConfirm() {
>+      if (Browser._waitToCloseTimeout) {
>+        clearTimeout(Browser._waitToCloseTimeout);
>+        Browser._waitToCloseTimeout = 0;

I do not like the timeout at all. I'd like to think about ways to avoid it.
Attachment #467548 - Flags: review?(mark.finkle) → review-
Whiteboard: [at-risk]
Whiteboard: [at-risk] → [at-risk] [fennec-softblocker]
OS: Maemo → All
Hardware: ARM → All
Attached patch Patch - without setTimeout (obsolete) — Splinter Review
(In reply to comment #4)
> >+    function waitForUserConfirm() {
> >+      if (Browser._waitToCloseTimeout) {
> >+        clearTimeout(Browser._waitToCloseTimeout);
> >+        Browser._waitToCloseTimeout = 0;
> 
> I do not like the timeout at all. I'd like to think about ways to avoid it.

The timeout could be easily ignored (see this patch), I have first add it to allow tab closing if the content hang (like http://www.w3.org/TR/html5/Overview.html).

But it seems pretty bad to have to wait for the page to load before closing the page, so i won't recommend that.

About adding the "alert" in the content side I still think we gonna have the same problem because we _need_ to wait for a response from the content in many cases.
Comment on attachment 515807 [details] [diff] [review]
Patch - without setTimeout

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

>+let WindowUnloadListener = {
>+  init: function() {
>+    addMessageListener("Browser:CanUnload", this);
>+  },
>+
>+  receiveMessage: function(aMessage) {
>+    let canUnload = docShell.contentViewer.permitUnload();
>+    sendSyncMessage("Browser:CanUnload:Return", { permit: canUnload });
>+  }
>+};
>+
>+WindowUnloadListener.init();

Move this out of binding/browser.js and put it in content.js inside the Content object (no need for a new object)

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+  tryToCloseTab: function tryToCloseTab(aTab) {
>+    let tab = aTab instanceof XULElement ? this.getTabFromChrome(aTab)
>+                                          : aTab;

No wrap needed

>+    if (!tab || !this._getNextTab(tab))
>       return;
> 
>+    tab.browser.messageManager.sendAsyncMessage("Browser:CanUnload", {});
>+  },
>+
>+  closeTab: function closeTab(aTab) {

I think we should keep "closeTab" as the main entry point and create a "_doCloseTab" that is called from the message handler. Make sense?

r-, but really close
Attachment #515807 - Flags: review-
Attached patch Patch v0.3 (obsolete) — Splinter Review
(In reply to comment #6)
> >+    if (!tab || !this._getNextTab(tab))
> >       return;
> > 
> >+    tab.browser.messageManager.sendAsyncMessage("Browser:CanUnload", {});
> >+  },
> >+
> >+  closeTab: function closeTab(aTab) {
> 
> I think we should keep "closeTab" as the main entry point and create a
> "_doCloseTab" that is called from the message handler. Make sense?

I'm not fully sure of what you mean here but I have updated the patch so told me if it was what you said.
Attachment #467548 - Attachment is obsolete: true
Attachment #515807 - Attachment is obsolete: true
Attachment #516243 - Flags: review?(mark.finkle)
Comment on attachment 516243 [details] [diff] [review]
Patch v0.3

Looks good, but I don't like calling the private "_doCloseTab" directly. So I have one additional request: Let's make closeTab take an options object to allow force closing the tab.

closeTab(someTab, { forceClose: true });

if forceClose == true, we skip the sendAsyncMessage and just call _doCloseTab.

I'd like to keep _doCloseTab as hidden as possible.

r+ with that nit
Attachment #516243 - Flags: review?(mark.finkle) → review+
Attachment #516249 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/cd34cafa188a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I just checked this and it is still reproducing on:

Build ID: Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b13pre) Gecko/20110315 Firefox/4.0b13pre Fennec /4.0b6pre
Device: Nokia N900

Checked on Android too and it is not reproducible.
For me, the TC is worksforme, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.1) Gecko/20110317 Firefox/4.0b13pre Fennec/4.0

Andreea, could you perhaps check another time on the N900, using the latest trunk build?
VERIFIED FIXED on:

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.2a1pre) Gecko/20110406 Firefox/4.2a1pre Fennec /4.1a1pre

Device: Motorola Droid 2 (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.