Implement the form autofill autocomplete footer

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: MattN, Assigned: ralin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3])

MozReview Requests

()

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

Attachments

(2 attachments)

Implement the footer allowing management of saved autofill profiles.

Comment 1

2 years ago
(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!
Whiteboard: [form autofill:M1] → [form autofill]
Depends on: 1329903
Duplicate of this bug: 1329906
(Assignee)

Comment 4

a year ago
taking this bug per https://bugzilla.mozilla.org/show_bug.cgi?id=1329628#c17 and discussion in meeting.
Assignee: nobody → ralin
Blocks: 1329628
Status: NEW → ASSIGNED
Whiteboard: [form autofill] → [form autofill:M3]
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1372235
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
mozreview-review
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.
Attachment #8876643 - Flags: review?(MattN+bmo)
(Reporter)

Comment 10

a year ago
mozreview-review
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
Attachment #8876985 - Flags: review?(MattN+bmo)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1372235
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 15

a year ago
mozreview-review-reply
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.
(Reporter)

Comment 16

a year ago
mozreview-review-reply
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.
(Reporter)

Comment 17

a year ago
mozreview-review-reply
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.
(Reporter)

Comment 18

a year ago
mozreview-review
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": ["jaws@mozilla.com"],
>      "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?
Attachment #8876643 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 19

a year ago
mozreview-review
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
Attachment #8876985 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 20

a year ago
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": ["jaws@mozilla.com"],
>      "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"],
Flags: needinfo?(gfritzsche)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

a year ago
mozreview-review-reply
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.
(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.
Flags: needinfo?(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?
Flags: needinfo?(fbertsch)
(Assignee)

Comment 26

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 39

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

Comment 40

a year ago
mozreview-review
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": ["jaws@mozilla.com"],
>      "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.
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

a year ago
(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
Keywords: checkin-needed

Comment 45

a year ago
Pushed by mozilla@noorenberghe.ca:
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
Keywords: checkin-needed
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 -
Flags: needinfo?(ralin)
(Assignee)

Comment 47

a year ago
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.
Flags: needinfo?(ralin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Depends on: 1373130
(Assignee)

Comment 53

a year ago
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.
Flags: needinfo?(ralin) → needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(fbertsch)
(Assignee)

Comment 56

a year ago
(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
(Assignee)

Updated

a year ago
See Also: → bug 1373662
(Assignee)

Comment 57

a year ago
Added an experiment in bug 1373662 comment 2 to verify the issue of binding attaching.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

a year ago
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.
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed

Comment 61

a year ago
Pushed by lchang@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/329a0ee18186
https://hg.mozilla.org/mozilla-central/rev/53bde2fc1fa5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Flags: qe-verify?
Flags: needinfo?(ralin)
(Assignee)

Comment 64

11 months ago
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
Flags: needinfo?(ralin)
(Assignee)

Updated

9 months ago
See Also: → bug 1413473
You need to log in before you can comment on or make changes to this bug.