Last Comment Bug 474527 - onbeforeaccept handler in a <prefwindow> can't prevent the window from closing
: onbeforeaccept handler in a <prefwindow> can't prevent the window from closing
Status: RESOLVED FIXED
[mentor=MattN][lang=js]
:
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla29
Assigned To: Manish Goregaokar [:manishearth]
:
Mentors:
Depends on: 965534
Blocks: 388287
  Show dependency treegraph
 
Reported: 2009-01-20 17:37 PST by Robby Dermody
Modified: 2014-02-12 10:45 PST (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.93 KB, patch)
2009-01-20 20:56 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
enndeakin: review+
Details | Diff | Review
updated patch [Backout: Comment 7] (6.97 KB, patch)
2009-01-27 17:47 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
Patch 1.0 (7.37 KB, patch)
2013-08-30 00:59 PDT, Manish Goregaokar [:manishearth]
no flags Details | Diff | Review
Patch 1.1 (7.36 KB, patch)
2013-09-10 04:16 PDT, Manish Goregaokar [:manishearth]
no flags Details | Diff | Review
Patch 1.2 (7.39 KB, patch)
2013-09-13 05:55 PDT, Manish Goregaokar [:manishearth]
no flags Details | Diff | Review
Patch 1.3 (7.39 KB, patch)
2013-09-13 06:33 PDT, Manish Goregaokar [:manishearth]
MattN+bmo: feedback+
Details | Diff | Review

Description Robby Dermody 2009-01-20 17:37:35 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5

I have a custom FF extension I'm working on that has a tab in the FF preferences window. In preferences_overlay.xul I have a tag like:

<prefwindow id="BrowserPreferences" onbeforeaccept="return objPhLinkSettings.validateSettings();">

Then in validateSettings() I check the settings of the text fields to make sure the data entered is valid. If it isn't, I'd like to pop up an alert box stating so, **and allowing the user to correct the error and try again**. Going from the MDC docs, I should be able to just return false from this function. Doing this will cause the prefs not to be saved (good), but will lead to the Preferences window itself still being closed (bad), when it should stay open to allow the user to correct the error and try again.

Returning true from this validateSettings() function works as expected: the Preferences window closes and the prefs are saved.

I'll mark this as normal priority for now because I don't see a workaround to it. Unless there is some other way I could be validating preferences before allowing the prefs window to close...

Reproducible: Always

Steps to Reproduce:
1.
2.
3.


Expected Results:  
return false from the callback should keep the preferences window open and not save the prefs.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-20 20:56:33 PST
Created attachment 357911 [details] [diff] [review]
patch

The current dialogaccept handler does allow beforeaccept handlers to prevent the prefs from being saved, but the window always closes regardless. This patch makes the return value propagate to dialog.xml's _fireButtonEvent, which will also cancel the window close.

I searched http://mxr-test.konigsberg.mozilla.org/addons/ (MXR for addons.mozilla.org) for addons that use this event, and most never return false and just use it to do cleanup or work. The ones that do return false (only 3 of them) look like they were intending it to prevent the dialog from closing, since they use it to validate the pref values and alert() when the validation fails.

Not sure who the best person to review this is, really...
Comment 3 Neil Deakin 2009-01-26 08:24:19 PST
Comment on attachment 357911 [details] [diff] [review]
patch

>+      // try again, this one should succeed
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, true, "window now closed");
>+      is(dialogShown.valueFromPreferences, true, "pref committed");
>+

You might want to check the other values again here just to make sure.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-27 17:47:46 PST
Created attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]
Comment 5 Serge Gautherie (:sgautherie) 2009-02-16 07:38:04 PST
Comment on attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]


http://hg.mozilla.org/mozilla-central/rev/6430d01da9e3
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-16 13:16:52 PST
This failed unit tests on windows and was backed out:

 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234810582.1234815682.2911.gz
Comment 7 Serge Gautherie (:sgautherie) 2009-02-16 15:56:54 PST
Comment on attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]


Backed out:
http://hg.mozilla.org/mozilla-central/rev/dc4f9eebe88b
http://hg.mozilla.org/mozilla-central/rev/14da8a8f1e4d

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234810582.1234815682.2911.gz
WINNT 5.2 mozilla-central unit test on 2009/02/16 10:56:22

*** 1075 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul | Test timed out.
}
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-18 12:34:55 PST
Thanks Serge/Ben.
Comment 9 Manish Goregaokar [:manishearth] 2013-08-30 00:59:10 PDT
Created attachment 797731 [details] [diff] [review]
Patch 1.0

