Closed Bug 474527 Opened 16 years ago Closed 10 years ago

onbeforeaccept handler in a <prefwindow> can't prevent the window from closing

Categories

(Toolkit :: Preferences, defect)

Product:
Toolkit
Component:
Preferences
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: robbyd, Assigned: manishearth)

References

Details

(Whiteboard: [mentor=MattN][lang=js])

Attachments

(2 files, 4 obsolete files)

updated patch [Backout: Comment 7]
6.97 KB, patch
Details | Diff | Splinter Review
Patch 1.3
7.39 KB, patch
MattN
: feedback+
Details | Diff | Splinter Review 
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.
Product: Firefox → Toolkit
QA Contact: preferences → preferences
Summary: onbeforeaccept handler in custom extension <prefwindow> XUL broken → onbeforeaccept handler in custom extension <prefwindow> can't prevent the window from closing
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Hardware: x86_64 → All
Attached patch patch (obsolete) — — Splinter Review 
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...
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #357911 - Flags: review?(enndeakin)
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.
Attachment #357911 - Flags: review?(enndeakin) → review+
Flags: in-testsuite?
Keywords: checkin-needed
Comment on attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]


http://hg.mozilla.org/mozilla-central/rev/6430d01da9e3
Attachment #359189 - Attachment description: updated patch → updated patch [Checkin: Comment 5]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
This failed unit tests on windows and was backed out:

 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234810582.1234815682.2911.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
}
Attachment #359189 - Attachment description: updated patch [Checkin: Comment 5] → updated patch [Backout: Comment 7]
Attachment #359189 - Attachment is obsolete: true
Thanks Serge/Ben.
Blocks: 388287
Target Milestone: mozilla1.9.2a1 → ---
Attachment #359189 - Attachment is obsolete: false
Summary: onbeforeaccept handler in custom extension <prefwindow> can't prevent the window from closing → onbeforeaccept handler in a <prefwindow> can't prevent the window from closing
Attached patch Patch 1.0 (obsolete) — — Splinter Review 
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.
Attachment #797731 - Flags: review?(mnoorenberghe+bmo)
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>
Attachment #797731 - Attachment description: gavin.patch → Patch 1.0
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.
Attached patch Patch 1.1 (obsolete) — — Splinter Review 
Changed patch author, because the bulk of the changes are Gavin's.
Attachment #797731 - Attachment is obsolete: true
Attachment #797731 - Flags: review?(mnoorenberghe+bmo)
Attachment #802214 - Flags: review?(mnoorenberghe+bmo)
Attached patch Patch 1.2 (obsolete) — — Splinter Review 
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?
Attachment #802214 - Attachment is obsolete: true
Attachment #802214 - Flags: review?(mnoorenberghe+bmo)
Attached patch Patch 1.3 — — Splinter Review 
(qrefreshed on wrong patch stack)
Attachment #804453 - Flags: review?(mnoorenberghe+bmo)
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
Attachment #804453 - Flags: feedback+
Attachment #804434 - Attachment is obsolete: true
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?
Attachment #804453 - Flags: review?(mnoorenberghe+bmo)
(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.
So, should we investigate the test timeouts by resubmitting to the tryserver? Or just go ahead with event.preventDefault ?
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
Assignee: gavin.sharp → manishearth
Status: REOPENED → ASSIGNED
Flags: needinfo?(MattN+bmo)
Whiteboard: [mentor=MattN][lang=js]
(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.
Flags: needinfo?(MattN+bmo)
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.
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.
Try looked good so I pushed it.

https://hg.mozilla.org/integration/fx-team/rev/f331b33c345d
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/f331b33c345d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This likely caused the intermittent orange bug 965534.
Depends on: 965534
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: