Implement Data Forms (XEP-0004)

This has been discussed on IRC a bit, and today I took a call and asked abdelrhman to try and go ahead with using the browser request approach (like what twitter does for auth). I suggested two possible approaches to retrieving the user input: -> We get access to the window object, so a listener can be added to the form and we can use that to get the input element values. -> In case that doesn't work for some reason, we can set the form's target to an arbitrary URL, and extract the input values from the URL in onLocationChange (the form will need to use GET, obviously). IRC log where I suggested this: http://log.bezut.info/instantbird/160525/#m194

This is initial design for applying first approach in the previous comment. and this is an example of the listener that should be passed to |XMPPDataForms|'s constructor >let listener = { > QueryInterface: XPCOMUtils.generateQI([Ci.nsIFormSubmitObserver]), > notifyInvalidSubmit: function(aForm, aInvalidElements) {}, > notify: function(aForm, aWindow, aActionURI) {} >};

Finished the parsing function, added two test cases (one from XEP and the other from a real server).

Comment on attachment 8765478 [details] [diff] [review] v2 - WIP Review of attachment 8765478 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! Functionality-wise, it appears to be coming along nicely - though I haven't tested it yet. I focused on giving you feedback for the JSM, I haven't looked at the test yet. Whatever I reviewed is f+, but I'm leaving the flag so that I remember to come back and review the test as well. ::: chat/content/browserRequest.js @@ +61,5 @@ > > onLocationChange: function(/*in nsIWebProgress*/ aWebProgress, > /*in nsIRequest*/ aRequest, > /*in nsIURI*/ aLocation) { > + // Some schemes does not have host. Grammar nit: "Some schemes do not have a host." ::: chat/protocols/xmpp/xmpp-dataForms.jsm @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public I don't like the name of this file. The name of the file should supply some information on what functionality the file provides. Since it's in the XMPP folder, it's pretty obvious that it's related to XMPP - so the "xmpp-" prefix is probably not required. How about DataFormHelper.jsm? Feel free to come up with your own ideas and discuss - we can bikeshed on IRC. @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +this.EXPORTED_SYMBOLS = ["XMPPDataForms"]; > + > +var {classes: Cc, interfaces: Ci, utils: Cu} = Components; Please use const here instead of var. @@ +9,5 @@ > +Cu.import("resource:///modules/imServices.jsm"); > +Cu.import("resource:///modules/imXPCOMUtils.jsm"); > +Cu.import("resource:///modules/xmpp-xml.jsm"); > + > +function XMPPDataForms(aPromptText, aAccount, aCallback, aStanza) { Again, this function should have a more informative name - we can discuss it on IRC. Don't forget to change it in EXPORTED_SYMBOLS too. Also, this function is technically an API that the file is exposing - you should write a documentation comment for it which describes what it does, when it is intended to be used, and a description of each parameter. @@ +12,5 @@ > + > +function XMPPDataForms(aPromptText, aAccount, aCallback, aStanza) { > + let url = getDataURI(generateHTML(parseElements(aStanza))); > + let browserRequest = { > + get promptText() { return aPromptText; }, Please put the function body in a new line, like: get promptText() { return aPromptText; }, @@ +16,5 @@ > + get promptText() { return aPromptText; }, > + account: aAccount, > + url: url, > + cancelled: function() { > + Services.obs.removeObserver(this._listener, "invalidformsubmit"); Can |cancelled| be called before we are ever |loaded|? In that case, this._listener will be undefined. It's always best to do a null check in these situations before attempting to blindly remove an observer. You could also check the API and ensure that cancelled will never be called before loaded, but sometimes APIs change ;). @@ +23,5 @@ > + loaded: function(aWindow, aWebProgress) { > + this._listener = { > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIFormSubmitObserver]), > + notifyInvalidSubmit: function(aForm, aInvalidElements) { > + aCallback.notifyInvalidSubmit(aForm, aInvalidElements); Hmm, "notifyInvalidSubmit" and "notify" seem like obscure names to me. I know this is the terminology that nsIFormSubmitObserver uses, but I think we can simplify the names for our consumers. Let's discuss this on IRC. @@ +37,5 @@ > + }; > + Services.obs.notifyObservers(browserRequest, "browser-request", null); > +} > + > +// Returns array of elements. This comment should be more descriptive - what is an "element"? Is it an object? If so, what properties does it have? @@ +40,5 @@ > + > +// Returns array of elements. > +function parseElements(aStanza) { > + if (!aStanza) > + return []; Add a blank line after this to make it more readable. @@ +42,5 @@ > +function parseElements(aStanza) { > + if (!aStanza) > + return []; > + let x = aStanza.getElement(["x"]); > + if (x && x.uri != Stanza.NS.xdata) Hmm, you are doing a null-check on x here. That tells me that x can possibly be null. But, if x is null, we should be returning early, right? :) @@ +46,5 @@ > + if (x && x.uri != Stanza.NS.xdata) > + return []; > + > + let elements = []; > + for (let element of x.children) { First of all, I think it's confusing that you have an array called elements, but you are using the singular word "element" to refer to individual members of |children|. Please use "child" instead of "element". :) Here, you are essentially mapping each member of x.children to an "element", which appears to be an object with properties for type and value, and options and defaults in the case of fields. Try using Array.prototype.map() instead of a for loop - it makes things a little more readable than an if/else if/else block, I think. Feel free to start a discussion on IRC. While I'm at it, in case there are some elements you don't care about (elements which are not "title" or "instructions" or "field", you can use Array.prototype.filter to get rid of the ones you don't want before map'ing. @@ +50,5 @@ > + for (let element of x.children) { > + if (element.localName == "title") > + elements.push({type: "title", value: element.innerText}); > + else if (element.localName == "instructions") > + elements.push({type: "instructions", value: element.innerText}); These two conditions result in the same action - you can probably OR them together. @@ +59,5 @@ > + if (field.type == "hidden") { > + // The field is not shown to the form-submitting entity. > + continue; > + } > + else if (field.type== "fixed") Please put one space on both sides of operators. @@ +65,5 @@ > + else if (field.type == "list-multi" || field.type == "list-single") { > + // Get options. > + let options = element.getElements(["option"]); > + field.options = []; > + for (let option of options) { Another great place to use Array.prototype.map! @@ +72,5 @@ > + field.options.push({label: label, value: optionValue}); > + } > + field.defaults = []; > + let defaults = element.getElements(["value"]); > + for (let defaultValue of defaults) Map could be used here too! @@ +81,5 @@ > + let varName = element.attributes["var"]; > + let label = element.attributes["label"]; > + let required = element.getElement(["required"]); > + let desc = element.getElement(["desc"]); > + if (varName) In JavaScript, it's legal to access undefined properties, and also to assign undefined values. Are the null checks useful? Is anything different with and without the null checks? @@ +96,5 @@ > + } > + return elements; > +} > + > +// Generates HTML page contains a form of supplied aElements. Grammatical nit: Please change this to "Generates a HTML page containing a form with the supplied elements". @@ +97,5 @@ > + return elements; > +} > + > +// Generates HTML page contains a form of supplied aElements. > +// aElements is an array of elements. Please briefly describe what the structure of an individual element, or mention that the reader should look at the comment above parseElements (assuming you write a more descriptive comment there). @@ +99,5 @@ > + > +// Generates HTML page contains a form of supplied aElements. > +// aElements is an array of elements. > +function generateHTML(aElements) { > + const docHeader = "<html><head><meta charset='utf-8'></head><body>"; I think you want a slash before the closing arrow bracket for the meta tag: <meta charset='utf-8'/> Just a side note: if you ever run into a situation where you need to hard-code more complex HTML or something, you can use template strings to put it in multiple lines to make it more readable: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals. Probably not necessary here though. @@ +102,5 @@ > +function generateHTML(aElements) { > + const docHeader = "<html><head><meta charset='utf-8'></head><body>"; > + const docFooter = "</body></html>"; > + > + if (aElements.length == 0) 0 is false-y in JavaScript, you can just do |if (!aElements.length)| @@ +116,5 @@ > + > + return docHeader + content + docFooter; > +} > + > +// Returns data URI for an HTML page that is provided by aString. Hmm, when you say "HTML page" I automatically think of a file or URL. Please change "an HTML page" to "HTML markup" or something similar. Even better, you can just change aString to something like aHTMLMarkup to make it obvious.

> Since it's in the XMPP folder, it's pretty obvious > that it's related to XMPP - so the "xmpp-" prefix > is probably not required. Ignore this, clokep pointed out to me that we prefix files because they all end up in the same folder after build. And anyway, it matches the other files in the folder. I should have checked before commenting ;)

(In reply to Nihanth Subramanya [:nhnt11] from comment #4) > @@ +9,5 @@ > > +Cu.import("resource:///modules/imServices.jsm"); > > +Cu.import("resource:///modules/imXPCOMUtils.jsm"); > > +Cu.import("resource:///modules/xmpp-xml.jsm"); > > + > > +function XMPPDataForms(aPromptText, aAccount, aCallback, aStanza) { > > Again, this function should have a more informative name - we can discuss it > on IRC. Don't forget to change it in EXPORTED_SYMBOLS too. The name is not changed in this patch. (I left it to be discussed on IRC) > @@ +81,5 @@ > > + let varName = element.attributes["var"]; > > + let label = element.attributes["label"]; > > + let required = element.getElement(["required"]); > > + let desc = element.getElement(["desc"]); > > + if (varName) > > In JavaScript, it's legal to access undefined properties, and also to assign > undefined values. Are the null checks useful? Is anything different with and > without the null checks? > Are the null checks useful? Yes, as this will need to modify the tests as I use deepEqual. The dataForms will be used in that way > XMPPDataForms("test", aAccount, aStanza, aResult => { > if (aResult === FORM_CANCELLED) { > // user has cancelled the form. > return; > } > // aResult is a map between element names to array of values. > });

Comment on attachment 8770742 [details] [diff] [review] v1 - support data forms Review of attachment 8770742 [details] [diff] [review]: ----------------------------------------------------------------- How can I test this in a "real" environment? ::: chat/protocols/xmpp/test/test_dataForms.js @@ +130,5 @@ > + ] > + } > +]; > + > +function testParse() { Can we add a test for generating HTML? You can probably use the same data and just add a third field which is html. ::: chat/protocols/xmpp/xmpp-dataForms.jsm @@ +25,5 @@ > +// the mapping is passed to aCallback. > +// If the user has cancelled the form, the constant FORM_CANCELLED will > +// be passed to the call back. > +function XMPPDataForms(aPromptText, aAccount, aStanza, aCallback) { > + let url = getDataURI(generateHTML(parseElements(aStanza))); I don't see much value in getDataURI being a separate function. @@ +36,5 @@ > + cancelled: function() { > + dataFormsFinish(FORM_CANCELLED); > + }, > + loaded: function(aWindow, aWebProgress) { > + this._listener = { Any particular reason this is all inside of the loaded function? I think it could all be outside and just have the addObserver calls in here. @@ +44,5 @@ > + Services.obs.removeObserver(this, "formsubmit"); > + this.window.close(); > + delete this.window; > + }, > + notifyInvalidSubmit: function(aForm, aInvalidElements) { What defines something as an "invalid submit"? @@ +48,5 @@ > + notifyInvalidSubmit: function(aForm, aInvalidElements) { > + if (!aInvalidElements.length) > + return; > + > + let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports); Could be good to show *all* elements with errors? @@ +50,5 @@ > + return; > + > + let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports); > + element.focus(); > + this.window.alert(element.validationMessage); Using alerts is a pretty terrible experience, I think it's common now to like show the error near the field, not sure how hard this would be though! @@ +52,5 @@ > + let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports); > + element.focus(); > + this.window.alert(element.validationMessage); > + }, > + notify: function(aForm, aWindow, aActionURI) { Can you put a comment at the top of it saying what this does? @@ +126,5 @@ > + }; > + > + function dataFormsFinish(aResult) { > + if (aCallback) { > + if (typeof aCallback === "function") We should do this error checking for if aCallback is a function at the top of XMPPDataForms. @@ +132,5 @@ > + else > + throw "aCallback must be a function"; > + } > + > + if (!browserRequest) How can browserRequest be false-y? @@ +134,5 @@ > + } > + > + if (!browserRequest) > + return; > + if ("_listener" in browserRequest) I assume we have this in case it is we called finish before it finished loading? @@ +141,5 @@ > + > + Services.obs.notifyObservers(browserRequest, "browser-request", null); > +} > + > +// Returns array of elements and each element is an object has a type property I find this comment to be extremely confusing. I think it needs some better formatting. It might also help to use lists, etc. instead of trying to fit everything into sentences. @@ +157,5 @@ > +// when it's displayed to the user. > +// boolean, jid-multi, jid-single, text-multi, text-private, text-single. > +function parseElements(aStanza) { > + if (!aStanza) > + return []; Should this just return [] or should it raise an exception? Why would this be called without a stanza? @@ +163,5 @@ > + let x = aStanza.getElement(["x"]); > + if (!x || x.uri != Stanza.NS.xdata) > + return []; > + > + let filteredElements = I'd just call this elements and then reuse the variable. @@ +166,5 @@ > + > + let filteredElements = > + x.children.filter(child => child.attributes["type"] != "hidden"); > + > + let elements = filteredElements.map(child => { You can just return the result of the map call. @@ +173,5 @@ > + else if (child.localName != "field") > + throw `Unhandled element type ${child.type}`; > + > + let field = {}; > + field.type = child.attributes["type"]; Do this as one statement for clarity. let field = { type: child.attributes["type"]; }; @@ +183,5 @@ > + let options = child.getElements(["option"]); > + field.options = options.map(option => { > + let label = option.attributes["label"]; > + let optionValue = option.getElement(["value"]).innerText; > + return {label: label, value: optionValue}; Again, I don't think you need the separate variables here, just put it directly into the object definition. @@ +199,5 @@ > + field.name = name; > + if (label) > + field.label = label; > + if (desc) > + field.description = desc ? desc.innerText : null; You've already checked if desc is truth-y, so this ternary will always return desc.innerText, no? @@ +209,5 @@ > +} > + > +// Generates HTML markup containing a form with the supplied elements. > +// aElements is an array of elements (See comment above parseElements). > +function generateHTML(aElements) { I went back and forth in my head a bit about generateHTML and parseElements being separate functions, but I guess it's nice for testing? @@ +214,5 @@ > + const docHeader = "<html><head><meta charset='utf-8'/></head><body>"; > + const docFooter = "</body></html>"; > + > + if (!aElements.length) > + return docHeader + docFooter; Would we even want to show a form in this case? Won't this break the UI? @@ +227,5 @@ > + content += `<h1>${child.value}</h1><br/>`; > + break; > + case "instructions": > + case "fixed": > + content += `<b>* ${child.value}</b><br/><br/>`; This seems like weird formatting. Why bold? You probably want something more like <p>${child.value}</p>. @@ +231,5 @@ > + content += `<b>* ${child.value}</b><br/><br/>`; > + break; > + case "boolean": > + if (!child.name) > + return; Shouldn't this have been error checked already? @@ +241,5 @@ > + content += '>'; > + content += `<option value="" selected>None</option>`; > + content += `<option value="1">True</option>`; > + content += `<option value="0">False</option>`; > + content += "</select> <br/><br/>"; Wouldn't this make more sense as a <input type="checkbox">? Can this really have three values (not given, True, False)? That's not a boolean... @@ +286,5 @@ > + content += '>'; > + child.options.forEach(option => { > + content += `<option value="${option.value}"`; > + if (child.defaults.includes(option.value)) > + content += " selected"; Should we do error checking that list-single can only have a single item in defaults? Or maybe we just don't care and we can let the browser decide which to show. :P

(In reply to Patrick Cloke [:clokep] from comment #7) > How can I test this in a "real" environment? After approving patch here, I'll submit a patch for in-band registration and this will be the real test. > Can we add a test for generating HTML? You can probably use the same data > and just add a third field which is html. generating HTML is not stable in output because of adding elements like (<br/>), so any change in generating will cause failure in testing even if the change is just for formatting. > @@ +36,5 @@ > > + cancelled: function() { > > + dataFormsFinish(FORM_CANCELLED); > > + }, > > + loaded: function(aWindow, aWebProgress) { > > + this._listener = { > > Any particular reason this is all inside of the loaded function? I think it > could all be outside and just have the addObserver calls in here. Yes, using |aWindow| > What defines something as an "invalid submit"? https://dxr.mozilla.org/mozilla-central/source/dom/html/nsIFormSubmitObserver.idl > @@ +48,5 @@ > > + notifyInvalidSubmit: function(aForm, aInvalidElements) { > > + if (!aInvalidElements.length) > > + return; > > + > > + let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports); > > Could be good to show *all* elements with errors? This line is used to focus on the first invalid element. I added a comment about that in this patch. > @@ +157,5 @@ > > +// when it's displayed to the user. > > +// boolean, jid-multi, jid-single, text-multi, text-private, text-single. > > +function parseElements(aStanza) { > > + if (!aStanza) > > + return []; > > Should this just return [] or should it raise an exception? Why would this > be called without a stanza? raise an exception is better. > @@ +209,5 @@ > > +} > > + > > +// Generates HTML markup containing a form with the supplied elements. > > +// aElements is an array of elements (See comment above parseElements). > > +function generateHTML(aElements) { > > I went back and forth in my head a bit about generateHTML and parseElements > being separate functions, but I guess it's nice for testing? No, it's used because in-band registration which uses data forms will have some elements outside |x| element, so we will use a different parser. > @@ +214,5 @@ > > + const docHeader = "<html><head><meta charset='utf-8'/></head><body>"; > > + const docFooter = "</body></html>"; > > + > > + if (!aElements.length) > > + return docHeader + docFooter; > > Would we even want to show a form in this case? Won't this break the UI? I think throw will be better. > @@ +241,5 @@ > > + content += '>'; > > + content += `<option value="" selected>None</option>`; > > + content += `<option value="1">True</option>`; > > + content += `<option value="0">False</option>`; > > + content += "</select> <br/><br/>"; > > Wouldn't this make more sense as a <input type="checkbox">? Can this really > have three values (not given, True, False)? That's not a boolean... Boolean has different meaning according to XEP: if it's required, the user should select between true or false and we shouldn't assume true or false. if it's not required, the user may not prefer to choose. checkbox will have only two values if the user does not make a change, it will have false value. > @@ +286,5 @@ > > + content += '>'; > > + child.options.forEach(option => { > > + content += `<option value="${option.value}"`; > > + if (child.defaults.includes(option.value)) > > + content += " selected"; > > Should we do error checking that list-single can only have a single item in > defaults? Or maybe we just don't care and we can let the browser decide > which to show. :P No, we can let the browser decide which to show :D

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #8) > Boolean has different meaning according to XEP: > if it's required, the user should select between true or false and we > shouldn't assume true or false. > if it's not required, the user may not prefer to choose. > checkbox will have only two values if the user does not make a change, it > will have false value. Are you sure? 3.3 "The field enables an entity to gather or provide an either-or choice between two options. The default value is "false". [10]"

(In reply to aleth [:aleth] from comment #9) > (In reply to Abdelrhman Ahmed [:abdelrhman] from comment #8) > > Boolean has different meaning according to XEP: > > if it's required, the user should select between true or false and we > > shouldn't assume true or false. > > if it's not required, the user may not prefer to choose. > > checkbox will have only two values if the user does not make a change, it > > will have false value. > > Are you sure? > 3.3 "The field enables an entity to gather or provide an either-or choice > between two options. The default value is "false". [10]" Sorry, I have not seen that before! (seems I got confused because of examples section). You are right.

Added support for CAPTCHAs

Comment on attachment 8784119 [details] [diff] [review] v5 - support data forms Review of attachment 8784119 [details] [diff] [review]: ----------------------------------------------------------------- I'm leaving the r? because I haven't finished reviewing (some feedback is better than no feedback, right?). General comment: please put all the functions in xmpp-dataForms.jsm in a single object. FORM_CANCELLED can be part of that object too. Then you only have to export that one object from the module. Let me know if you have any questions about this. ::: chat/protocols/xmpp/xmpp-dataForms.jsm @@ +4,5 @@ > + > +// This module is impelments XEP-0004: Data Forms to handle, dispaly and get > +// the information which are requested by the service (e.g. In­Band > +// Registration, User Search, etc.) and allow the user to enter these > +// information through UI. There are a few spelling mistakes in this comment, and I think it can be made more clear and concise in general. Here's a suggestion: This module implements XEP-004: Data Forms. It allows the XMPP service to show information to and request input from the user by generating and displaying a UI consisting of basic form elements. This is useful for in-band registration, user search, etc. @@ +24,5 @@ > +// When the user submits the forms (after filling required information), > +// the submitted information are checked and mapped to their values, then > +// the mapping is passed to aCallback. > +// If the user has cancelled the form, the constant FORM_CANCELLED will > +// be passed to the call back. A better way to write this comment is to simply state that a HTML form will be created using the browserRequest API, and the entered data will be passed to the callback when the user submits the form. Then, describe each parameter in separate lines (like a documentation comment). For example: /** * This creates and shows to the user a HTML form using the browserRequest API. * The entered data will be passed to the callback when the user submits the form. * If the form is cancelled, FORM_CANCELLED will be passed. * * aPromptText - This will be shown as the title of the opened form window. * aAccount - blabla * aStanza - The XMPP stanza containing the "jabber:x:data" element. * aCallback - blabla * aParser - blabla */ @@ +28,5 @@ > +// be passed to the call back. > +function XMPPDataForms(aPromptText, aAccount, aStanza, aCallback, > + aParser = parseElements) { > + if (aCallback) { > + if (!(typeof aCallback === "function")) This can be shortened to > if (typeof aCallback !== "function") because the typeof operator works even if the operand is undefined. @@ +32,5 @@ > + if (!(typeof aCallback === "function")) > + throw "aCallback must be a function"; > + } > + > + let parserdElements = aParser(aStanza); s/parserdElements/parsedElements/g Your editor probably has a way to replace all instances :) @@ +60,5 @@ > + this.window.close(); > + delete this.window; > + }, > + > + // Callback is used when user submits a form with invalid values. "Callback" is confusing because we also have aCallback from the arguments. How about "This function" instead? Or just "Used when..." @@ +72,5 @@ > + Components.interfaces.nsISupports); > + element.focus(); > + }, > + > + // Callback is used when user submits a form successfully. Same here. @@ +78,5 @@ > + // Maps element names to array of values. > + let result = new Map(); > + > + // Get all elements that have name attribute as other elements are > + // not submitted (title, instructions, etc.). How about "Get only elements that have a 'name' attribute. Other elements (e.g. title and instructions) aren't submitted"? @@ +90,5 @@ > + if (elm.hasAttribute("jid") && elm.value) { > + let isJid = this.account._parseJID(elm.value) ? true : false; > + if (!isJid) { > + elm.focus(); > + elm.setCustomValidity(`The jid ${elm.value} is not vaild`); Hmm, this string should be localized somewhere. @@ +104,5 @@ > + result.set(elm.name, [elm.value]); > + break; > + case "select-multiple": > + let options = > + Array.prototype.slice.call(elm.querySelectorAll('[selected]'),0) Space after the comma. @@ +116,5 @@ > + // JID element can be empty as it's not required. > + if (elm.hasAttribute("jid") && elm.value) { > + let jids = new Set(elm.value.split(",")); > + let isJidOk = true; > + jids.forEach(jid => { I think your logic inside the forEach is flawed, see below. In any case, you can use Array.prototype.every here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every This will probably fix the logic "automatically" too. @@ +121,5 @@ > + isJidOk = this.account._parseJID(jid) ? true : false; > + if (!isJidOk) { > + elm.focus(); > + elm.setCustomValidity(`The jid ${elm.value} is not vaild`); > + return; You're returning here, but you're only returning from the callback passed to forEach - not from forEach itself. forEach will continue with the next element with no interruption. @@ +125,5 @@ > + return; > + } > + lines.push(jid); > + }); > + if (!isJidOk) { At this point, you are basically only testing if the last jid was okay. @@ +146,5 @@ > + case "checkbox": > + result.set(elm.name, [elm.checked ? 1 : 0]); > + break; > + default: > + throw `Unhandled element type ${elm.type}`; Again, this should probably be localized. Let's ask aleth/clokep for input as well. @@ +193,5 @@ > +// jid-multi > +// jid-single > +// text-multi > +// text-private > +// text-single I think we need to figure out a better way to write this comment, but we'll save that for the next iteration of the patch. For now, please switch to a multiline comment (/** ... */).

Drive-by comment: In the top area of the data forms dialog, the font sizes/alignment looks a bit wrong.

I tried testing this today from the patches in bug 955317. It successfully prompted me for a password, but I did not see a data form appear. I used jabber.otr.im. What am I doing wrong?

(In reply to Patrick Cloke [:clokep] from comment #19) > I tried testing this today from the patches in bug 955317. It successfully > prompted me for a password, but I did not see a data form appear. I used > jabber.otr.im. What am I doing wrong? The form depends on the server, and not all servers have one. See bug 955317 Comment 31 for some that do. You currently also have to build without --enable-extensions=purple iirc

(In reply to aleth [:aleth] from comment #20) > (In reply to Patrick Cloke [:clokep] from comment #19) > > I tried testing this today from the patches in bug 955317. It successfully > > prompted me for a password, but I did not see a data form appear. I used > > jabber.otr.im. What am I doing wrong? > > The form depends on the server, and not all servers have one. See bug 955317 > Comment 31 for some that do. You currently also have to build without > --enable-extensions=purple iirc I was using the list from bug 955317, comment 25. It explicitly says those support data forms. Is that data wrong?

(In reply to Patrick Cloke [:clokep] from comment #21) > > The form depends on the server, and not all servers have one. See bug 955317 > > Comment 31 for some that do. You currently also have to build without > > --enable-extensions=purple iirc > > I was using the list from bug 955317, comment 25. It explicitly says those > support data forms. Is that data wrong? No, but if the prpl already has all the info it needs to fill in the data form (often it's just a password), it doesn't display it. (Look at the patch in the other bug if you want to see the details.)

(In reply to aleth [:aleth] from comment #22) > No, but if the prpl already has all the info it needs to fill in the data > form (often it's just a password), it doesn't display it. (Look at the patch > in the other bug if you want to see the details.) You may be confusing data forms the XEP (which shows up in the debug log) with the pop up dialog.

Having trouble with jabber.ccc.de. conversations.im is another public server using a data forms captcha.

Comment on attachment 8795705 [details] [diff] [review] v7 - support data forms Review of attachment 8795705 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good overall! If a captcha is wrong on conversations.im, you get an error stanza containing <error xmlns="jabber:client" code="405" type="cancel"> <not-allowed xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/> <text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"> The CAPTCHA verification has failed </text> </error> I wonder if it would make sense to somehow use the text here instead of showing "Unknown error during account registration" in the account manager. That may be for the other bug though. ::: chat/protocols/xmpp/xmpp-dataForms.jsm @@ +33,5 @@ > + * aParser - Optional parameter to pass custom parser instead of the > + * default one. > + */ > + handleForm: function(aPromptText, aAccount, aStanza, aCallback, > + aParser = this.parseElements) { nit: indent @@ +34,5 @@ > + * default one. > + */ > + handleForm: function(aPromptText, aAccount, aStanza, aCallback, > + aParser = this.parseElements) { > + if (aCallback) { Is aCallback optional? @@ +52,5 @@ > + let htmlMarkup = this.generateHTML(parsedElements.elements); > + let url = "data:text/html," + encodeURIComponent(htmlMarkup); > + let browserRequest = { > + get promptText() { > + return aPromptText; This gets displayed in a very small font, which looks a bit strange. Can you tweak browserRequest.css a bit to make it look better? @@ +57,5 @@ > + }, > + account: aAccount, > + url: url, > + cancelled: function() { > + dataFormsFinish(parent.FORM_CANCELLED); Does using an arrow function work here instead of using parent? @@ +77,5 @@ > + > + // Focus on the first invalid element. > + let element = > + aInvalidElements.queryElementAt(0, > + Components.interfaces.nsISupports); nit: shorten to Ci.nsISupports @@ +89,5 @@ > + > + // Get only elements that have a 'name' attribute. Other elements > + // (e.g. title and instructions) aren't submitted. > + let elements = aForm.querySelectorAll('[name]'); > + for (let elm of elements) { I think clokep prefers 'element' to 'elm' ;) @@ +99,5 @@ > + if (elm.hasAttribute("jid") && elm.value) { > + let isJid = this.account._parseJID(elm.value) ? true : false; > + if (!isJid) { > + elm.focus(); > + elm.setCustomValidity(`The jid ${elm.value} is not vaild`); This string should be localized as it is displayed to the user @@ +108,5 @@ > + return; > + } > + } > + > + if (elm.value) Move an if (!elm.value) break; to the top of this case to save if clauses @@ +127,5 @@ > + let jids = new Set(elm.value.split(",")); > + let isOK = jids.every(jid => { > + if (!this.account._parseJID(jid)) { > + elm.focus(); > + elm.setCustomValidity(`The jid ${elm.value} is not vaild`); Same string as above @@ +132,5 @@ > + elm.oninput = function() { > + elm.setCustomValidity(""); > + elm.oninput = null; > + } > + return false; In fact it's the same jid checker function as above. Use a helper function to avoid duplication? @@ +178,5 @@ > + }, > + > + /** > + * Returns an object contains array of elements and showForm boolean to > + * indicate that form should not be shown if there are no required information Returns an object that contains an array of elements and a showForm boolean that indicates when the form should not be shown as there is no required information. @@ +180,5 @@ > + /** > + * Returns an object contains array of elements and showForm boolean to > + * indicate that form should not be shown if there are no required information > + * Each element is an object has a type property that determines other > + * properties in that object. ...an object that has... ...that determines the other properties. @@ +181,5 @@ > + * Returns an object contains array of elements and showForm boolean to > + * indicate that form should not be shown if there are no required information > + * Each element is an object has a type property that determines other > + * properties in that object. > + * Supported types and other properties: ...and their properties: @@ +185,5 @@ > + * Supported types and other properties: > + * title : value (string) > + * instructions : value (string) > + * > + * The following types have properties: ...have the properties: @@ +189,5 @@ > + * The following types have properties: > + * * name (string) > + * * label (string) > + * * required (boolean) > + * * description (string) maybe indent this a bit more (2 spaces?) so it looks less like a double star @@ +238,5 @@ > + if (value) > + field.value = value.innerText; > + field.required = child.getElement(["required"]) ? true : false; > + > + if (field.type == "hidden") field may not have a type property, in which case this will cause a strict warning. Seen on conversations.im @@ +324,5 @@ > + `${child.label}</input><br/><br/>`; > + break; > + case "jid-single": > + case "text-single": > + case "text-private": These seem by default to be not very wide at all (see screenshot), so e.g. the URL is hidden. Add some css or style rule to use more of the window width. @@ +370,5 @@ > + default: > + throw `Unhandled element type ${child.type}`; > + } > + }); > + let btnValue = this.hasRequiredInfo(aElements) ? "Submit" : "OK"; Should be localized if that's the button text. @@ +376,5 @@ > + > + return docHeader + content + docFooter; > + }, > + > + // Checks if the parsed elements has required information or not. // Checks if there are any elements that require information from the user.