From 8b41ee9a80c1d6b3613339e7388035d3fbcc575d Mon Sep 17 00:00:00 2001 From: Francisco Jordano Date: Fri, 14 Jun 2013 16:50:12 +0100 Subject: [PATCH] =?UTF-8?q?Bug=20880624=20-=20[MMS/SMS]=20The=20dialog=20w?= =?UTF-8?q?hen=20tapping=20on=20a=20contact=20into=20=E2=80=98To=E2=80=99?= =?UTF-8?q?=20field=20is=20not=20correct?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- apps/sms/index.html | 5 +- apps/sms/js/dialog.js | 145 +++++++++++++++++++++++++ apps/sms/js/message_manager.js | 22 ++-- apps/sms/js/recipients.js | 55 ++++++++-- apps/sms/js/startup.js | 3 +- apps/sms/js/thread_ui.js | 29 ++--- apps/sms/js/utils.js | 70 ++++++++++++ apps/sms/locales/sms.en-US.properties | 3 +- apps/sms/style/custom_dialog.css | 1 - apps/sms/style/notification.css | 1 + apps/sms/style/sms.css | 14 +++ apps/sms/test/unit/dialog_test.js | 147 ++++++++++++++++++++++++++ apps/sms/test/unit/mock_utils.js | 3 +- apps/sms/test/unit/recipients_test.js | 4 +- apps/sms/test/unit/utils_test.js | 70 ++++++++++++ 15 files changed, 525 insertions(+), 47 deletions(-) create mode 100644 apps/sms/js/dialog.js create mode 100644 apps/sms/test/unit/dialog_test.js diff --git a/apps/sms/index.html b/apps/sms/index.html index 5b65016e6aa0..647077c59527 100644 --- a/apps/sms/index.html +++ b/apps/sms/index.html @@ -33,7 +33,6 @@ @@ -201,7 +200,7 @@

Edit mode