Attempted to fix patch by canceling the event bubble.

I have verified that this works with my patch for bug 388287, however I can't get the tests to run.
Comment 10 Manish Goregaokar [:manishearth] 2013-08-30 01:00:06 PDT
Comment on attachment 797731 [details] [diff] [review]
Patch 1.0

># HG changeset patch
># Parent 44e615a66f3fbe54af80058fdac1814687255820
># User Manish Goregaokar <manishsmail@gmail.com>
>Bug 474527 - onbeforeaccept handler in a <prefwindow> can't prevent the window from closing; r=MattN
>
>diff --git a/toolkit/content/tests/chrome/Makefile.in b/toolkit/content/tests/chrome/Makefile.in
>--- a/toolkit/content/tests/chrome/Makefile.in
>+++ b/toolkit/content/tests/chrome/Makefile.in
>@@ -57,16 +57,18 @@ MOCHITEST_CHROME_FILES = \
> 		test_autocomplete4.xul \
> 		test_autocomplete5.xul \
> 		test_autocomplete_delayOnPaste.xul \
> 		file_autocomplete_with_composition.js \
> 		test_autocomplete_with_composition_on_input.html \
> 		test_autocomplete_with_composition_on_textbox.xul \
> 		test_keys.xul \
> 		window_keys.xul \
>+		test_preferences_beforeaccept.xul \
>+		window_preferences_beforeaccept.xul \
> 		test_showcaret.xul \
> 		window_showcaret.xul \
> 		test_righttoleft.xul \
> 		test_dialogfocus.xul \
> 		dialog_dialogfocus.xul \
> 		test_screenPersistence.xul \
> 		window_screenPosSize.xul \
> 		test_titlebar.xul \
>diff --git a/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul b/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
>@@ -0,0 +1,62 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
>+
>+<window title="Preferences Window Tests"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+  <script type="application/javascript" 
>+          src="chrome://mochikit/content/MochiKit/packed.js"/>
>+  <script type="application/javascript"
>+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
>+
>+  <script type="application/javascript">
>+  <![CDATA[
>+    SimpleTest.waitForExplicitFinish();
>+
>+    // No instant-apply for this test
>+    var prefB = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+    prefB.setBoolPref("browser.preferences.instantApply", false);
>+
>+    var prefWindow = openDialog("window_preferences_beforeaccept.xul", "", "", windowOnload);
>+
>+    function windowOnload() {
>+      var dialogShown = prefWindow.document.getElementById("tests.beforeaccept.dialogShown");
>+      var called = prefWindow.document.getElementById("tests.beforeaccept.called");
>+      is(dialogShown.value, true, "dialog opened, shown pref set");
>+      is(dialogShown.valueFromPreferences, null, "shown pref not committed");
>+      is(called.value, null, "beforeaccept not yet called");
>+      is(called.valueFromPreferences, null, "beforeaccept not yet called, pref not committed");
>+
>+      // try to accept the dialog, should fail the first time
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, false, "window not closed");
>+      is(dialogShown.value, true, "shown pref still set");
>+      is(dialogShown.valueFromPreferences, null, "shown pref still not committed");
>+      is(called.value, true, "beforeaccept called");
>+      is(called.valueFromPreferences, null, "called pref not committed");
>+
>+      // try again, this one should succeed
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, true, "window now closed");
>+      is(dialogShown.valueFromPreferences, true, "shown pref committed");
>+      is(called.valueFromPreferences, true, "called pref committed");
>+
>+      // clean up after ourselves
>+      prefB.clearUserPref("browser.preferences.instantApply");
>+      prefB.clearUserPref("tests.beforeaccept.dialogShown");
>+      prefB.clearUserPref("tests.beforeaccept.called");
>+
>+      SimpleTest.finish();
>+    }
>+  ]]>
>+  </script>
>+
>+  <body xmlns="http://www.w3.org/1999/xhtml">
>+    <p id="display"></p>
>+    <div id="content" style="display: none"></div>
>+    <pre id="test"></pre>
>+  </body>
>+
>+</window>
>diff --git a/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul b/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul
>@@ -0,0 +1,45 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<!--
>+  XUL Widget Test for preferences window
>+-->
>+<prefwindow xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            title="preferences window"
>+            width="300" height="300"
>+            windowtype="test:preferences"
>+            buttons="accept,cancel"
>+            onbeforeaccept="return beforeAccept();"
>+            onload="onDialogLoad();"
>+>
>+  <script type="application/javascript">
>+  <![CDATA[
>+    function onDialogLoad() {
>+      var pref = document.getElementById("tests.beforeaccept.dialogShown");
>+      pref.value = true;
>+
>+      // call the onload handler we were passed
>+      window.arguments[0]();
>+    }
>+
>+    function beforeAccept() {
>+      var beforeAcceptPref = document.getElementById("tests.beforeaccept.called");
>+      var oldValue = beforeAcceptPref.value;
>+      beforeAcceptPref.value = true;
>+
>+      return !!oldValue;
>+    }
>+  ]]>
>+  </script>
>+
>+  <prefpane id="sample_pane" label="Sample Prefpane">
>+    <preferences id="sample_preferences">
>+      <preference id="tests.beforeaccept.called"
>+                  name="tests.beforeaccept.called"
>+                  type="bool"/>
>+      <preference id="tests.beforeaccept.dialogShown"
>+                  name="tests.beforeaccept.dialogShown"
>+                  type="bool"/>
>+    </preferences>
>+  </prefpane>
>+  <label>Test Prefpane</label>
>+</prefwindow>
>diff --git a/toolkit/content/widgets/preferences.xml b/toolkit/content/widgets/preferences.xml
>--- a/toolkit/content/widgets/preferences.xml
>+++ b/toolkit/content/widgets/preferences.xml
>@@ -1026,19 +1026,21 @@
>           return win;
>         ]]>
>         </body>
>       </method>
>     </implementation>
>     <handlers>
>       <handler event="dialogaccept">
>       <![CDATA[
>-        if (!this._fireEvent("beforeaccept", this)) 
>-          return;
>-        
>+        if (!this._fireEvent("beforeaccept", this)){
>+          return false;
>+          event.stopPropagation();
>+          event.preventDefault();        
>+        }
>         if (this.type == "child" && window.opener) {
>           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>                                .getService(Components.interfaces.nsIPrefBranch);
>           var instantApply = psvc.getBoolPref("browser.preferences.instantApply");
>           if (instantApply) {
>             var panes = this.preferencePanes;
>             for (var i = 0; i < panes.length; ++i)
>               panes[i].writePreferences(true);
>@@ -1091,16 +1093,18 @@
>           var panes = this.preferencePanes;
>           for (var i = 0; i < panes.length; ++i)
>             panes[i].writePreferences(false);
> 
>           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>                                .getService(Components.interfaces.nsIPrefService);
>           psvc.savePrefFile(null);
>         }
>+
>+        return true;
>       ]]>
>       </handler>
>       <handler event="command">
>         if (event.originalTarget.hasAttribute("pane")) {
>           var pane = document.getElementById(event.originalTarget.getAttribute("pane"));
>           this.showPane(pane);
>         }
>       </handler>
Comment 11 Manish Goregaokar [:manishearth] 2013-08-30 07:33:23 PDT
Test passed (I ran ./mach mochitest-chrome toolkit/content/tests/chrome/test_preferences_beforeaccept.xul)


 0:35.30 15 INFO TEST-START | Shutdown
 0:35.30 16 INFO Passed:  12
 0:35.30 17 INFO Failed:  0
 0:35.30 18 INFO Todo:    0
 0:35.30 19 INFO Slowest: 1873ms - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
 0:35.30 20 INFO SimpleTest FINISHED
 0:35.73 21 INFO TEST-INFO | Ran 1 Loops
 0:35.73 22 INFO SimpleTest FINISHED
 0:37.06 INFO | automation.py | Application ran for: 0:00:27.808761
 0:37.06 INFO | zombiecheck | Reading PID log: /tmp/tmphJCttXpidlog
 0:37.10 WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
Mochitest INFO | runtests.py | Running tests: end.
Comment 12 Manish Goregaokar [:manishearth] 2013-09-10 04:16:24 PDT
Created attachment 802214 [details] [diff] [review]
Patch 1.1

Changed patch author, because the bulk of the changes are Gavin's.
Comment 13 Manish Goregaokar [:manishearth] 2013-09-13 05:55:00 PDT
Created attachment 804434 [details] [diff] [review]
Patch 1.2

Whoops, had mixed up some of the code.


Interestingly, the tests work fine on my computer with or without any part/all of the propagation prevention code.

Is there a way to test this on WINNT?
Comment 14 Manish Goregaokar [:manishearth] 2013-09-13 06:33:23 PDT
Created attachment 804453 [details] [diff] [review]
Patch 1.3

(qrefreshed on wrong patch stack)
Comment 15 Matthew N. [:MattN] (behind on reviews) 2013-09-23 07:32:13 PDT
Comment on attachment 804453 [details] [diff] [review]
Patch 1.3

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

(In reply to Manish Goregaokar [:manishearth] from comment #13)
> Created attachment 804434 [details] [diff] [review]

There is an "x" at the end of Gavin's name in the user header of the patch.

> Interestingly, the tests work fine on my computer with or without any
> part/all of the propagation prevention code.
>
> Is there a way to test this on WINNT?

Which operating system are you testing on? Do you not have access to Windows? I have pushed this to try server[1] to see if it passes on Windows with the three combinations:
event.stopPropagation - https://tbpl.mozilla.org/?tree=Try&rev=a36361432c9d
event.preventDefault  - https://tbpl.mozilla.org/?tree=Try&rev=abeb8588abcc
Both                  - https://tbpl.mozilla.org/?tree=Try&rev=f9810c6f793c

Manish, can you check the results sometime after a few hours? I am on vacation this week so won't have time to watch the results.

::: toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
@@ +4,5 @@
> +
> +<window title="Preferences Window Tests"
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <script type="application/javascript" 

Nit: trailing whitespace

::: toolkit/content/widgets/preferences.xml
@@ +1032,5 @@
>        <handler event="dialogaccept">
>        <![CDATA[
> +        if (!this._fireEvent("beforeaccept", this)){
> +          event.stopPropagation();
> +          event.preventDefault();        

Nit: trailing whitespace
Comment 16 Matthew N. [:MattN] (behind on reviews) 2013-10-25 01:44:09 PDT
Comment on attachment 804453 [details] [diff] [review]
Patch 1.3

Manish, were you able to see the try server results when they were still available? Do you have an updated patch?
Comment 17 Manish Goregaokar [:manishearth] 2013-10-25 01:48:31 PDT
(In reply to Matthew N. [:MattN] from comment #16)
> Comment on attachment 804453 [details] [diff] [review]
> Patch 1.3
> 
> Manish, were you able to see the try server results when they were still
> available? Do you have an updated patch?

Oh, I think I forgot to comment about this (Sorry about that). All three seemed to work pretty fine, with some unrelated test timeouts for two of them. I believe the second one had no failures.
Comment 18 Manish Goregaokar [:manishearth] 2013-10-25 02:10:24 PDT
So, should we investigate the test timeouts by resubmitting to the tryserver? Or just go ahead with event.preventDefault ?
Comment 19 Matthew N. [:MattN] (behind on reviews) 2014-01-29 17:32:44 PST
If we don't need to use preventDefault or stopProgation, I don't think we should use them. Here is a try push addressing my review comments with some other test cleanup fixes. If it's green, I'll push it.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a96df034bf76
Comment 20 Manish Goregaokar [:manishearth] 2014-01-29 22:39:43 PST Comment hidden (obsolete)
Comment 21 Matthew N. [:MattN] (behind on reviews) 2014-01-29 22:43:02 PST
(In reply to Manish Goregaokar [:manishearth] from comment #20)
I already starred (*) those two failures since they aren't related to this patch AFAICT.

I will look again in a few hours or tomorrow and push it if there are no failures.
Comment 22 Manish Goregaokar [:manishearth] 2014-01-29 22:51:37 PST
Ah. Was in the process of posting a second comment with a breakdown of the tests. Realized that there were still unfinished tests halfway through it.

Where would the test failures come from, though? Previous patches pushed to try?

IIRC last time we had similar issues, some yellow unrelated failures from XP.
Comment 23 Matthew N. [:MattN] (behind on reviews) 2014-01-29 22:53:55 PST
They are intermittent failures (often caused by tests that have race conditions). When you click on an orange run, you will see suggestions of possibly related intermittent failures and the two from comment 21 match known failures.
Comment 24 Matthew N. [:MattN] (behind on reviews) 2014-01-30 00:43:26 PST
Try looked good so I pushed it.

https://hg.mozilla.org/integration/fx-team/rev/f331b33c345d
Comment 25 Ryan VanderMeulen [:RyanVM] 2014-01-30 13:28:39 PST
https://hg.mozilla.org/mozilla-central/rev/f331b33c345d
Comment 26 Neil Deakin 2014-02-12 10:43:28 PST
This likely caused the intermittent orange bug 965534.

Note You need to log in before you can comment on or make changes to this bug.