[Form Autofill] Collect information on how much time users spent on page with forms (w/wo form autofill)

NEW
Assigned to

Status

()

Toolkit
Form Manager
3 months ago
14 days ago

People

(Reporter: steveck, Assigned: steveck, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [story][form autofill:M3][autofill-metrics])

User Story

Product Owner should be able to see how long the user spends filling out forms with Form Autofill enabled and disabled.

MozReview Requests

()

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 months ago
Product Owner should be able to see how long does the users spent on the form under form autofill feature enabled/disabled.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

Hi Matt, I'm not sure if there's other way to access the telemetry, but adding a new probe in  Histogram.json directly is one of the solution I know by far. Please let me know if there's other alternative solution for system addon.

BTW we might need basic heuristic for collecting data, otherwise we'll collect nothing since our heuristic could only work for autocomplete property.
Attachment #8846567 - Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
(Assignee)

Comment 4

2 months ago
...Or only collect the data from target website
(Assignee)

Comment 5

2 months ago
Hi Joe, talking about where to collect the data before the feature enabled, do you have any preference? Should we only record the results from 12 target websites, or record data from all the websites that could apply our heuristic checking?
Flags: needinfo?(jcheng)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review122724

::: commit-message-0ec22:1
(Diff revision 1)
> +Bug 1341569 - Part 2: Add the from created time and telemetry probe for form filling duration. r?MattN

Did you mean "form" created time?

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

This won't handle "forms" without a <form>. I guess that would be fine for an initial landing but it would be good to refactor/share the formless logic out of pwmgr eventually.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

In my opinion we should only start the timer when the user first interacts with the form e.g. a `click` or `input` event otherwise we will be basically measuring from page load time which won't be accurate especially when things like session restore are involved.

::: toolkit/components/telemetry/Histograms.json:11065
(Diff revision 1)
>      "kind": "categorical",
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {

I think integer seconds isn't granular enough. I suspect we will only move the needle by less than 10 seconds so with the rounding of seconds it may be noisy to see a difference.

::: toolkit/components/telemetry/Histograms.json:11066
(Diff revision 1)
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],

Nit: I think you can add our public list too: autofill@…

::: toolkit/components/telemetry/Histograms.json:11067
(Diff revision 1)
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],
> +    "expires_in_version": "60",

IIRC I think we want this probe to be opt-out on the release channel.

::: toolkit/components/telemetry/Histograms.json:11068
(Diff revision 1)
> +    "kind": "linear",
> +    "high": 600,
> +    "n_buckets": 20,

I think https://telemetry.mozilla.org/histogram-simulator/#low=0&high=600&n_buckets=22&kind=linear&generate=uniform would be better to have round numbers for the buckets.
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

::: browser/extensions/formautofill/FormAutofillParent.jsm:54
(Diff revision 1)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  const ENABLED_PREF = "browser.formautofill.enabled";
> +const EXPERIMENT_PREF="browser.formautofill.experimental";

Nit: spaces around operators

