1300995 - Implement the form autofill autocomplete footer

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

Implement the footer allowing management of saved autofill profiles.

Comment 1

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #0)
> Implement the footer allowing management of saved autofill profiles.

Matt, can you elaborate on what this means or provide a screenshot?  Thanks!

Comment 2

[Matthew N. [:MattN]]

https://mozilla.invisionapp.com/share/MA8GQUEVZ#/screens/188574112

Comment 4

[Ray Lin[:ralin]]

taking this bug per https://bugzilla.mozilla.org/show_bug.cgi?id=1329628#c17 and discussion in meeting.

Comment 5

[Ray Lin[:ralin]]

Attachment: Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

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

Comment 6

[Ray Lin[:ralin]]

Still lack of a test for the footer, I'll do it in part 2 patch. And apart from the footer implementation, I also refactored the bindings to let the profile-item and the footer extend from a base binding since there's a high portion of shared codes.

Comment 7

[Ray Lin[:ralin]]

Attachment: Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

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

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

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

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review152708

::: browser/extensions/formautofill/FormAutofillContent.jsm:532
(Diff revision 2)
> +    if (e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN &&
> +        lastAutoCompleteResult &&
> +        lastAutoCompleteResult.getStyleAt(selectedIndex) != "autofill-profile" &&
> +        selectedIndex == lastAutoCompleteResult.matchCount - 1) {
> +      Services.cpmm.sendSyncMessage("FormAutofill:OnMoreOptionEnter");

Does this need to be a sync message? If not, please make it async

::: browser/extensions/formautofill/FormAutofillContent.jsm:532
(Diff revision 2)
> +    if (e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN &&
> +        lastAutoCompleteResult &&
> +        lastAutoCompleteResult.getStyleAt(selectedIndex) != "autofill-profile" &&
> +        selectedIndex == lastAutoCompleteResult.matchCount - 1) {
> +      Services.cpmm.sendSyncMessage("FormAutofill:OnMoreOptionEnter");
> +    }

Nit: The coding style is to use an early return instead:
```js
if (e.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN ||
    lastAutoCompleteResult) {
  return;    
}

let selectedRowStyle = lastAutoCompleteResult.getStyleAt(selectedIndex);
if (selectedRowStyle == "autofill-footer") {
  Services.cpmm.sendAsyncMessage("FormAutofill:OpenPreferences");
}
```

Later we would add a condition for the insecure warning row to open the SUMO page. Alternatively you can use one message but then send the `selectedRowStyle` along with the message. I would then name the message something like "FormAutofill:HandleAutocompleteEnter" or something.

::: browser/extensions/formautofill/FormAutofillContent.jsm:536
(Diff revision 2)
> +
> +    if (e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN &&
> +        lastAutoCompleteResult &&
> +        lastAutoCompleteResult.getStyleAt(selectedIndex) != "autofill-profile" &&
> +        selectedIndex == lastAutoCompleteResult.matchCount - 1) {
> +      Services.cpmm.sendSyncMessage("FormAutofill:OnMoreOptionEnter");

I find the name "FormAutofill:OnMoreOptionEnter" hard to undertand. I think it would be clearest to describe the action instead: "FormAutofill:OpenPreferences"

There wouldn't be much in common with the insecure warning anyways as that would instead open a SUMO page probably.

::: browser/extensions/formautofill/FormAutofillParent.jsm:205
(Diff revision 2)
> +        const win = RecentWindow.getMostRecentBrowserWindow();
> +        win.openUILinkIn("about:preferences#privacy", "tab", {
> +          allowPopups: true,
> +          relatedToCurrent: true,
> +        });

I forgot the other day to mention that you should probably use `win.openPreferences` since it's the standard way to open preferences

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:186
(Diff revision 2)
>    getStyleAt(index) {
>      this._checkIndexBounds(index);
> +    if (index == this.matchCount - 1) {
> +      return "autofill-profile-footer";

Why not "autofill-footer"? It's not specific to profiles

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:70
(Diff revision 2)
>      if (!Services.prefs.getBoolPref("extensions.formautofill.addresses.enabled")) {
>        return;
>      }
>  
> +    const doc = content.document;
> +    const {chromeEventHandler: target} = doc.ownerGlobal.getInterface(Ci.nsIDocShell);

`target` may be confused with `message.target` so I would recommend using a different name or ideally not aliasing at all. If you're worried about the length of the lines that use it, you can wrap the arguments.

::: browser/extensions/formautofill/content/formautofill.xml:60
(Diff revision 2)
> +        <![CDATA[
> +          let outerBoxRect = this.parentNode.getBoundingClientRect();
> +
> +          // Make item fit in popup as XUL box could not constrain
> +          // item's width
> +          this._itemBox.style.width =  outerBoxRect.width + "px";

Existing nit: Remove the extra space after `=`

::: browser/extensions/formautofill/content/formautofill.xml:142
(Diff revision 2)
> +    </implementation>
> +  </binding>
>  
> -      <method name="_onChanged">
> -        <body></body>
> -      </method>
> +  <binding id="autocomplete-profile-listitem-footer" extends="chrome://formautofill/content/formautofill.xml#autocomplete-profile-listitem-base">
> +    <xbl:content xmlns="http://www.w3.org/1999/xhtml">
> +      <div anonid="profile-footer" class="profile-item-box profile-footer">

`autofill-footer` would be more clear IMO

::: browser/extensions/formautofill/content/formautofill.xml:148
(Diff revision 2)
> -      </method>
> +        window.openUILinkIn("about:preferences#privacy", "tab", {
> +          relatedToCurrent: true,
> +        });

This should also use openPreferences

::: browser/extensions/formautofill/content/formautofill.xml:170
(Diff revision 2)
> -          this._label.textContent = primary;
> -          this._comment.textContent = secondary;
> +          const footerTextBundleKey = this._itemBox.getAttribute("size") === "small" ?
> +            "popupFooterOptionShort" : "popupFooterOption";
> +          const footerText = this._stringBundle.GetStringFromName(footerTextBundleKey);
>  
> -          // Make item fit in popup as XUL box could not constrain
> +          this._itemBox.textContent = footerText;

Add a comment about what this is doing

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:12
(Diff revision 2)
>  savedProfiles = Saved Profiles…
>  saveAddressMessage = Firefox now saves your form data to help you fill out forms faster!
>  viewAutofillOptions = View Form Autofill options…
>  openAutofillMessagePanel = Open Form Autofill message panel
> +popupFooterOption = Form Autofill Options
> +popupFooterOptionShort = More Options

I don't think "More Options" is a good description of what will happen when a user selects it… it makes it seem like it will add more profiles to the list. Why not "Autofill Options" or just "Options". Also, we should probably use the word "Preferences" on OS X and "Options" otherwise since that's what we use for regular options. We really need to complete the copy review with Michelle…

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:13
(Diff revision 2)
>  
>  xul|richlistitem[originaltype="autofill-profile"][selected="true"] > .profile-item-box {
>    background-color: #F2F2F2;
>  }
>  
>  .profile-item-box {

Rename `profile-item-box` to `autofill-item-box` if it's being shared with non-profile rows.

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:83
(Diff revision 2)
> +  background-color: #EDEDED;
> +  justify-content: center;
> +}
> +.profile-footer:hover {
> +  background-color: var(--arrowpanel-dimmed);

Please file an MVP bug to test the autofill UI in Windows high contrast mode.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

https://reviewboard.mozilla.org/r/148302/#review152726

::: browser/extensions/formautofill/test/browser/browser.ini:10
(Diff revision 1)
>    ../fixtures/autocomplete_basic.html
>  
>  [browser_check_installed.js]
>  [browser_editProfileDialog.js]
>  [browser_first_time_use_doorhanger.js]
> +[browser_popup_footer.js]

Nit: Use "autocomplete" instead of "popup" since "popup" is more generic.

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:1
(Diff revision 1)
> +const url = "http://mochi.test:8888/browser/browser/extensions/formautofill/test/browser/autocomplete_basic.html";

Can you put "http://mochi.test:8888/browser/browser/extensions/formautofill/test/browser/" in a const in head.js and use it here.

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:1
(Diff revision 1)
> +const url = "http://mochi.test:8888/browser/browser/extensions/formautofill/test/browser/autocomplete_basic.html";
> +

Nit: use uppercase for const normally

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:3
(Diff revision 1)
> +registerCleanupFunction(async function() {
> +  let addresses = await getAddresses();
> +  if (addresses.length) {
> +    await removeAddresses(addresses.map(address => address.guid));
> +  }
> +});

This should also be moved to head.js (and removed from existing bc tests).

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:10
(Diff revision 1)
> +  if (addresses.length) {
> +    await removeAddresses(addresses.map(address => address.guid));
> +  }
> +});
> +
> +add_task(async function test_setup_storage() {

Nit: setup functions normally have the `setup_` prefix, test functions have the `test_` prefix so remove `test_` from this one function.

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:29
(Diff revision 1)
> +    // "identifyAutofillFields" is invoked asynchronously in "focusin" event. We
> +    // should make sure fields are ready to open the popup.
> +    await sleep(500);

This will probably break on debug builds which are slower. Why can't we use waitForCondition here and check the popup's state? If there is no form history then I thought that would work.

::: browser/extensions/formautofill/test/browser/browser_popup_footer.js:43
(Diff revision 1)
> +    // press enter on the footer
> +    await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
> +    await BrowserTestUtils.waitForCondition(() => {
> +      listItemElems = itemsBox.querySelectorAll(".autocomplete-richlistitem");
> +      return listItemElems.length == expectedItemLength;
> +    });
> +    prefTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, privacyPrefURL);
> +    for (let i = 0; i < expectedItemLength; i++) {
> +      await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
> +    }
> +    await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
> +    await BrowserTestUtils.removeTab(await prefTabPromise);
> +    ok(true, "Tab: preferences#privacy was successfully opened by pressing enter on the footer");
> +  });

This should be a separate test function

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

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

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

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

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review152708

Thanks for your prompt review. :D

> Does this need to be a sync message? If not, please make it async

I used async at first, and I thought it should work...but somehow the browser crashes if use async api. I haven't figure out why...

> Nit: The coding style is to use an early return instead:
> ```js
> if (e.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN ||
>     lastAutoCompleteResult) {
>   return;    
> }
> 
> let selectedRowStyle = lastAutoCompleteResult.getStyleAt(selectedIndex);
> if (selectedRowStyle == "autofill-footer") {
>   Services.cpmm.sendAsyncMessage("FormAutofill:OpenPreferences");
> }
> ```
> 
> Later we would add a condition for the insecure warning row to open the SUMO page. Alternatively you can use one message but then send the `selectedRowStyle` along with the message. I would then name the message something like "FormAutofill:HandleAutocompleteEnter" or something.

Re-wrote to your way, much clearer, thanks. I think seperate into two messages should be fine.

> I forgot the other day to mention that you should probably use `win.openPreferences` since it's the standard way to open preferences

Updated to `openPreferences` and added a new "origin" type for autocomplete popup.

> Why not "autofill-footer"? It's not specific to profiles

Renamed all to autofill-footer.

> I don't think "More Options" is a good description of what will happen when a user selects it… it makes it seem like it will add more profiles to the list. Why not "Autofill Options" or just "Options". Also, we should probably use the word "Preferences" on OS X and "Options" otherwise since that's what we use for regular options. We really need to complete the copy review with Michelle…

Use "Preferences" for Mac and "Options" for Window/Linux now. Also removed the word "More" in small layout, so the result would be:

OSX: Form Autofill Preferences | Preferences
Windows/Linx: Form Autofill Options | Options

> Please file an MVP bug to test the autofill UI in Windows high contrast mode.

Do you mean adding a test for UI in HCM or refine the looks and feels in HCM? Since I don't know there's a way to test HCM. Thanks.

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

https://reviewboard.mozilla.org/r/148302/#review152726

Thanks for the review.

> This will probably break on debug builds which are slower. Why can't we use waitForCondition here and check the popup's state? If there is no form history then I thought that would work.

It seems no need to wait for a period before pressing down key on my laptop, but I'm not super sure checking popup's state is enough for knowing identifyAutofillFields is invoked. I encountered the same issue in mochitest that no popup would show up sometimes, and we use the similar way to mitigate the issue.

> This should be a separate test function

Split into two separate test functions.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review152708

> I used async at first, and I thought it should work...but somehow the browser crashes if use async api. I haven't figure out why...

Oh… I think you can get a crash if a sync message is used during an async message or something like that. Maybe the GetSelectedIndex message is causing it. I don't think we can land with a sync message as there is now an allowlist of sync message and I doubt we can convince people to approve this.

> Do you mean adding a test for UI in HCM or refine the looks and feels in HCM? Since I don't know there's a way to test HCM. Thanks.

I mean a bug to manually test HCM and potentially fix any found issues.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

https://reviewboard.mozilla.org/r/148302/#review152726

> It seems no need to wait for a period before pressing down key on my laptop, but I'm not super sure checking popup's state is enough for knowing identifyAutofillFields is invoked. I encountered the same issue in mochitest that no popup would show up sometimes, and we use the similar way to mitigate the issue.

If the popup doesn't open after pressing down then that seems like a bug that needs to be fixed.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review153146

r=me once we fix the syncMessage and check that it's fine to change the histogram labels

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:11
(Diff revisions 2 - 3)
>  popupFooterOption = Form Autofill Options
> -popupFooterOptionShort = More Options
> +popupFooterOptionShort = Options
> +popupFooterOptionOSX = Form Autofill Preferences
> +popupFooterOptionOSXShort = Preferences

I think this could be confused with the doorhanger footer which is also a type of popup… Maybe autocompleteFooterOption…

::: browser/extensions/formautofill/FormAutofillParent.jsm:206
(Diff revision 3)
>          this._onFormSubmit(data, target);
> +        break;
> +      }
> +      case "FormAutofill:OpenPreferences": {
> +        const win = RecentWindow.getMostRecentBrowserWindow();
> +        win.openPreferences("panePrivacy", {origin: "autocompletePopup"});

I think "autofillFooter" is more clear

::: toolkit/components/telemetry/Histograms.json:6446
(Diff revision 3)
>      "record_in_processes": ["main", "content"],
>      "bug_numbers": [1330315],
>      "alert_emails": ["[e-mail]"],
>      "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["aboutHome", "aboutTelemetry", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],
> +    "labels": ["aboutHome", "aboutTelemetry", "autocompletePopup", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],

I'm not sure if it's fine to change the labels like this afterwards. Can you ask Georg or Dexter?

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

https://reviewboard.mozilla.org/r/148302/#review153152

Thanks

Comment 20

[Ray Lin[:ralin]]

Hi Georg,
 

We're going to add a button on the autocomplete popup to open preferences pane, but I could not find a proper label on the current list to pass to openPreferences. Is it fine to add a new label like this? Thanks.

::: toolkit/components/telemetry/Histograms.json:6446
(Diff revision 3)
>      "record_in_processes": ["main", "content"],
>      "bug_numbers": [1330315],
>      "alert_emails": ["[e-mail]"],
>      "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["aboutHome", "aboutTelemetry", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],
> +    "labels": ["aboutHome", "aboutTelemetry", "autocompletePopup", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

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

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

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

Comment 23

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review153146

latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70fefc55ae7a5372a1cd22572f318e904c5edfd1

It's interesting..I switch back to Async, and Async just works now, maybe some issues were fixed after rebased. I'll see if all green on try. Thanks.

Comment 24

[Georg Fritzsche [:gfritzsche]]

(In reply to Ray Lin[:ralin] from comment #20)
> We're going to add a button on the autocomplete popup to open preferences
> pane, but I could not find a proper label on the current list to pass to
> openPreferences. Is it fine to add a new label like this? Thanks.

Can you call out the difference more clearly? It is hard to see with this format.

In general, you can add new labels on the end of the list without problems, but not elsewhere.

Comment 25

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> In general, you can add new labels on the end of the list without problems,
> but not elsewhere.

Frank, can you confirm this for the aggregator?

Comment 26

[Ray Lin[:ralin]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> (In reply to Ray Lin[:ralin] from comment #20)
> > We're going to add a button on the autocomplete popup to open preferences
> > pane, but I could not find a proper label on the current list to pass to
> > openPreferences. Is it fine to add a new label like this? Thanks.
> 
> Can you call out the difference more clearly? It is hard to see with this
> format.
> 
Sorry about the format. The difference is a new value was inserted on the labels list.
> In general, you can add new labels on the end of the list without problems,
> but not elsewhere.
I see, then that won't be a problem since adding a new label is exact what I want to do here.

Comment 27

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

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

Comment 28

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

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

Comment 29

[Ray Lin[:ralin]]

Fixed the existing tests since the results are changed due to adding a footer.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b846ac311bbc0924b7eaed7e435ae12cfc35b164

Comment 30

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/5-6/

Comment 31

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

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

Comment 32

[Ray Lin[:ralin]]

still has bc7 failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=107050881&repo=try&lineNumber=4194

It looks like a null pointer issue of nsIAutoCompleteInput declaration, so maybe it's because our handle did something prior to EnterMatch that causing mInput referring to elsewhere rather than the original focused input. I could not reproduce this issue on my laptop now, I'll dig deeper into it tomorrow.

Comment 33

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/6-7/

Comment 34

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/5-6/

Comment 35

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/7-8/

Comment 36

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/6-7/

Comment 37

[Ray Lin[:ralin]]

So far the latest patch looks good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd3beff9b0059584b987f406b081397add3b13c
 
For the part1 patch:
Instead of immediately sending message to parent in keypress handler, I add a "DOMAutoComplete" event listener to make sure openPreference is happened after EnterMatch to avoid the potential crash caused by null pointer.

For the part2 patch:
I added back the sleep(500) since still some timeout failures on debug build. I've filed bug 1373130 to address this issue.

Matt, could you have a quick look on the changes after your last review? Thanks.

Comment 38

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/7-8/

Comment 39

[Ray Lin[:ralin]]

Sorry, I misplaced the second parameter of setTimeout....
The correct try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e41d43f88fc0922ab5b9c02d3c29e3610bf3024

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

https://reviewboard.mozilla.org/r/147994/#review153796

::: toolkit/components/telemetry/Histograms.json:6446
(Diff revision 8)
>      "record_in_processes": ["main", "content"],
>      "bug_numbers": [1330315],
>      "alert_emails": ["[e-mail]"],
>      "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["aboutHome", "aboutTelemetry", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],
> +    "labels": ["aboutHome", "aboutTelemetry", "autofillFooter", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "devDisconnectedAlert", "experimentsOpenPref", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storagePressure", "translationInfobar", "UITour", "menubar", "notifOpenSettings", "other"],

As Georg said, you can only add a new label (autofillFooter) to the *end* of this list.

Comment 41

[Matthew N. [:MattN]]

(In reply to Ray Lin[:ralin] from comment #37)
> Matt, could you have a quick look on the changes after your last review?

In the future please make sure I'm flagged as I don't always read all bugmail for the component. If you fix the label issue then I think it's fine to land.

Comment 42

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/8-9/

Comment 43

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/8-9/

Comment 44

[Ray Lin[:ralin]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #41)
> (In reply to Ray Lin[:ralin] from comment #37)
> > Matt, could you have a quick look on the changes after your last review?
> 
> In the future please make sure I'm flagged as I don't always read all
> bugmail for the component. 
Got it.
>If you fix the label issue then I think it's fine to land.
Fixed the issue, thank you :D

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a4962ec1b83
Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8c18e90f0774
Part 2. Add a browser chrome test for form autofill popup footer. r=MattN

Comment 46

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing new test browser_autocomplete_footer.js intermittently:

https://hg.mozilla.org/integration/autoland/rev/2ab8812c2505ec59896bbab9a020cfa3159ee4e3
https://hg.mozilla.org/integration/autoland/rev/e9d9a7fcac218a2980daaa83558bce58ac7bb8fb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8c18e90f0774c3da06fc15bb8787b26d2645f5a9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107236293&repo=autoland

[task 2017-06-15T07:54:21.682288Z] 07:54:21     INFO - TEST-START | browser/extensions/formautofill/test/browser/browser_autocomplete_footer.js
[task 2017-06-15T07:55:06.713300Z] 07:55:06     INFO - TEST-INFO | started process screentopng
[task 2017-06-15T07:55:08.453195Z] 07:55:08     INFO - TEST-INFO | screentopng: exit 0
[task 2017-06-15T07:55:08.456841Z] 07:55:08     INFO - Buffered messages logged at 07:54:21
[task 2017-06-15T07:55:08.457989Z] 07:55:08     INFO - Entering test bound setup_storage
[task 2017-06-15T07:55:08.461119Z] 07:55:08     INFO - Leaving test bound setup_storage
[task 2017-06-15T07:55:08.464423Z] 07:55:08     INFO - Entering test bound test_click_on_footer
[task 2017-06-15T07:55:08.468000Z] 07:55:08     INFO - Buffered messages logged at 07:54:24
[task 2017-06-15T07:55:08.471897Z] 07:55:08     INFO - Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource://formautofill/FormAutofillContent.jsm" line: 156}]
[task 2017-06-15T07:55:08.473662Z] 07:55:08     INFO - Buffered messages finished
[task 2017-06-15T07:55:08.476203Z] 07:55:08     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_autocomplete_footer.js | Test timed out -

Comment 47

[Ray Lin[:ralin]]

Thanks for the information, :aryx

I guess it's like Matt mentioned, 500ms might be still not long enough for debug build. But no better way to fix it now, I'll extend the period to make it more reliable until we fix bug 1373130.

Comment 48

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/9-10/

Comment 49

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/9-10/

Comment 50

[Ray Lin[:ralin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6613f95e2892c9e3cc79a578aec8a07c902c8f65

Thanks

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c104436d1937
Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it. r=MattN
https://hg.mozilla.org/integration/autoland/rev/cc5161b809a0
Part 2. Add a browser chrome test for form autofill popup footer. r=MattN

Comment 52

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently timing out in own test browser_autocomplete_footer.js:

https://hg.mozilla.org/integration/autoland/rev/7a729935d60d6eb4d0f7b58ea6307634badd4c9e
https://hg.mozilla.org/integration/autoland/rev/f881b39e669dbe0e4717cba6fbdb12d9ba732bfc

Linux x64 asan: https://treeherder.mozilla.org/logviewer.html#?job_id=107437973&repo=autoland
Linux x64 debug: https://treeherder.mozilla.org/logviewer.html#?job_id=107443413&repo=autoland
OS X debug: https://treeherder.mozilla.org/logviewer.html#?job_id=107447884&repo=autoland

Comment 53

[Ray Lin[:ralin]]

One more wait for binding attachment can fix the test, might looks a bit hacky, but I don't have any more proper way come to mind right now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=840c5b0c4dbc74ac52203da30c9946f344d2303a

I'll need a further experiment to see what really causes the issue. I suspect that the binding might hasn't attached yet at the moment we click on the footer, due to the delay stylesheet insertion while popup open. For the reason why this issue didn't break any existing tests is because no matter binding is attached or not, if we use keyboard navigation to test functionality, we actually rely on the backend instead of popup UI. But once we need to click on the footer, then we must ensure binding is correctly attached before we start to click since the handler is on our own binding.

The problem turns back to https://bugzilla.mozilla.org/show_bug.cgi?id=1364477#c39 and what we've mentioned in the workweek about the flicker when first time opening popup.  My deduction is that since the flicker is observable by naked eyes, it's likely for bc test to simulate click event on the wrong binding, especially in debug build.


*1  await BrowserTestUtils.waitForCondition(() => autoCompletePopup.popupOpen);
   // Click on the footer
   const listItemElems = itemsBox.querySelectorAll(".autocomplete-richlistitem");
   const prefTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, PRIVACY_PREF_URL);
*2  await EventUtils.synthesizeMouseAtCenter(listItemElems[listItemElems.length - 1], {});

As if the problem is exact this case, I don't think we could come up with a straight fix now. No clue would tell the binding has successfully attached between *1 to *2. I'd like to land this patch with the workaround first, and refactor those fragile test cases later, do you think it's fine? Thanks.

Comment 54

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/10-11/

Comment 55

[Frank Bertsch [:frank]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> > In general, you can add new labels on the end of the list without problems,
> > but not elsewhere.
> 
> Frank, can you confirm this for the aggregator?

Yes, this should be fine for the aggregator.

Comment 56

[Ray Lin[:ralin]]

(In reply to Frank Bertsch [:frank] from comment #55)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> > > In general, you can add new labels on the end of the list without problems,
> > > but not elsewhere.
> > 
> > Frank, can you confirm this for the aggregator?
> 
> Yes, this should be fine for the aggregator.

Thank you Frank, Georg :D

Comment 57

[Ray Lin[:ralin]]

Added an experiment in bug 1373662 comment 2 to verify the issue of binding attaching.

Comment 58

[Ray Lin[:ralin]]

Comment on attachment 8876643 [details]
Bug 1300995 - Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147994/diff/10-11/

Comment 59

[Ray Lin[:ralin]]

Comment on attachment 8876985 [details]
Bug 1300995 - Part 2. Add a browser chrome test for form autofill popup footer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148302/diff/11-12/

Comment 60

[Ray Lin[:ralin]]

Instead of inserting one more 2 sec wait for binding attachment, I simply added a attribute every time when our binding being attached that provide a basis to determine whether our binding is attached or not.

Seeing the result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4941c2faea5ed363e633626c0dda65fa386491a0, I believe this approach has already solved the issue. I'm flagging checkin-needed as no major changes since Matt's last review. Thanks.

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/329a0ee18186
Part 1. Add a footer on formautofill popup to let users open a preferences privacy tab when click on it. r=MattN
https://hg.mozilla.org/integration/autoland/rev/53bde2fc1fa5
Part 2. Add a browser chrome test for form autofill popup footer. r=MattN

Comment 62

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/329a0ee18186
https://hg.mozilla.org/mozilla-central/rev/53bde2fc1fa5

Comment 63

[Andrei Vaida [:avaida]]

Ray, is this something that manual QE should be looking at? Note that the link from Comment 2 is no longer working, so we don't have any kind of context here.

Comment 64

[Ray Lin[:ralin]]

Apart from UI part(bug 1378350), its functionality has already been covered by the automation tests, so I guess we don't need other manual tests here. Thanks :-) 

More context (see point 4): https://mozilla.invisionapp.com/share/AP8TFZ22G#/screens/185446489