1019471 - (fx-autofill-profile-edit) Dialog to add/edit/view an autofill profile

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Matthew N. [:MattN]]

A single view with editable <input>s for an initial set of fields. The dialog would be passed an optional profile identifier for the edit/view case. The dialog will initially do no validation and simply load/save profiles.

Simple tests for loading/saving should be written.

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1019471 - Create a dialog to add/edit/view an autofill profile

Review commit: https://reviewboard.mozilla.org/r/114056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114056/

Comment 2

[Scott Wu [:scottwu]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114056/diff/1-2/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review117700

::: browser/extensions/formautofill/content/EditProfile.css:1
(Diff revisions 1 - 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Nit: The filename shouldn't start with an uppercase letter.

::: browser/extensions/formautofill/content/EditProfile.xhtml:1
(Diff revisions 1 - 2)
>  <?xml version="1.0" encoding="UTF-8"?>

Same with this filename.

Comment 4

[Scott Wu [:scottwu]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114056/diff/2-3/

Comment 5

[Scott Wu [:scottwu]]

Hi Matt, I moved the addMessageListener part from FormAutofillPreferences.jsm to FormAutofillParent.jsm. I believe you mentioned that I could have the parent listen to add & edit profile messages only when editProfile.xhtml loads, but I can't remember the reason behind it.

The <select> styling still need some work, but I can work on it as a separate bug.

When editProfile.xhtml loaded, I expect the profile manager to either open a blank form or open an existing profile. If an existing profile is chosen, I assume the profile detail along with its guid would be passed into the edit form to populate it, so I wouldn't need to query the profile w/ guid.

Comment 6

[Matthew N. [:MattN]]

(In reply to Scott Wu [:scottwu] from comment #5)
> Hi Matt, I moved the addMessageListener part from
> FormAutofillPreferences.jsm to FormAutofillParent.jsm. I believe you
> mentioned that I could have the parent listen to add & edit profile messages
> only when editProfile.xhtml loads, but I can't remember the reason behind it.

I mentioned that but I don't think it's necessary.

> The <select> styling still need some work, but I can work on it as a
> separate bug.

Sure, no problem as I don't think that's important for M1.

> When editProfile.xhtml loaded, I expect the profile manager to either open a
> blank form or open an existing profile. If an existing profile is chosen, I
> assume the profile detail along with its guid would be passed into the edit
> form to populate it, so I wouldn't need to query the profile w/ guid.

Sure, sounds good given our discussion of how this would work.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review118592

I didn't look closely at the HTML/CSS yet.

::: browser/extensions/formautofill/FormAutofillPreferences.jsm:53
(Diff revision 3)
>    init(document) {
> +    this.createPreferenceGroup(document);
> +    this.attachEventListeners();
> +
> +    return this.refs.formAutofillGroup;

What I was suggesting the other day is that `init` doesn't assume you're loading the privacy pane but I guess the group will always be created before we show any subdialog with the current design.

::: browser/extensions/formautofill/content/editProfile.js:17
(Diff revision 3)
> +function EditProfile(profile) {
> +  this._profile = profile;
> +  this.attachEventListeners();
> +}
> +
> +EditProfile.prototype = {

I guess this should be EditDialog since it should be a noun describing what it is representing?

::: browser/extensions/formautofill/content/editProfile.js:39
(Diff revision 3)
> +   * Asks FormAutofillParent to edit a profile.
> +   * @param  {string} guid
> +   * @param  {object} profile
> +   */
> +  edit(guid, profile) {
> +    Services.cpmm.sendAsyncMessage("FormAutofill:EditProfile",

Why don't we use one message called FormAutofill:SaveProfile that takes an object with an optional guid property?

::: browser/extensions/formautofill/content/editProfile.js:39
(Diff revision 3)
> +   * Asks FormAutofillParent to edit a profile.
> +   * @param  {string} guid
> +   * @param  {object} profile
> +   */
> +  edit(guid, profile) {
> +    Services.cpmm.sendAsyncMessage("FormAutofill:EditProfile",

Why don't we use a single message (e.g. FormAutofill:SaveProfile) and method e.g. `saveProfile` where the message data has an optional guid property.

::: browser/extensions/formautofill/content/editProfile.js:40
(Diff revision 3)
> +   * @param  {string} guid
> +   * @param  {object} profile
> +   */
> +  edit(guid, profile) {
> +    Services.cpmm.sendAsyncMessage("FormAutofill:EditProfile",
> +                                   [guid, profile]);

Nit: An object would be clearer than an array

::: browser/extensions/formautofill/content/editProfile.js:46
(Diff revision 3)
> +   */
> +  fill(profile) {

`fill` is a bit too generic I think. How about `populateExistingValues` or `loadInitialValues`

::: browser/extensions/formautofill/content/editProfile.js:60
(Diff revision 3)
> +
> +  /**
> +   * Get inputs from the form.
> +   * @returns {object}
> +   */
> +  getProfile() {

Nit: buildProfileObject?

::: browser/extensions/formautofill/content/editProfile.js:61
(Diff revision 3)
> +  /**
> +   * Get inputs from the form.
> +   * @returns {object}
> +   */
> +  getProfile() {
> +    return Array.from(document.forms[0]).reduce((obj, input) => {

Nit: `document.forms[0].elements` would be clearer than relying on the fact that forms[0] acts like an array.

::: browser/extensions/formautofill/content/editProfile.js:90
(Diff revision 3)
> +        if (Object.keys(this.getProfile()).length == 0) {
> +          this.refs.save.setAttribute("disabled", true);

Building the profile for every input event seems wasteful creating a lot of garbage objects but I guess waiting for a `change` event would mean that a user couldn't click Save while still focused on the one and only field they filled. I guess this is okay for now.

::: browser/extensions/formautofill/content/editProfile.js:126
(Diff revision 3)
> +  /**
> +   * Attach event listener
> +   */
> +  attachEventListeners() {
> +    window.addEventListener("DOMContentLoaded", this, {once: true});
> +    document.addEventListener("click", this);

It seems like this should be on the two buttons individually or #controls-container

::: browser/extensions/formautofill/content/editProfile.js:140
(Diff revision 3)
> +    document.removeEventListener("input", this);
> +  },
> +};
> +
> +// Pass in argument from openDialog
> +new EditProfile(window.arguments[0]);

In order to make testing the dialog much easier I was thinking we would use a query parameter e.g. editProfile.xhtml?guid=… and then the page would send a message to the parent to get the profile. I guess the problem with that is timing since the async message might take time and the user may see empty fields in the meantime unless we hide the form while waiting. Given that this will appear more snappy I think this is fine and we can have tests using window.open in subframes to avoid having to wait for windows/tabs to open.

::: browser/extensions/formautofill/content/editProfile.xhtml:6
(Diff revision 3)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE html [
> +  <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd">
> +  %htmlDTD;
> +]>
> +<html xmlns="http://www.w3.org/1999/xhtml" xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

I would leave the xul namespace out until/unless we need it since I want to avoid it.

::: browser/extensions/formautofill/content/editProfile.xhtml:50
(Diff revision 3)
> +    </label>
> +    <label id="country-container">
> +      <span>Country</span>
> +      <select id="country">
> +        <option/>
> +        <option>United State</option>

Add an "s": "States". You will probably want @value="US" eventually.

Comment 8

[Scott Wu [:scottwu]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review118592

> Why don't we use one message called FormAutofill:SaveProfile that takes an object with an optional guid property?

Good idea. I'll combine the two methods.

> Building the profile for every input event seems wasteful creating a lot of garbage objects but I guess waiting for a `change` event would mean that a user couldn't click Save while still focused on the one and only field they filled. I guess this is okay for now.

I could also listen to a `change` event, and only listen to `input` when necessary. But maybe I can come back to optimize this part later.

> Add an "s": "States". You will probably want @value="US" eventually.

Ok! Is the complete list of countries with their unique keys and localized names something in our codebase already? or is it something we need to pull in and maintain?

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114056/diff/3-4/

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review118592

> I could also listen to a `change` event, and only listen to `input` when necessary. But maybe I can come back to optimize this part later.

Yeah, it's okay for now.

> Ok! Is the complete list of countries with their unique keys and localized names something in our codebase already? or is it something we need to pull in and maintain?

It's at https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/regionNames.properties and I previously gave Luke some code to enumerate those entries. The library in bug 1345033 will give us more address data.

Comment 11

[Luke Chang [:lchang] (inactive)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #10)
> It's at
> https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> global/regionNames.properties and I previously gave Luke some code to
> enumerate those entries.

Yes, I guess it is https://dxr.mozilla.org/mozilla-central/source/intl/strres/nsIStringBundle.idl

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review120192

::: browser/extensions/formautofill/content/editProfile.css:5
(Diff revision 4)
> +body {
> +  font-size: 1rem;

What is this for? I don't understand what impact it would have in this case.

::: browser/extensions/formautofill/content/editProfile.xhtml:1
(Diff revision 4)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE html [

You're missing the license header here. See https://www.mozilla.org/en-US/MPL/headers/ for the HTML comment version

::: browser/extensions/formautofill/content/editProfile.xhtml:14
(Diff revision 4)
> +    <label id="first-name-container">
> +      <span>First Name</span>
> +      <input id="first-name" type="text"/>
> +    </label>
> +    <label id="middle-name-container">
> +      <span>Middle Name</span>
> +      <input id="middle-name" type="text"/>
> +    </label>
> +    <label id="last-name-container">

In the future we may want to just have code generate this markup to reduce typos.

::: browser/extensions/formautofill/content/editProfile.xhtml:55
(Diff revision 4)
> +        <option>United States</option>
> +      </select>
> +    </label>
> +    <label id="email-container">
> +      <span>Email</span>
> +      <input id="email" type="text"/>

type=email?

::: browser/extensions/formautofill/content/editProfile.xhtml:59
(Diff revision 4)
> +      <span>Email</span>
> +      <input id="email" type="text"/>
> +    </label>
> +    <label id="tel-container">
> +      <span>Phone</span>
> +      <input id="tel" type="text"/>

type="tel"?

::: browser/extensions/formautofill/content/editProfile.xhtml:64
(Diff revision 4)
> +      <input id="tel" type="text"/>
> +    </label>
> +  </form>
> +  <div id="controls-container">
> +    <button id="cancel">Cancel</button>
> +    <button id="save" disabled="true">Save</button>

For XHTML the best practice for boolean attributes is to repeat the attribute name as the value: disabled="disabled".

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review120238

::: browser/extensions/formautofill/content/editProfile.js:12
(Diff revision 4)
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://formautofill/FormAutofillUtils.jsm");
> +
> +function EditDialog(profile) {

This really shouldn't land without tests but since you're gone the rest of the week I'll land this to help unblock other things. Please file a follow-up to add a mochitest-browser-chrome test to test adding and updating profiles from the dialog.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review120252

::: browser/extensions/formautofill/content/editProfile.xhtml:62
(Diff revision 4)
> +  <div id="controls-container">
> +    <button id="cancel">Cancel</button>
> +    <button id="save" disabled="true">Save</button>

If I'm not mistaken, the order of these buttons needs to change based on the OS. Perhaps we can do that with CSS since we can't use the preprocessor if we have a single system add-on for all platforms.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e58b83fd06a9
Create a dialog to add/edit/view an autofill profile. r=MattN

Comment 16

[Matthew N. [:MattN]]

Needinfo to file the follow-ups and reply to the comments above when you're back.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e58b83fd06a9

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8839396 [details]
Bug 1019471 - Create a dialog to add/edit/view an autofill profile

https://reviewboard.mozilla.org/r/114056/#review120252

> If I'm not mistaken, the order of these buttons needs to change based on the OS. Perhaps we can do that with CSS since we can't use the preprocessor if we have a single system add-on for all platforms.

See the #ifdef at https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/toolkit/content/widgets/dialog.xml#23-41

Comment 19

[Scott Wu [:scottwu]]

Thanks, I've opened follow-up bugs.

For ordering the buttons based on the OS: Bug 1352331.
For the test cases: Bug 1347186.