489423 - Port nsPromptService to Fennec "dialogs"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

Working on a patch

Comment 2

[Fabrice Desré]

Mark, I have a patch that only lacks select().

It should be ready tomorrow.

Comment 3

[Fabrice Desré]

Attachment: 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)]

Fabrice, I think it missed the most important thing : fennecPromptService.js :)

There is also a Makefile.in~ in the diff.

Comment 5

[Fabrice Desré]

Attachment: (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é]

Attachment: screenshot

Comment 7

[Fabrice Desré]

Attachment: 2nd screenshot : editing in about:config

Comment 8

[Fabrice Desré]

Attachment: 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é]

Attachment: New version that works with the dialog implementation in bug 511627

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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é <[e-mail]>, 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 13

[Fabrice Desré]

Attachment: 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: [e-mail]]]

(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?)]

I noticed that we need to handle nsIPromptService2 as well (bug 514196)

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

And a followup bug to style the dialogs in the manner chosen by Madhava

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed: https://hg.mozilla.org/mobile-browser/rev/490fc5d5d055

with stubs for nsIPromptService2

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

verified with login prompt for intranet.mozilla.org on an n810 with 1.9.2 nightly 20090921 build