diff --git a/apps/sms/js/dialog.js b/apps/sms/js/dialog.js new file mode 100644 index 000000000000..c573e0a05862 --- /dev/null +++ b/apps/sms/js/dialog.js @@ -0,0 +1,145 @@ +'use strict'; + +/* + Generic confirm screen. Only 'cancel/default' is mandatory. For having l10n + you should define the key value and you have to set l10n to 'true'. Options + should follow the following structure: + + { + title: { + value: 'foo Title', + l10n: false + }, + body: { + value: 'foo Body', + l10n: false + }, + options: { + // Cancel is a mandatory option. You need to define at least the text + cancel: { + text: { + value: 'cancel', + l10n: true + } + }, + // Confirm is an optional one. As in cancel, you could add as well a method + // with params + confirm: { + text: { + value: 'remove', + l10n: true + }, + method: function(params) { + fooFunction(params); + }, + params: [arg1, arg2,....] + } + } +*/ + + +var Dialog = function(params) { + // We need, at least, one cancel option string + if (!params || !params.options || !params.options.cancel) { + return; + } + var _ = navigator.mozL10n.get; + var handlers = new WeakMap(); + // Create the structure + this.form = document.createElement('form'); + this.form.dataset.type = 'confirm'; + this.form.setAttribute('role', 'dialog'); + + // We fill the main info + + // The title should take into account localization as well + var titleDOM = document.createElement('strong'); + var title = (!params.title.l10n) ? params.title.value : _(params.title.value); + titleDOM.textContent = title; + if (params.title.l10n) { + titleDOM.dataset.l10nId = params.title.value; + } + // We make the same for the body + var bodyDOM = document.createElement('small'); + var body = (!params.body.l10n) ? params.body.value : _(params.body.value); + bodyDOM.textContent = body; + if (params.body.l10n) { + bodyDOM.dataset.l10nId = params.body.value; + } + + // Adding this elements to the DOM + var infoSection = document.createElement('section'); + // We create the info container + var infoContainer = document.createElement('p'); + infoContainer.appendChild(titleDOM); + infoContainer.appendChild(bodyDOM); + // We append to the section + infoSection.appendChild(infoContainer); + // At the end we have to append to the form + this.form.appendChild(infoSection); + + // Adding options. In this case we have a maximum of 2, with different styles + // per button + var menu = document.createElement('menu'); + // Default button (Cancel button). It's mandatory + var cancelButton = document.createElement('button'); + var cancelOption = params.options.cancel; + var cancel = !cancelOption.text.l10n ? + cancelOption.text.value : _(cancelOption.text.value); + cancelButton.textContent = cancel; + if (cancelOption.text.l10n) { + cancelButton.dataset.l10nId = cancelOption.text.value; + } + handlers.set(cancelButton, cancelOption); + menu.appendChild(cancelButton); + + if (params.options.confirm) { + var confirmOption = params.options.confirm; + var confirmButton = document.createElement('button'); + var confirm = !confirmOption.text.l10n ? + confirmOption.text.value : _(confirmOption.text.value); + confirmButton.textContent = confirm; + confirmButton.className = 'recommend'; + if (confirmOption.text.l10n) { + confirmButton.dataset.l10nId = confirmOption.text.value; + } + handlers.set(confirmButton, confirmOption); + menu.appendChild(confirmButton); + } else { + // If there is only one item, we take the 100% of the space available + cancelButton.style.width = '100%'; + } + + this.form.addEventListener('submit', function(event) { + event.preventDefault(); + }); + + menu.addEventListener('click', function(event) { + var action = handlers.get(event.target); + + if (!action) { + return; + } + + var method = (action && action.method) || function() {}; + + // Delegate operation to target method. This allows + // for a custom "Cancel" to be provided by calling program + method.apply(null, action.params || []); + + // Hide action menu when click is received + this.hide(); + + }.bind(this)); + // Appending the action menu to the form + this.form.appendChild(menu); +}; + +// We prototype functions to show/hide the UI of action-menu +Dialog.prototype.show = function() { + document.body.appendChild(this.form); +}; + +Dialog.prototype.hide = function() { + document.body.removeChild(this.form); +}; diff --git a/apps/sms/js/message_manager.js b/apps/sms/js/message_manager.js index 12bbedd88a51..50266d248e5b 100644 --- a/apps/sms/js/message_manager.js +++ b/apps/sms/js/message_manager.js @@ -211,15 +211,25 @@ var MessageManager = { return; } - if (activity.number || activity.contact) { - var recipient = activity.contact || { - number: activity.number, - source: 'manual' + // Choose the appropiate contact resolver, if we + // have a contact object, and no number,just use a dummy source, + // and return the contact, if not, if we have a number, use + // one of the functions to get a contact based on a number + var contactSource = Contacts.findByPhoneNumber.bind(Contacts); + var phoneNumber = activity.number; + if (activity.contact && !phoneNumber) { + contactSource = function dummySource(contact, cb) { + cb(activity.contact); }; - - ThreadUI.recipients.add(recipient); + phoneNumber = activity.contact.number || activity.contact.tel[0].value; } + Utils.getContactDisplayInfo(contactSource, phoneNumber, + (function onData(data) { + data.source = 'contacts'; + ThreadUI.recipients.add(data); + }).bind(this)); + // If the message has a body, use it to populate the input field. if (activity.body) { ThreadUI.setMessageBody( diff --git a/apps/sms/js/recipients.js b/apps/sms/js/recipients.js index e2aa43ddf6fa..a0187b17f206 100644 --- a/apps/sms/js/recipients.js +++ b/apps/sms/js/recipients.js @@ -22,6 +22,7 @@ this.email = opts.email || ''; this.editable = opts.editable || 'true'; this.source = opts.source || 'manual'; + this.display = opts.display || ''; } /** * set @@ -704,7 +705,12 @@ // // 1.a Delete Mode // - Recipients.View.prompts.remove(target, function(result) { + var recipientWrapper = { + target: target, + recipient: recipient + }; + Recipients.View.prompts.remove(recipientWrapper, + function(result) { if (result.isConfirmed) { owner.remove( relation.get(target) @@ -896,20 +902,47 @@ }; Recipients.View.prompts = { - remove: function(candidate, callback) { + remove: function(params, callback) { var response = { isConfirmed: false, - recipient: candidate + recipient: params.target }; - var message = navigator.mozL10n.get('recipientRemoval', { - recipient: candidate.textContent.trim() - }); // If it's a contact we should ask to remove - if (confirm(message)) { - response.isConfirmed = true; - } - - callback(response); + var dialog = new Dialog( + { + title: { + value: params.recipient.name || params.recipient.number, + l10n: false + }, + body: { + value: params.recipient.display, + l10n: false + }, + options: { + cancel: { + text: { + value: 'cancel', + l10n: true + } + }, + confirm: { + text: { + value: 'remove', + l10n: true + }, + method: function(callback, response) { + if (response) { + response.isConfirmed = true; + if (callback && typeof callback === 'function') { + callback(response); + } + } + }, + params: [callback, response] + } + } + }); + dialog.show(); } }; diff --git a/apps/sms/js/startup.js b/apps/sms/js/startup.js index c309dccb703c..3af0b326d476 100644 --- a/apps/sms/js/startup.js +++ b/apps/sms/js/startup.js @@ -6,9 +6,9 @@ var lazyLoadFiles = [ 'shared/js/async_storage.js', 'shared/js/l10n_date.js', - 'shared/js/custom_dialog.js', 'shared/js/notification_helper.js', 'shared/js/gesture_detector.js', + 'js/dialog.js', 'js/blacklist.js', 'js/contacts.js', 'js/recipients.js', @@ -34,7 +34,6 @@ var lazyLoadFiles = [ 'shared/style/switches.css', 'shared/style/confirm.css', 'shared/style_unstable/progress_activity.css', - 'style/custom_dialog.css', 'shared/style/action_menu.css', 'shared/style/responsive.css', 'style/notification.css' diff --git a/apps/sms/js/thread_ui.js b/apps/sms/js/thread_ui.js index 3c553ccdaa35..a61872da524d 100644 --- a/apps/sms/js/thread_ui.js +++ b/apps/sms/js/thread_ui.js @@ -440,13 +440,12 @@ var ThreadUI = global.ThreadUI = { }); activity.onsuccess = (function() { - var details = Utils.getContactDetails('', activity.result); - - this.recipients.add({ - name: details.title || details.number || activity.result.name[0], - number: details.number || activity.result.number, - source: 'contacts' - }); + Utils.getContactDisplayInfo(Contacts.findByPhoneNumber.bind(Contacts), + activity.result.number, + (function onData(data) { + data.source = 'contacts'; + this.recipients.add(data); + }).bind(this)); }).bind(this); activity.onerror = (function(e) { @@ -1584,21 +1583,9 @@ var ThreadUI = global.ThreadUI = { continue; } - var number = current.value; - var title = details.title || number; - var type = current.type && current.type.length ? current.type[0] : ''; - var carrier = current.carrier ? (current.carrier + ', ') : ''; - var separator = type || carrier ? ' | ' : ''; var li = document.createElement('li'); - var data = { - name: title, - number: number, - type: type, - carrier: carrier, - separator: separator, - nameHTML: '', - numberHTML: '' - }; + + var data = Utils.getDisplayObject(details.title, current); ['name', 'number'].forEach(function(key) { var escapedData = Utils.escapeHTML(data[key]); diff --git a/apps/sms/js/utils.js b/apps/sms/js/utils.js index 8c26ccb3866e..72d990d129eb 100644 --- a/apps/sms/js/utils.js +++ b/apps/sms/js/utils.js @@ -364,6 +364,76 @@ parsed[$1] = $2; }); return parsed; + }, + /* + Using a contact resolver, a function that can looks for contacts, + get the format for the dissambiguation. + + Used mainly in activities since they need to pick a contact from just + the number. + */ + getContactDisplayInfo: function(resolver, phoneNumber, callback) { + resolver(phoneNumber, function onContacts(contacts) { + var contact; + if (Array.isArray(contacts)) { + if (contacts.length == 0) { + callback(null); + return; + } + contact = contacts[0]; + } else { + if (contacts === null) { + callback(null); + return; + } + contact = contacts; + } + + var tel = null; + for (var i = 0; i < contact.tel.length && tel == null; i++) { + if (contact.tel[i].value === phoneNumber) { + tel = contact.tel[i]; + } + } + + // Get the title in the standar way + var details = Utils.getContactDetails(tel, contact); + var info = Utils.getDisplayObject(details.title || null, tel); + /* + XXX: We need to move this to use a single point for + formating: + ${type}${separator}${carrier}${numberHTML} + */ + info.display = info.type + + info.separator + + info.carrier + + tel.value; + + callback(info); + }); + }, + /* + Given a title for a contact, a the current information for + an specific phone, of that contact, creates an object with + all the information needed to display data. + */ + getDisplayObject: function(theTitle, tel) { + var number = tel.value; + var title = theTitle || number; + var type = tel.type && tel.type.length ? tel.type[0] : ''; + var carrier = tel.carrier ? (tel.carrier + ', ') : ''; + var separator = type || carrier ? ' | ' : ''; + var data = { + name: title, + number: number, + type: type, + carrier: carrier, + separator: separator, + nameHTML: '', + numberHTML: '' + }; + + return data; } }; diff --git a/apps/sms/locales/sms.en-US.properties b/apps/sms/locales/sms.en-US.properties index 55565dbf23e3..bb658575d7cf 100644 --- a/apps/sms/locales/sms.en-US.properties +++ b/apps/sms/locales/sms.en-US.properties @@ -47,10 +47,11 @@ recipient[two] = {{n}} recipients recipient[few] = {{n}} recipients recipient[many] = {{n}} recipients recipient[other] = {{n}} recipients -recipientRemoval = Do you want to remove {{recipient}} from the recipients list?' attachmentSize = {{n}} kB attachmentSizeMB = {{n}} MB attachmentOpenError = There is no application available to open this file type. +# Dialog to disambiguate in a new message +remove = Remove # Group participants participant = {[ plural(n) ]} diff --git a/apps/sms/style/custom_dialog.css b/apps/sms/style/custom_dialog.css index 8522d553d4b1..f8896ed7d6ab 100644 --- a/apps/sms/style/custom_dialog.css +++ b/apps/sms/style/custom_dialog.css @@ -99,7 +99,6 @@ body[role="application"] [role="dialog"]:not([data-type="edit"]):not([data-type= text-shadow: 1px 1px 0 rgba(255, 255, 255, 0.3); vertical-align: middle; white-space: nowrap; - width: 100%; } body[role="application"] [role="dialog"]:not([data-type="edit"]) menu[data-items="2"]:not([type="toolbar"]) button { diff --git a/apps/sms/style/notification.css b/apps/sms/style/notification.css index 6b2aa1466be8..1c1a308596e0 100644 --- a/apps/sms/style/notification.css +++ b/apps/sms/style/notification.css @@ -27,3 +27,4 @@ section[role="notification"] p { font-size: 1.5rem; text-align: left; } + diff --git a/apps/sms/style/sms.css b/apps/sms/style/sms.css index 8d13140aa5b1..5cc181ca4092 100644 --- a/apps/sms/style/sms.css +++ b/apps/sms/style/sms.css @@ -8,6 +8,20 @@ html, body { overflow-y: hidden !important; } + +/* + Override BB. Some CSS tweaks for having a better confirm readability +*/ +form[role="dialog"][data-type="confirm"] p { + margin-top: 0 !important; + border-bottom: 0.1rem solid #686868; +} + +form[role="dialog"][data-type="confirm"] p small{ + line-height: 2rem; +} + + /* Override Building Block [Header] styles */ section[role="region"] > header:first-child .icon.icon-clear { background-image: url(images/icons/clear.png); diff --git a/apps/sms/test/unit/dialog_test.js b/apps/sms/test/unit/dialog_test.js new file mode 100644 index 000000000000..2b58f3510379 --- /dev/null +++ b/apps/sms/test/unit/dialog_test.js @@ -0,0 +1,147 @@ +'use strict'; + +requireApp('sms/test/unit/mock_l10n.js'); +requireApp('sms/js/dialog.js'); + +suite('Dialog', function() { + var nativeMozL10n = navigator.mozL10n; + + var params = null; + + + suiteSetup(function() { + loadBodyHTML('/index.html'); + navigator.mozL10n = MockL10n; + params = + { + title: { + value: 'Foo Title', + l10n: false + }, + body: { + value: 'Foo body', + l10n: false + }, + options: { + cancel: { + text: { + value: 'Foo Cancel', + l10n: false + } + } + } + + }; + }); + + suiteTeardown(function() { + navigator.mozL10n = nativeMozL10n; + params = null; + }); + + test('Creation', function() { + var dialog = new Dialog(params); + // We check if the object is created properly + assert.isFalse(!dialog.show); + assert.equal(typeof dialog.show, 'function'); + assert.isFalse(!dialog.hide); + assert.equal(typeof dialog.hide, 'function'); + }); + + test('Appending to DOM', function() { + var previouslyDefinedForms = document.getElementsByTagName('form').length; + // In this case we have several forms pre-defined (5): + // - "messages-compose-form" + // - "messages-edit-form" + // - "loading" + // - "attachment" + // - "threads-edit-form" + assert.equal(previouslyDefinedForms, 5); + // Now we create the new element + var dialog = new Dialog(params); + // We check if the object is appended to the DOM + dialog.show(); + // Is appended properly? + var currentlyDefinedForms = document.getElementsByTagName('form'); + var currentlyDefinedFormsLength = currentlyDefinedForms.length; + assert.equal(currentlyDefinedFormsLength, 6); + // We check the type + var dialogForm = currentlyDefinedForms[currentlyDefinedFormsLength - 1]; + assert.equal(dialogForm.dataset.type, 'confirm'); + }); + + test('Checking the structure. Default.', function() { + // Now we create the new element + var dialog = new Dialog(params); + // We append the element to the DOM + dialog.show(); + // We retrieve the last created form + var currentlyDefinedForms = document.getElementsByTagName('form'); + var currentlyDefinedFormsLength = currentlyDefinedForms.length; + // We check the type + var dialogForm = currentlyDefinedForms[currentlyDefinedFormsLength - 1]; + // We check how many buttons we have (only the mandatory one) + var formOptions = dialogForm.getElementsByTagName('button'); + assert.equal(formOptions.length, 1); + }); + + test('Checking the structure. Confirm.', function() { + // We add the confirm + params.options.confirm = { + text: { + value: 'Foo Cancel', + l10n: false + } + }; + // Now we create the new element + var dialog = new Dialog(params); + // We append the element to the DOM + dialog.show(); + // We retrieve the last created form + var currentlyDefinedForms = document.getElementsByTagName('form'); + var currentlyDefinedFormsLength = currentlyDefinedForms.length; + // We check the type + var dialogForm = currentlyDefinedForms[currentlyDefinedFormsLength - 1]; + // We check how many buttons we have (mandatory + confirm one) + var formOptions = dialogForm.getElementsByTagName('button'); + assert.equal(formOptions.length, 2); + // We check if there is a 'recommend' style + var optionalOptions = dialogForm.getElementsByClassName('recommend'); + assert.equal(optionalOptions.length, 1); + }); + + test('Checking the localization.', function() { + var l10nCancel = 'keyCancel'; + var l10nConfirm = 'keyConfirm'; + params.options.cancel = { + text: { + value: 'keyCancel', + l10n: true + } + }; + params.options.confirm = { + text: { + value: 'keyConfirm', + l10n: true + } + }; + // Now we create the new element + var dialog = new Dialog(params); + // We append the element to the DOM + dialog.show(); + // We retrieve the last created form + var currentlyDefinedForms = document.getElementsByTagName('form'); + var currentlyDefinedFormsLength = currentlyDefinedForms.length; + // We check the type + var dialogForm = currentlyDefinedForms[currentlyDefinedFormsLength - 1]; + // We check how many buttons we have (mandatory + confirm one) + var formOptions = dialogForm.getElementsByTagName('button'); + assert.equal(formOptions.length, 2); + // We check localization + assert.equal(formOptions[0].dataset.l10nId, l10nCancel); + assert.equal(formOptions[1].dataset.l10nId, l10nConfirm); + }); + + + +}); diff --git a/apps/sms/test/unit/mock_utils.js b/apps/sms/test/unit/mock_utils.js index bb34e428090a..d321ebfe83cf 100644 --- a/apps/sms/test/unit/mock_utils.js +++ b/apps/sms/test/unit/mock_utils.js @@ -21,5 +21,6 @@ var MockUtils = { getContactDetails: Utils.getContactDetails, getResizedImgBlob: Utils.getResizedImgBlob, removeNonDialables: Utils.removeNonDialables, - compareDialables: Utils.compareDialables + compareDialables: Utils.compareDialables, + getDisplayObject: Utils.getDisplayObject }; diff --git a/apps/sms/test/unit/recipients_test.js b/apps/sms/test/unit/recipients_test.js index 4b1fc1a60e39..e2d8e4a3f952 100644 --- a/apps/sms/test/unit/recipients_test.js +++ b/apps/sms/test/unit/recipients_test.js @@ -49,7 +49,9 @@ suite('Recipients', function() { email: 'a@b.com', source: 'none', // Mapped to node attr, not true boolean - editable: 'true' + editable: 'true', + // Disambiguation 'display' attribute + display: 'Type | Carrier, Number' }; }); diff --git a/apps/sms/test/unit/utils_test.js b/apps/sms/test/unit/utils_test.js index e8bf209fa31f..18a06abfe643 100644 --- a/apps/sms/test/unit/utils_test.js +++ b/apps/sms/test/unit/utils_test.js @@ -830,5 +830,75 @@ suite('Utils.Template', function() { ); }); }); +}); + +suite('getDisplayObject', function() { + + test('Tel object with carrier title and type', function() { + var myTitle = 'My title'; + var type = 'Mobile'; + var carrier = 'Carrier'; + var value = 111111; + var data = Utils.getDisplayObject(myTitle, { + 'value': value, + 'carrier': carrier, + 'type': [type] + }); + assert.equal(data.name, myTitle); + assert.equal(data.separator, ' | '); + assert.equal(data.type, type); + assert.equal(data.carrier, carrier + ', '); + assert.equal(data.number, value); + }); + + test('Tel object without title and type', function() { + var myTitle = 'My title'; + var type = 'Mobile'; + var value = 111111; + var data = Utils.getDisplayObject(myTitle, { + 'value': value, + 'carrier': null, + 'type': [type] + }); + + assert.equal(data.name, myTitle); + assert.equal(data.separator, ' | '); + assert.equal(data.type, type); + assert.equal(data.carrier, ''); + assert.equal(data.number, value); + }); + + test('Tel object with NO carrier title and NO type', function() { + var myTitle = 'My title'; + var type = 'Mobile'; + var value = 111111; + var data = Utils.getDisplayObject(myTitle, { + 'value': value + }); + + assert.equal(data.name, myTitle); + assert.equal(data.separator, ''); + assert.equal(data.type, ''); + assert.equal(data.carrier, ''); + assert.equal(data.number, value); + }); + + test('Tel object with carrier title and type and NO title', function() { + var myTitle = 'My title'; + var type = 'Mobile'; + var carrier = 'Carrier'; + var value = 111111; + var data = Utils.getDisplayObject(null, { + 'value': value, + 'carrier': carrier, + 'type': [type] + }); + + assert.equal(data.name, value); + assert.equal(data.separator, ' | '); + assert.equal(data.type, type); + assert.equal(data.carrier, carrier + ', '); + assert.equal(data.number, value); + }); });