Closed Bug 1334615 Opened 7 years ago Closed 7 years ago

Add a probe to determine whether the keyboard or the mouse was used to select an action

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: past, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

What triggers the final action, <Enter> or a click? I'm thinking of a histogram with two buckets.
We want separate probes for the location and search bars.
Priority: -- → P1
I would say there are three input mechanisms of interest:

- pressing <Enter> directly, leading to a default search
- clicking on a result from the dropdown
- navigating to a result from the dropdown using the arrow keys and pressing <Enter>.

Would it make sense to distinguish between all 3 of these?
Dave, that makes sense to me.  This adds two new histograms:

  "FX_URLBAR_SELECTED_RESULT_METHOD": {
    "expires_in_version": "never",
    "kind": "categorical",
    "labels": [
      "enter",
      "chooseEnter",
      "click"
    ],
    ...
  },
  "FX_SEARCHBAR_SELECTED_RESULT_METHOD": {
    "expires_in_version": "never",
    "kind": "categorical",
    "labels": [
      "enter",
      "chooseEnter",
      "click"
    ],
    ...
  },

"enter" => The user hit Enter without choosing a result.
"chooseEnter" => The user chose a result and then hit Enter.
"click" => The user clicked a result.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

https://reviewboard.mozilla.org/r/121626/#review125306

I have some questions (mostly for David).

First, in the urlbar case do we really want the actions to be indexed by index, or do we want them indexed by the result type? Or both? It sounds far more useful to connect action to result type, than to blind indices.
What about the searchbar, since all the results are results, it may be fine to use index? But it could also split by something like "typed, historical, remote, oneoff".

Second: should this ignore one-offs? We already have one-offs in UITelemetry, but it could also be useful to have them here. Though, doesn't look like the code is currently doing anything to filter or distinguish them. Maybe I'm reading it wrong, looks like this may return oneoffs with index -1?

The third main issue is that these are "never" probes, that means by policy they need automated tests to land.
Some tests already exist and could be extended, see
http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_UsageTelemetry_urlbar.js
and
http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_UsageTelemetry_searchbar.js

::: browser/modules/BrowserUsageTelemetry.jsm:248
(Diff revision 1)
> +      this.isFirstResultHeuristic = false;
>        return;
>      }
> +
> +    this.selectedIndex = input.popup.selectedIndex;
> +    this.isFirstResultHeuristic = input.popup._isFirstResultHeuristic;

why don't we directly do here:
if (this.selectedIndex == 0 && input.popup._isFirstResultHeuristic) {
  this.selectedIndex = -1;
}
(with the nice comment), instead of complicating recordUrlbarSelectedResultMethod and having to bring on 2 properties?

