Last Comment Bug 489423 - Port nsPromptService to Fennec "dialogs"
: Port nsPromptService to Fennec "dialogs"
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Fabrice Desré
:
:
Mentors:
: 493562 512104 (view as bug list)
Depends on: 510880 511627
Blocks: 482816
  Show dependency treegraph
 
Reported: 2009-04-21 12:42 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2010-06-11 01:04 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (29.27 KB, patch)
2009-08-07 07:47 PDT, Fabrice Desré
no flags Details | Diff | Splinter Review
(hopefully) complete patch (41.60 KB, patch)
2009-08-10 03:23 PDT, Fabrice Desré
no flags Details | Diff | Splinter Review
screenshot (35.89 KB, image/png)
2009-08-10 08:15 PDT, Fabrice Desré
no flags Details
2nd screenshot : editing in about:config (57.89 KB, image/png)
2009-08-11 02:47 PDT, Fabrice Desré
no flags Details
Improved version (42.59 KB, patch)
2009-08-12 04:10 PDT, Fabrice Desré
no flags Details | Diff | Splinter Review
New version that works with the dialog implementation in bug 511627 (34.22 KB, patch)
2009-08-20 05:52 PDT, Fabrice Desré
mark.finkle: review-
Details | Diff | Splinter Review
fixes from comment #11 (27.94 KB, patch)
2009-08-31 07:12 PDT, Fabrice Desré
mark.finkle: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2009-04-21 12:42:58 PDT
Since we are moving to a non-windowed dialog system in Fennec, we will need to convert the prompt service to use inline XUL <box> dialogs.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2009-08-06 08:54:23 PDT
Working on a patch
Comment 2 Fabrice Desré 2009-08-06 12:21:42 PDT
Mark, I have a patch that only lacks select().

It should be ready tomorrow.
Comment 3 Fabrice Desré 2009-08-07 07:47:41 PDT
Created attachment 393189 [details] [diff] [review]
WIP

WIP patch :

Go to chrome://browser/content/prompt/test.xul to test it.

TODO :
 - support for  button default flags, BUTTON_DELAY_ENABLE, STD_OK_CANCEL_BUTTONS, STD_YES_NO_BUTTONS
 - the <select> element is not working

There some obvious UI issues like the background color of toggles that is white and should be the same as the dialog's background.
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2009-08-07 08:06:34 PDT
Fabrice, I think it missed the most important thing : fennecPromptService.js :)

There is also a Makefile.in~ in the diff.
Comment 5 Fabrice Desré 2009-08-10 03:23:55 PDT
Created attachment 393488 [details] [diff] [review]
(hopefully) complete patch

Patch with the missing file, and updated to work with the recent tiling merge.

Use chrome://browser/content/prompt/test.xul to test.

Known issues :
- select() doesn't work
- fennecDialogs.jsm should be in a modules/ subdir, reachable from resource://app/modules/fennecDialogs.jsm
Comment 6 Fabrice Desré 2009-08-10 08:15:55 PDT
Created attachment 393514 [details]
screenshot
Comment 7 Fabrice Desré 2009-08-11 02:47:15 PDT
Created attachment 393737 [details]
2nd screenshot : editing in about:config
Comment 8 Fabrice Desré 2009-08-12 04:10:30 PDT
Created attachment 394006 [details] [diff] [review]
Improved version

- moved waitForModalDialog() to FennecDialog.waitForClose() since it can be useful for other users of the dialog manager.
- added code to prevent dialogs growing larger than the screen
Comment 9 Fabrice Desré 2009-08-20 05:52:34 PDT
Created attachment 395569 [details] [diff] [review]
New version that works with the dialog implementation in bug 511627
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2009-08-22 17:10:11 PDT
*** Bug 512104 has been marked as a duplicate of this bug. ***
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2009-08-24 14:07:56 PDT
Comment on attachment 395569 [details] [diff] [review]
New version that works with the dialog implementation in bug 511627

