Closed
Bug 489423
Opened 15 years ago
Closed 15 years ago
Port nsPromptService to Fennec "dialogs"
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec1.0b3+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 1.0b3+ | --- |
People
(Reporter: mfinkle, Assigned: fabrice.desre)
References
Details
Attachments
(3 files, 4 obsolete files)
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.
Reporter | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Updated•15 years ago
|
tracking-fennec: ? → 1.0b3+
Reporter | ||
Comment 1•15 years ago
|
||
Working on a patch
Assignee | ||
Comment 2•15 years ago
|
||
Mark, I have a patch that only lacks select(). It should be ready tomorrow.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Attachment #393189 -
Flags: review?(mark.finkle)
Comment 4•15 years ago
|
||
Fabrice, I think it missed the most important thing : fennecPromptService.js :) There is also a Makefile.in~ in the diff.
Assignee | ||
Comment 5•15 years ago
|
||
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
Attachment #393189 -
Attachment is obsolete: true
Attachment #393488 -
Flags: ui-review?(madhava)
Attachment #393488 -
Flags: review?(mark.finkle)
Attachment #393189 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 6•15 years ago
|
||
Assignee | ||
Comment 7•15 years ago
|
||
Assignee | ||
Comment 8•15 years ago
|
||
- 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
Attachment #393488 -
Attachment is obsolete: true
Attachment #394006 -
Flags: ui-review?(madhava)
Attachment #394006 -
Flags: review?(mark.finkle)
Attachment #393488 -
Flags: ui-review?(madhava)
Attachment #393488 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #394006 -
Attachment is obsolete: true
Attachment #394006 -
Flags: ui-review?(madhava)
Attachment #394006 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•15 years ago
|
Attachment #395569 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 11•15 years ago
|
||
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.
Attachment #395569 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 13•15 years ago
|
||
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.
Attachment #395569 -
Attachment is obsolete: true
Attachment #397618 -
Flags: review?(mark.finkle)
Comment 14•15 years ago
|
||
(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
Reporter | ||
Updated•15 years ago
|
Attachment #397618 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 15•15 years ago
|
||
I noticed that we need to handle nsIPromptService2 as well (bug 514196)
Reporter | ||
Comment 16•15 years ago
|
||
I also missed gavin's comment. We should file a followup bug to clear up the use of document.documentElement
Reporter | ||
Comment 17•15 years ago
|
||
And a followup bug to style the dialogs in the manner chosen by Madhava
Reporter | ||
Comment 18•15 years ago
|
||
pushed: https://hg.mozilla.org/mobile-browser/rev/490fc5d5d055 with stubs for nsIPromptService2
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•15 years ago
|
Assignee: mark.finkle → fabrice.desre
Comment 19•15 years ago
|
||
verified with login prompt for intranet.mozilla.org on an n810 with 1.9.2 nightly 20090921 build
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•