::: browser/extensions/formautofill/bootstrap.js:43
(Diff revision 1)
>  function startup() {
>    // Besides this pref, we'll need dom.forms.autocomplete.experimental enabled
>    // as well to make sure form autocomplete works correctly.
> -  if (!Services.prefs.getBoolPref("browser.formautofill.experimental")) {
> +  if (Services.prefs.getBoolPref("browser.formautofill.experimental")) {

I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.

Have you considered putting this telemetry in Satchel instead?
Attachment #8846566 - Flags: review?(MattN+bmo)
Attachment #8846567 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 8

2 months ago
mozreview-review-reply
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

> I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.
> 
> Have you considered putting this telemetry in Satchel instead?

If we put telemetry in Satchel, that means we could only record the data from target websites.  There's no heuristic logic in Satchel to know whether the website is suitable for profile filling. I'm fine with that if Joe wants to narrow down the focus to limited websites.
(Assignee)

Comment 9

2 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review123280

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

Thanks for pointing out this issue, I'll create a bug for it since we'll have lots of post-submitting tasks in Q2

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

Yeah I think that would be more accurate.

Updated

2 months ago
Assignee: nobody → schung
Whiteboard: [story] [form autofill][autofill-metrics] → [story][form autofill:M2][autofill-metrics]
(Assignee)

Comment 10

2 months ago
Hi Joe, except for the question in comment 5, I have more questions for you:
- In your imagination, would you expect that the result is displayed like these histograms[1] that each histogram will represent the submitted time while feature disabled and enabled?
- In autofill enabled case, should we record the time only when feature is enabled and users actually apply the stored profile?

[1] https://mzl.la/2mvvRgl : Dashboard about the ADDON_CONTENT_POLICY_SHIM_BLOCKING_LOADING_MS
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8846566 - Flags: review?(MattN+bmo)
(Assignee)

Updated

2 months ago
Attachment #8846567 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8846566 - Attachment is obsolete: true
(Assignee)

Comment 14

2 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review129880

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js
(Diff revision 3)
>    handleEvent(evt) {
>      if (!evt.isTrusted) {
>        return;
>      }
>  
> -    if (!Services.prefs.getBoolPref("browser.formautofill.enabled")) {

I removed this part in order to trigger identifyAutofillFields and create cache. I think it's easier to maintain the form status even when feature is disabled, and it won't affect the search result since it should fallback to history at startSearch.

Updated

a month ago
Whiteboard: [story][form autofill:M2][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135088

::: browser/extensions/formautofill/FormAutofillContent.jsm:413
(Diff revision 3)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.inputStartedTime);
> +      });

Change this to use the form submit observer from your other bug.

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

This is a great place to use https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryStopwatch.jsm

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

I think this should be "input" since "change" only fires when you blur the field IIRC so we wouldn't be measuring time spent in the first field. Also, it would be great for memory and CPU usage to use event bubbling and only have a listener on the whole document for each handler and it gets removed for that handler by looking up the event.target's handler.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

For any event listener from untrusted pages we should be returning early if `event.isTrusted` is false.

::: toolkit/components/telemetry/Histograms.json:11333
(Diff revision 3)
> +    "kind": "exponential",
> +    "high": 300000,
> +    "n_buckets": 22,
> +    "bug_numbers": [1341569],
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Collect information on how much time users spent on page with forms"

Nit: "Collect information on " isn't necessary.

"Milliseconds between focusing an autofill-eligible field and submitting its form"
Attachment #8846567 - Flags: review?(MattN+bmo)
(Assignee)

Comment 16

a month ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135128

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

I knew we had this API for time recording, but my main concern is that user might fill multiple forms at the same time. We could only store one of the record if that happened(or we skip this time recording since it should be a rare case). WDYT?
The API already handles this with the aObj argument: https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/toolkit/components/telemetry/TelemetryStopwatch.jsm#124-128
(Assignee)

Updated

a month ago
Duplicate of this bug: 990201
(Assignee)

Comment 19

a month ago
(In reply to Steve Chung [:steveck] from comment #10)
> Hi Joe, except for the question in comment 5, I have more questions for you:
> - In your imagination, would you expect that the result is displayed like
> these histograms[1] that each histogram will represent the submitted time
> while feature disabled and enabled?

After some discussion with Joe, he is satisfied with the basic dashboards that brought by main pings.

> - In autofill enabled case, should we record the time only when feature is
> enabled and users actually apply the stored profile?

He would like to know the form submitting time even the feature is disabled, that means we'll need to identify forms and listen to earlysubmit event all the time. We might need to beware of this since the overhead wont be reduced even user disable the feature.
(Assignee)

Updated

a month ago
Flags: needinfo?(jcheng)
User Story: (updated)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

22 days ago
mozreview-review
Comment on attachment 8864474 [details]
Bug 1341569 - Part 2: Add properties about change state to understand when the autofill changed.

https://reviewboard.mozilla.org/r/136156/#review139240

::: browser/extensions/formautofill/FormAutofillHandler.jsm:97
(Diff revision 1)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
>          elementWeakRef: Cu.getWeakReference(element),
> +        autofillState: null,

I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.
Comment hidden (mozreview-request)
(Assignee)

Updated

21 days ago
Attachment #8864474 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 25

21 days ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

::: browser/extensions/formautofill/FormAutofillHandler.jsm:66
(Diff revision 6)
>    filledProfileGUID: null,
>  
>    /**
> +   * Boolean to represent if the form has been edited manually.
> +   */
> +  hasNonProfileAutofillValue: false,

This might not a necessay property and I just want to save some time for checking the state in getFormFillingState. Do you think we could keep it but make it looks more like a private property, or remove it and check every input's value and autofillState everytime?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:97
(Diff revision 6)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
>          elementWeakRef: Cu.getWeakReference(element),
> +        autofillState: null,

I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:256
(Diff revision 6)
> +   * will be in used in the end.
> +   */
> +  startFillFormTimer() {
> +    HISTTOGRAM_KEYS.forEach((key) => {
> +      if (TelemetryStopwatch.timeElapsedKeyed(FORM_FILLING_MS_HIST, key, this.form) == -1) {
> +        TelemetryStopwatch.startKeyed(FORM_FILLING_MS_HIST, key, this.form);

I'm not sure if it's kind of abuse the TelemetryStopwatch because I started 3 timers at the same time because we could not confirm which timer will be applied in the end.

Comment 26

21 days ago
mozreview-review-reply
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.

Why do we need to distinguish whether the `input` event is from the user or not? Is that only for pwmgr autofill? For regular form autofill I think that would still signifiy a user started filling a form.

> I'm not sure if it's kind of abuse the TelemetryStopwatch because I started 3 timers at the same time because we could not confirm which timer will be applied in the end.

Hmm… yeah, while discussing the measurement with Georg I realized that this would be a problem with using TelemetryStopwatch. Let me think about it.
(Assignee)

Comment 27

21 days ago
mozreview-review-reply
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> Why do we need to distinguish whether the `input` event is from the user or not? Is that only for pwmgr autofill? For regular form autofill I think that would still signifiy a user started filling a form.

Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.

Comment 28

21 days ago
mozreview-review-reply
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.

OK, I guess we had different ideas in mind and apologies for not looking at the patch before discussing. In my mind the "input" event with once:true should be used to know that a user started filling (not sure if it works for <select>) but we only need to use the "change" event (which happens on blur for <input>) to know that a user made a correction. I think we should coordinate with Ray for this since we should use the same logic as removing the highlight. How do we want to handle a user changing the the value and back to the autofilled one without blurring e.g. "John" => Joh" (with backspace) => "John"
(Assignee)

Comment 29

18 days ago
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #28)
> Comment on attachment 8846567 [details]
> Bug 1341569 - Add the form created time and telemetry probe for form filling
> duration.
> 
> https://reviewboard.mozilla.org/r/119618/#review139456
> 
> > Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.
> 
> OK, I guess we had different ideas in mind and apologies for not looking at
> the patch before discussing. In my mind the "input" event with once:true
> should be used to know that a user started filling (not sure if it works for
> <select>) but we only need to use the "change" event (which happens on blur
> for <input>) to know that a user made a correction. I think we should
> coordinate with Ray for this since we should use the same logic as removing
> the highlight. How do we want to handle a user changing the the value and
> back to the autofilled one without blurring e.g. "John" => Joh" (with
> backspace) => "John"

About the highlighting, I confirmed with Ray that the example you mentioned should remove the highlight even filled data is resumed, and it's also the result that Chrome behaves.

I think the highlighting behavior and actual contact(profile) modification might be decoupled(like Chrome), but we didn't reach consensus about the timing for showing update doorhanger, and I assume that this metrics should align with the doorhanger. Ideally the form visually changes(whether highlights are existed or not) and the storage changes should be the same, but there are some corner cases that would make us confused:
- 1) Edit the fields and revert the changes before submitting the form(like MattN's example)
- 2) Data in storage is changed(via sync or editing profile at the same time) while filling a form and submit

Hi Joe, IIRC Juwei would prefer "Not" to display the doorhanger in both cases, but she could compromise if the doorhanger is aligned with the visual changes. MattN prefers that this metrics should simply considering the data changes just like password manager did. So there are two questions for this metrics:
- ) How to define the "form changes" for this metrics?
- ) Should we decouple the definition of "changes" in update doorhanger and metrics?

Hi MattN, please correct me if I misunderstood your saying on IRC(and any additional insight is welcome)
Flags: needinfo?(jcheng)
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 30

16 days ago
Based on meeting notes, we'll decouple the metrics and update doorhanger since the doorhanger will don't show up in several situations, even the submitted content is different from the address in storage.
In the meeting today we decided that a change means that the value in the field is different than the one we would store (e.g. after stripping leading and trailing whitespace) for the field and we will only consider fields we autofilled when looking for changes.

For this time probe we will have 9 buckets like so (I just made up prefixes now):
address.manual
address.autofill
address.modified
creditCard.manual
creditCard.autofill
creditCard.modified
addressAndCreditCard.manual
addressAndCreditCard.autofill
addressAndCreditCard.modified

I used addressAndCreditCard in case instead of "both" in case we support a third type of autofill in the future.

For tracking the number of times forms were submitted in the three cases I guess we need to reopen bug 990201 since it will only need 6 buckets instead of 9 because we don't need to handle addressAndCreditCard together since we have individual fields to consider for each type.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review141908

Clearing review flag for now based on the meeting discussion. I'm fine with this bug only handling the address fields for now but it may make sense to consider both during implementation to avoid incorrect data in the short term by counting credit card fields as address fields.

If it no longer makes sense to use TelemetryStopwatch then it's fine not to… I didn't anticipate the complexity when I suggested it.

Thanks
Attachment #8846567 - Flags: review?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.