::: toolkit/components/telemetry/Histograms.json:5709
(Diff revision 1)
> +    "alert_emails": ["adw@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "categorical",
> +    "labels": [
> +      "enter",
> +      "chooseEnter",

nit: Maybe it's just me, I think "enterSelection" would be clearer as a string. Not that it matters that much since the analysis is made through a script.
Attachment #8848719 - Flags: review?(mak77)
Dave, could you please answer my questions in comment 6? (sorry if I mistyped your name :\)
Flags: needinfo?(dzeber)
Marco, I'm not sure I understand.  The patch doesn't actually record or otherwise directly use the index.  The only point of the index is to tell whether the user chose a non-default index, so that _recordUrlOrSearchbarSelectedResultMethod() can determine which category it should use.  All this bug is doing is setting up a simple accumulating histogram with three categories.  (Two histograms actually, one for the urlbar, one for the searchbar.)  It's not recording indexes or actions.

Re: a test, I'll work on one an post a new patch.
Flags: needinfo?(mak77)
(In reply to Drew Willcoxon :adw from comment #8)
> Marco, I'm not sure I understand.  The patch doesn't actually record or
> otherwise directly use the index.

Hm you are right, I was confused by all the index pass-through.

So basically these only count the global number of times the user interacted with a click, enter or select+enter. Without any hint about what he was doing or where.

I'm still not sure whether this is expected to count one-offs interactions or not.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #9)
> So basically these only count the global number of times the user interacted
> with a click, enter or select+enter. Without any hint about what he was
> doing or where.

Yes.

> I'm still not sure whether this is expected to count one-offs interactions
> or not.

Yeah, that's a reasonable question I guess.  This patch treats actions via the one-offs the same as actions via the results, which I thought was also reasonable.  So for example if you press the return key on a one-off but a non-default result index is selected, then it simply counts as a "chooseEnter" (or "enterSelection" as you suggest).  All that matters is the selected result index and how the action was initiated.
(In reply to Marco Bonardo [::mak] from comment #9)
> So basically these only count the global number of times the user interacted
> with a click, enter or select+enter. Without any hint about what he was
> doing or where.

Yes, as Drew confirmed, these are intended to be overall counts for each input type, not keyed by result index or type.

> I'm still not sure whether this is expected to count one-offs interactions
> or not.

I think since we're currently planning this to be indicative of user input preference in general, we should include one-offs in the count.
Flags: needinfo?(dzeber)
thanks, then we just need to address comment 6, apart from the "first" and "second" points.
This extends the tests that you mentioned and addresses your comments.
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

Benjamin, could you please look at the two new histograms this adds?  We want to measure how the user interacts with results in the urlbar and searchbar: whether they (a) click them, (b) type something and hit the Return key, or (c) type something, select a result, and then hit the Return key.
Attachment #8848719 - Flags: feedback?(benjamin)
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

https://reviewboard.mozilla.org/r/121626/#review128890

::: browser/components/search/content/search.xml:983
(Diff revision 2)
>              return;
>            }
>  
>            // Check for middle-click or modified clicks on the search bar
>            if (popupForSearchBar) {
> +

nit: this added newline doesn't look necessary

::: browser/modules/BrowserUsageTelemetry.jsm:538
(Diff revision 2)
> +    let histogram = Services.telemetry.getHistogramById(histogramID);
> +    let category =
> +      // command events are from the one-off context menu.  Treat them as clicks.
> +      event instanceof Ci.nsIDOMMouseEvent || event.type == "command" ? "click" :
> +      highlightedIndex >= 0 ? "enterSelection" :
> +      "enter";

Could you please refactor as (let's not be too strict about 80 chars wrap):

// command events are from the one-off context menu.  Treat them as clicks.
let isClick = event instanceof Ci.nsIDOMMouseEvent || event.type == "command";
let category = isClick ? "click"
                       : highlightedIndex >= 0 ? "enterSelection"
                                               : "enter";
Attachment #8848719 - Flags: review?(mak77) → review+
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

https://reviewboard.mozilla.org/r/121626/#review129036

::: toolkit/components/telemetry/Histograms.json:5705
(Diff revision 2)
>      "bug_numbers": [775825],
>      "description": "Firefox: The type of the selected result in the URL bar popup. See BrowserUsageTelemetry.jsm:URLBAR_SELECTED_RESULT_TYPES for the result types."
>    },
> +  "FX_URLBAR_SELECTED_RESULT_METHOD": {
> +    "alert_emails": ["adw@mozilla.org"],
> +    "expires_in_version": "never",

I really don't believe that opt-in/never is the thing you want here. Which PM/analyst will be using this data? I expect that as user-interaction-level data you want this to be opt-out, and to start out this should expire. Once the team has confidence that the data is useful we can discuss making it permanent.
Flags: needinfo?(past)
Dave Zeber will create the dashboards and Javaun Moradi will be monitoring them for product insights. They have requested this data to never expire in an attempt to support feature planning. I will ask them to comment here to confirm my understanding that they need the probes to not expire.
Flags: needinfo?(past)
Flags: needinfo?(jmoradi)
Flags: needinfo?(dzeber)
This new patch addresses your comments, Marco, and also null-checks the event since that caused failures on try.

(I happened to post this at the same time Benjamin feedback'ed the patch, but this new patch is unrelated to that.)
So does "never" also mean opt-in?  There's no never with opt-out?

Dave had already said this in bug 1334633:

(In reply to Dave Zeber [:dzeber] from bug 1334633 comment #5)
> The plan for these search probes is to start using them as standard
> instrumentation for evaluating search, so if possible I'd prefer them not to
> expire (expires in version "never"). If that's not feasible, maybe we can
> set it 10 versions out.
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

Setting f? on the new patch since Benjamin didn't clear it on the old one.  (It was my posting the new patch that cleared it.)
Attachment #8848719 - Flags: feedback?(benjamin)
The expiration and the population are independent. As written these probes don't have an opt-out marking so they are opt-in (prerelease-only) by default.

My main concern about this style of behavioral data is that we often use it for a while but then we keep it around "just in case it's valuable" in the future. Which is something we're explictly trying not to do. So I'd like to start this out for 6 months, make sure the data is useful, and then revisit whether to extend as-needed or make it actually permanent.
Yes, we need this probe (and in fact all of the new search probes tracked in bug 1334599) to be opt-out.

These probes (this one included) measure targeted interactions with the search UI. They will be used in dashboarding search engagement metrics, as well as future experimentation for UI and Awesomebar performance changes. As such, I'd like to make sure that they will be available when necessary without having to keep track of per-probe expiration versions. I'd prefer them to expire "never" from now, but if that's not possible, I'd like to set expiration at least 1 year out.
Flags: needinfo?(dzeber)
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

This is rebased on the current tree and makes these changes to the histograms json:

    "alert_emails": ["dzeber@mozilla.com"],
    "expires_in_version": "63",
    "releaseChannelCollection": "opt-out",
Attachment #8848719 - Flags: feedback?(benjamin)
Comment on attachment 8848719 [details]
Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action.

https://reviewboard.mozilla.org/r/121626/#review131596

data-r=me
Attachment #8848719 - Flags: review+
Thanks Benjamin.  Clearing the needinfo on Javaun since all questions have been resolved.
Flags: needinfo?(jmoradi)
There are some new try failures I need to figure out before landing.
Attachment #8848719 - Flags: feedback?(benjamin)
Looks like the problem is simply that the browser history added by previous tests causes a problem for the failing test.  Clearing history at the beginning fixes it locally for me.  One more try push and hopefully this will be good to go...
Try looks good, landing.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 856f71ccffb3 -d 35a4c18b0b75: rebasing 389522:856f71ccffb3 "Bug 1334615 - Add a probe to determine whether the keyboard or the mouse was used to select an action. r=bsmedberg,mak" (tip)
merging browser/components/search/content/search.xml
merging browser/modules/BrowserUsageTelemetry.jsm
merging browser/modules/test/browser/browser_UsageTelemetry_urlbar.js
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging browser/modules/test/browser/browser_UsageTelemetry_urlbar.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/telemetry/Histograms.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd703291b11a
Add a probe to determine whether the keyboard or the mouse was used to select an action. r=bsmedberg,mak
https://hg.mozilla.org/mozilla-central/rev/dd703291b11a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify+
QA Contact: brindusa.tot
Mark 54 won't fix as this is late for Beta54.
Hello guys,

I managed to test FX_URLBAR_SELECTED_RESULT_METHOD and FX_SEARCHBAR_SELECTED_RESULT_METHOD probes on Firefox 55.0b3 and on Firefox 56.0a1 (2017-06-21), under Windows 10x64.
The counting is correctly performed, but I have a question related to the distinguish of this input mechanism of interest.
Here are the there are three input mechanisms of interest applied (see Comment 3): 

- pressing <Enter> directly, leading to a default search
- clicking on a result from the dropdown
- navigating to a result from the dropdown using the arrow keys and pressing <Enter>.

If there is a difference between 'pressing <Enter> directly, leading to a default search' and  'navigating to a result from the dropdown using the arrow keys and pressing <Enter>.', wouldn't be expected to distinguish 'clicking on a result from the dropdown' from 'clicking the 'Go to the address in the location bar', or clicking on 'Search' buttons'?

At this moment 'clicking on a result from the dropdown' and 'clicking the 'Go to the address in the location bar', or clicking on 'Search' buttons' are counted as the same action.
Flags: needinfo?(adw)
Hi Mihai, thanks for bringing that up.  As far as I know, we're OK with counting those two actions as one, but it's good to know that they are counted together.
Flags: needinfo?(adw)
Based on Comment 38 and Comment 39, I'm marking this bug as Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: