Bug 1019471 (fx-autofill-profile-edit)

Dialog to add/edit/view an autofill profile

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: MattN, Assigned: scottwu)

Tracking

(Depends on: 11 bugs, Blocks: 4 bugs)

unspecified
mozilla55
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Flags: firefox-backlog+
Blocks: 990198
Depends on: 1020774
Depends on: 1020806
Depends on: 1020855
Depends on: 1020837
Summary: Implement a simple dialog to add/edit/view an autofill profile → Dialog to add/edit/view an autofill profile
Depends on: 1022898
Depends on: 1022902
Depends on: 1022916
Depends on: 1022920
Depends on: 1022921
Depends on: 1022924
Depends on: 1022949
Depends on: 1022951

Updated

3 years ago
Whiteboard: p=5 → [form autofill] p=5

Updated

3 years ago
Points: --- → 5
Whiteboard: [form autofill] p=5 → [form autofill]
Blocks: 1299819
Alias: fx-autofill-profile-edit
Whiteboard: [form autofill] → [form autofill:M1]
Priority: -- → P3

Updated

10 months ago
Assignee: nobody → lchang
Assignee: lchang → nobody

Updated

6 months ago
Assignee: nobody → scwwu
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
(Assignee)

Comment 5

6 months ago
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.
(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 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.
Attachment #8839396 - Flags: review?(MattN+bmo)
(Assignee)

Comment 8

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Reporter)

Comment 10

5 months ago
mozreview-review-reply
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

5 months ago
(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
Status: NEW → ASSIGNED
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".
Attachment #8839396 - Flags: review?(MattN+bmo) → review+
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 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

5 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e58b83fd06a9
Create a dialog to add/edit/view an autofill profile. r=MattN
Needinfo to file the follow-ups and reply to the comments above when you're back.
Flags: needinfo?(scwwu)

Comment 17

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e58b83fd06a9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348369
(Reporter)

Comment 18

5 months ago
mozreview-review-reply
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
(Assignee)

Comment 19

5 months ago
Thanks, I've opened follow-up bugs.

For ordering the buttons based on the OS: Bug 1352331.
For the test cases: Bug 1347186.
Flags: needinfo?(scwwu)
Blocks: 1370763
Depends on: 1370474, 1372076, 1372077
Depends on: 1352331, 1347186
(Assignee)

Updated

2 months ago
Depends on: 1375799
(Assignee)

Updated

2 months ago
No longer depends on: 1375799

Updated

2 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.