>diff -r 30e5022551d0 chrome/content/prompt/alert.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/chrome/content/prompt/alert.xul	Thu Aug 20 14:49:44 2009 +0200
>@@ -0,0 +1,17 @@
>+<?xml version="1.0"?>
>+<!DOCTYPE fdialog SYSTEM "chrome://browser/locale/fennecPrompt.dtd">

prompt.dtd is good enough

>+<dialog xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+      id="prompt-alert-dialog"
>+      onclose="this.PromptHelper.onCloseAlert(this);"
>+      script="chrome://browser/content/prompt/promptHelper.js">

prompt.js is good enough

>+  <label id="prompt-alert-title"/>
>+  <description id="prompt-alert-message"/>

I'd like to see a <scrollbox> around the <description> so we can try to support pannable messages in prompt dialogs.

>+  <hbox id="prompt-alert-checkbox-box" collapsed="true" align="center">
>+    <checkbox class="toggle-dark" id="prompt-alert-checkbox"/>
>+    <label id="prompt-alert-checkbox-msg"/>
>+  </hbox>
>+  <hbox pack="center">
>+    <button class="button-dark" label="&label.ok;"
>+            oncommand="document.getElementById('prompt-alert-dialog').close()"/>

Use "document.documentElement" instead of "document.getElementById('prompt-alert-dialog')"

Make this same change to the other XUL files

>diff -r 30e5022551d0 chrome/content/prompt/confirm.xul

>+      onload="this.PromptHelper.test();">

What is this.PromptHelper.test() ?

>diff -r 30e5022551d0 chrome/content/prompt/promptHelper.js

>+var PromptHelper = {

>+    onCloseAlert: function(dialog) {
>+        if (dialog.arguments) {
>+            dialog.arguments.value = document.getElementById('prompt-alert-checkbox').checked;
 Use " not '

>+    closeConfirm : function(confirm) {
>+        let dialog = document.getElementById("prompt-confirm-dialog");

Use document.documentElement when possible (here and anywhere else)

>+    closePrompt : function(confirm) {
>+        let dialog = document.getElementById("prompt-prompt-dialog");
>+        dialog.arguments.result = confirm;
>+        dialog.close();
>+    },

You should ab able to reuse closeConfirm with the document.documentElement change. Maybe rename it closePrompt ?

>+    closePassword : function(confirm) {
>+        let dialog = document.getElementById("prompt-password-dialog");
>+        dialog.arguments.result = confirm;
>+        dialog.close();
>+    },

Same

>+    closeSelect: function(confirm) {
>+        let dialog = document.getElementById("prompt-select-dialog");
>+        dialog.arguments.result = confirm;
>+        dialog.close();
>+    },

Same

>+}
>\ No newline at end of file

Make sure you have newlines at ends of files

>diff -r 30e5022551d0 chrome/content/prompt/promptPassword.xul

>+        <label value="&editfield0.label;"/>
>+        <textbox id="prompt-password-user"/>
>+      </row>
>+      <row align="center">
>+        <label value="&editfield1.label;"/>
>+        <textbox type="password" id="prompt-password-password"/>

Let's use "&user.label;" and "&password.label;"

>diff -r 30e5022551d0 chrome/content/prompt/select.xul

>+  <select id="prompt-select-list" xmlns="http://www.w3.org/1999/xhtml"
>+          onclick="SelectHelper.show(event.target);"/>

Maybe someday we will find a way to reuse the select dialog

>diff -r 30e5022551d0 chrome/content/prompt/test.xul

Not sure if we want to land the test.xul file with this patch. I'd really want this to be a browser-chrome test file.

>diff -r 30e5022551d0 components/Makefile.in

> EXTRA_COMPONENTS = \

>     helperAppDialog.js \
>+        fennecPromptService.js \

promptService.js is good enough

>diff -r 30e5022551d0 components/fennecPromptService.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/components/fennecPromptService.js	Thu Aug 20 14:49:44 2009 +0200
>@@ -0,0 +1,305 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Fabrice Desré <fabrice.desre@gmail.com>, Original author
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+const Ci = Components.interfaces;
>+const Cc = Components.classes;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+function fennecPromptService() {

PromptService

>+  let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
>+  this.document = wm.getMostRecentWindow("navigator:browser").document;

I don't think we should cache the document. We might actually have more than one window sometime. Make a helper function in the object that returns the document and use it in the other methods.

>+  // add a max-width style to prevent a element to grow larger 
>+  // than the screen width
>+  sizeElement: function(id, percent) {
>+    let elem = this.document.getElementById(id);
>+    let screenW = this.document.getElementById("main-window").getBoundingClientRect().width;
>+    elem.style.maxWidth = screenW * percent / 100 + "px"
>+  },


>+  
>+  alert: function(aParent, aDialogTitle, aText) {

aTitle is good enough. Same in other functions

>+    let dialog = this.openDialog("chrome://browser/content/prompt/alert.xul", null);
>+    this.document.getElementById("prompt-alert-title").value = aDialogTitle;
>+    this.document.getElementById("prompt-alert-message").appendChild(this.document.createTextNode(aText));
>+    this.sizeElement("prompt-alert-message", 80);
>+    //dialog.waitForClose();

Why not modal?

>+  },
>+  
>+  alertCheck: function(aParent, aDialogTitle, aText, aCheckMsg, aCheckState) {
>+    let dialog = this.openDialog("chrome://browser/content/prompt/alert.xul", aCheckState);
>+    this.document.getElementById("prompt-alert-title").value = aDialogTitle;
>+    this.document.getElementById("prompt-alert-message").appendChild(this.document.createTextNode(aText));
>+    this.sizeElement("prompt-alert-message", 80);
>+    this.document.getElementById("prompt-alert-checkbox").checked = aCheckState.value;
>+    this.document.getElementById("prompt-alert-checkbox-msg").value = aCheckMsg;
>+    this.sizeElement("prompt-alert-checkbox-msg", 50);
>+    this.document.getElementById("prompt-alert-checkbox-box").removeAttribute("collapsed");
>+    dialog.waitForClose();
>+  },
>+  
>+  confirm: function(aParent, aDialogTitle, aText) {
>+    var params = new Object();
>+    params.result = false;
>+    let dialog = this.openDialog("chrome://browser/content/prompt/confirm.xul", params);
>+    this.document.getElementById("prompt-confirm-title").value = aDialogTitle;
>+    this.document.getElementById("prompt-confirm-message").appendChild(this.document.createTextNode(aText));
>+    this.sizeElement("prompt-confirm-message", 80);
>+    dialog.waitForClose();
>+    return params.result;
>+  },
>+  
>+  confirmCheck: function(aParent, aDialogTitle, aText, aCheckMsg, aCheckState) {
>+    var params = new Object();
>+    params.result = false;
>+    params.checkbox = aCheckState;
>+    let dialog = this.openDialog("chrome://browser/content/prompt/confirm.xul", params);
>+    this.document.getElementById("prompt-confirm-title").value = aDialogTitle;
>+    this.document.getElementById("prompt-confirm-message").appendChild(this.document.createTextNode(aText));
>+    this.sizeElement("prompt-confirm-message", 80);
>+    this.document.getElementById("prompt-confirm-checkbox").checked = aCheckState.value;
>+    this.document.getElementById("prompt-confirm-checkbox-msg").value = aCheckMsg;
>+    this.sizeElement("prompt-confirm-checkbox-msg", 50);
>+    this.document.getElementById("prompt-confirm-checkbox-box").removeAttribute("collapsed");
>+    dialog.waitForClose();
>+    return params.result;
>+  },
>+  
>+  getLocaleString: function(key) {
>+    let bundleService = Cc["@mozilla.org/intl/stringbundle;1"].getService(Ci.nsIStringBundleService);
>+    let bundle = bundleService.createBundle("chrome://global/locale/commonDialogs.properties");

Now this could be cached in the PromptService. Move this part to the constructor.

>+    return bundle.GetStringFromName(key);

And use this._bundle here

>+  },
>+  
>+  //
>+  // Copied from chrome://global/content/commonDialog.js
>+  //
>+  setLabelForNode: function(aNode, aLabel, aIsLabelFlag)
>+{
 <snip>
>+},

Not sure if I think we care too much about accesskeys, but we can keep this for now.

Just put the "{" on the same line as the "function"


>+  confirmEx: function(aParent, aDialogTitle, aText, aButtonFlags, aButton0Title,
>+            aButton1Title, aButton2Title, aCheckMsg, aCheckState) {

aButton# is good enough since these are labels not titles and we don't have multiple params per button.


>diff -r 30e5022551d0 locales/en-US/chrome/fennecPrompt.dtd
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/locales/en-US/chrome/fennecPrompt.dtd	Thu Aug 20 14:49:44 2009 +0200

prompt.dtd

>+<!ENTITY label.ok "OK">
>+<!ENTITY label.cancel "Cancel">
>\ No newline at end of file

ok.label and cancel.label. Add the newline


Wow, this is a big patch, but I think you are very close. Great job.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2009-08-25 20:50:33 PDT
*** Bug 493562 has been marked as a duplicate of this bug. ***
Comment 13 Fabrice Desré 2009-08-31 07:12:35 PDT
Created attachment 397618 [details] [diff] [review]
fixes from comment #11

Here's what's not fixed as asked in #11 :

> >+  <hbox id="prompt-alert-checkbox-box" collapsed="true" align="center">
> >+    <checkbox class="toggle-dark" id="prompt-alert-checkbox"/>
> >+    <label id="prompt-alert-checkbox-msg"/>
> >+  </hbox>
> >+  <hbox pack="center">
> >+    <button class="button-dark" label="&label.ok;"
> >+            oncommand="document.getElementById('prompt-alert-dialog').close()"/>

> Use "document.documentElement" instead of
> "document.getElementById('prompt-alert-dialog')"

This is not possible since once the dialog is loaded, "document" is the main xul document. To simplify code, we could add a helper method in browser.js : getParentDialog(elem) much like getParentBinding() for XBLs.

> You should ab able to reuse closeConfirm with the document.documentElement
> change. Maybe rename it closePrompt ?

With the previous helper yes.

> >diff -r 30e5022551d0 chrome/content/prompt/test.xul

> Not sure if we want to land the test.xul file with this patch. I'd really want
> this to be a browser-chrome test file.

Sure, let me know where we should put it.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-08-31 07:32:38 PDT
(In reply to comment #13)
> This is not possible since once the dialog is loaded, "document" is the main
> xul document.

Sure, but document.documentElement is the <dialog> (i.e. the root element). Works fine, we use it elsewhere:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/filepicker/content/filepicker.js#451
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-02 09:11:26 PDT
I noticed that we need to handle nsIPromptService2 as well (bug 514196)
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-02 09:14:10 PDT
I also missed gavin's comment. We should file a followup bug to clear up the use of document.documentElement
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-02 09:16:29 PDT
And a followup bug to style the dialogs in the manner chosen by Madhava
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-02 09:17:13 PDT
pushed: https://hg.mozilla.org/mobile-browser/rev/490fc5d5d055

with stubs for nsIPromptService2
Comment 19 Joel Maher ( :jmaher) 2009-09-21 08:42:50 PDT
verified with login prompt for intranet.mozilla.org on an n810 with 1.9.2 nightly 20090921 build
Comment 20 Matt Brubeck (:mbrubeck) 2010-06-11 01:04:45 PDT
*** Bug 512104 has been marked as a duplicate of this bug. ***

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