Closed Bug 1102937 Opened 10 years ago Closed 10 years ago

Add UITelemetry for the improved search bar UI.

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 37
Iteration:
37.3 - 12 Jan

People

(Reporter: bwinton, Assigned: bwinton)

References

Details

Attachments

(1 file, 6 obsolete files)

A version that passes our test suite…
12.32 KB, patch
bwinton
: review+
Details | Diff | Splinter Review
Now that we're getting a much fancier search bar UI, it would be nice to know if users are actually using it! :) The specific questions I have are: How many people select a suggestion? Which suggestion result have they selected? (As in "first, second, third…" not "what was the text of the suggestion." :) How many people choose a different search engine from the list at the bottom? Which search engine do they select? (Either "first, second, third…" or "what is the name of the engine".) How many people select the "Change Search Settings" footer? These questions also apply to the updated home page and newtab page.
Flags: firefox-backlog+
(In reply to Blake Winton (:bwinton) from comment #0) > Which suggestion result have they selected? > (As in "first, second, third…" not "what was the text of the suggestion." > :) I'm guessing you also meant "was the selected suggestion coming from search history or from the search engine?".
Do the searchbars show history? I thought they were only showing suggestions for some reason… If they show history, then yes, it would be nice to know "first history, second history, …" or "first suggestion, second suggestion, …"
(In reply to Blake Winton (:bwinton) from comment #2) > Do the searchbars show history? Yes. > I thought they were only showing suggestions for some reason… Probably because of bug 1101996.
As a note, we're already tracking clicks on alternative search engines as "ddg.searchbar" (for instance), so this should tell us how the choice of search engine differs before and after this change… (Also, I'm looking into implementing the rest of this, and am remembering how confusing our search autocomplete code is. ;)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Flags: qe-verify?
Hi Blake, can you provide a point value.
Flags: needinfo?(bwinton)
Sure! 5 points, because of the autocomplete/search interactions. :) Also, qe-verify+, and I'll put instructions on how to do that in a later comment.
Points: --- → 5
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(bwinton)
Attached patch The first version of the patch. (obsolete) — — Splinter Review
Hey Matt, feel free to redirect this to someone else if you feel they're better equipped to handle it. I'm hoping to get this landed by Friday-ish and uplifted, so that I don't have to work over the Christmas holidays… :) Thanks!
Attachment #8536763 - Flags: review?(MattN+bmo)
Comment on attachment 8536763 [details] [diff] [review] The first version of the patch. Review of attachment 8536763 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/urlbarBindings.xml @@ +1212,5 @@ > Ci.nsISearchEngine.DATA_XML, > target.getAttribute("src"), false, > installCallback); > + } else if (target.classList.contains("search-setting-button")) { > + BrowserUITelemetry.countSearchSettingsEvent("searchbar"); It would be nice for this to be with the `openPreferences('paneSearch')` call to make it easier to understand what's going on. I would prefer if you used the @anonid as well as it's more of an ID and therefore less likely to change. ::: toolkit/content/widgets/autocomplete.xml @@ +486,5 @@ > #endif > this.mEnterEvent = aEvent; > + if (BrowserSearch.searchBar._textbox === this && > + this.mController.selection.currentIndex != -1) { > + BrowserSearch.searchBar.selection = { This doesn't look a telemetry change and I don't understand what it's doing.
Attachment #8536763 - Flags: review?(MattN+bmo) → feedback+
Attached patch The second version of the patch. (obsolete) — — Splinter Review
(In reply to Matthew N. [:MattN] from comment #8) > ::: browser/base/content/urlbarBindings.xml > > + } else if (target.classList.contains("search-setting-button")) { > > + BrowserUITelemetry.countSearchSettingsEvent("searchbar"); > > It would be nice for this to be with the `openPreferences('paneSearch')` > call to make it easier to understand what's going on. I would prefer if you > used the @anonid as well as it's more of an ID and therefore less likely to > change. Did you mean something like this? - oncommand="openPreferences('paneSearch')" + oncommand="BrowserUITelemetry.countSearchSettingsEvent('searchbar');openPreferences('paneSearch')" or were you thinking I should remove the oncommand attribute from the button, and handle the uitelemetry and the openPreferences in the command event handler? > ::: toolkit/content/widgets/autocomplete.xml > @@ +486,5 @@ > > #endif > > this.mEnterEvent = aEvent; > > + if (BrowserSearch.searchBar._textbox === this && > > + this.mController.selection.currentIndex != -1) { > > + BrowserSearch.searchBar.selection = { > > This doesn't look a telemetry change and I don't understand what it's doing. It's setting the kind of selection so that the searchbar can tell BrowserUITelemetry to log that (at http://mxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#575 ) I needed to do it this way because the searchBar has no idea that it's been autocompleted by the time it gets to recording the search event at http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#510 so this is a way of saving that state.)
Attachment #8536763 - Attachment is obsolete: true
Attachment #8537823 - Flags: review?(MattN+bmo)
I'm not familiar enough with BrowserSearch.searchBar.selection and I need to fix 2 Hello UITour bugs ASAP so if you could find another reviewer for today, that would be good.
Comment on attachment 8537823 [details] [diff] [review] The second version of the patch. Review of attachment 8537823 [details] [diff] [review]: ----------------------------------------------------------------- Unless I'm missing something, this seems to only register: - if the search was triggered from the keyboard vs search from the mouse. - if the user used a suggestion, and which one. I suspect you want to save way more data than that. Eg. has the user clicked a one-off button? Has the user clicked the header (that works since bug 1110235)? Has the user highlighted a one-off button from the keyboard and used it? It seems to me the only UX question I could answer based on the data collected by this patch whether having 10 items in the autocomplete panel is useful or if we could reduce that to eg. 6 (or really have data about which number would make the most sense). I'm sure with more data we could answer plenty of other things. ::: browser/base/content/urlbarBindings.xml @@ +927,3 @@ > if (aEvent.button == 0 && !aEvent.shiftKey && !aEvent.ctrlKey && > !aEvent.altKey && !aEvent.metaKey) { > + searchBar.selection = { Are you sure this code is actually called when clicking a one-click engine button? This binding is the one of the old search UI. The binding of the new UI inherits from it, so I expect this to be called when clicking a suggestion, but I'm not so sure it's called when we trigger a search from this code path: https://hg.mozilla.org/mozilla-central/annotate/addf2aa154fb/browser/base/content/urlbarBindings.xml#l1208 ::: browser/components/search/content/search.xml @@ +508,5 @@ > let engine = aEngine || this.currentEngine; > var submission = engine.getSubmission(aData, null, "searchbar"); > + let selection = this.selection; > + this.selection = null; > + BrowserSearch.recordSearchInHealthReport(engine, "searchbar", this.selection); Did you mean selection rather than this.selection? ::: toolkit/content/widgets/autocomplete.xml @@ +486,5 @@ > #endif > this.mEnterEvent = aEvent; > + if (BrowserSearch.searchBar._textbox === this && > + this.mController.selection.currentIndex != -1) { > + BrowserSearch.searchBar.selection = { searchBar.selection is something that didn't exist before your patch, right? If so, "selection" is probably too generic, it would be nice to use a name that shows the purpose of that value; I mean some identifier that I can mxr, or that will make me figure out quickly that this is saved for telemetry purpose and doesn't do anything else.
Attachment #8537823 - Flags: review?(MattN+bmo) → review-
Attached patch The third version of the patch. (obsolete) — — Splinter Review
(In reply to Florian Quèze [:florian] [:flo] from comment #11) > Unless I'm missing something, this seems to only register: > - if the search was triggered from the keyboard vs search from the mouse. > - if the user used a suggestion, and which one. > > I suspect you want to save way more data than that. Eg. has the user clicked > a one-off button? I believe this will be covered because we're already tracking clicks on alternative search engines as "ddg.searchbar" (for instance). Although that might merge in people who have chosen, say, Google as their alternative default search engine. Drats. Added. > ::: browser/base/content/urlbarBindings.xml > @@ +927,3 @@ > > if (aEvent.button == 0 && !aEvent.shiftKey && !aEvent.ctrlKey && > > !aEvent.altKey && !aEvent.metaKey) { > > + searchBar.selection = { > Are you sure this code is actually called when clicking a one-click engine > button? Perhaps not. > This binding is the one of the old search UI. The binding of the new UI > inherits from it, so I expect this to be called when clicking a suggestion, > but I'm not so sure it's called when we trigger a search from this code > path: > https://hg.mozilla.org/mozilla-central/annotate/addf2aa154fb/browser/base/ > content/urlbarBindings.xml#l1208 Yeah, changed. > ::: browser/components/search/content/search.xml > @@ +508,5 @@ > > let engine = aEngine || this.currentEngine; > > var submission = engine.getSubmission(aData, null, "searchbar"); > > + let selection = this.selection; > > + this.selection = null; > > + BrowserSearch.recordSearchInHealthReport(engine, "searchbar", this.selection); > Did you mean selection rather than this.selection? Curses, yes. Fixed! > ::: toolkit/content/widgets/autocomplete.xml > @@ +486,5 @@ > > #endif > > this.mEnterEvent = aEvent; > > + if (BrowserSearch.searchBar._textbox === this && > > + this.mController.selection.currentIndex != -1) { > > + BrowserSearch.searchBar.selection = { > searchBar.selection is something that didn't exist before your patch, right? > If so, "selection" is probably too generic, it would be nice to use a name > that shows the purpose of that value; I mean some identifier that I can mxr, > or that will make me figure out quickly that this is saved for telemetry > purpose and doesn't do anything else. Changed to "telemetrySearchDetails". :) Thanks, Blake.
Attachment #8537823 - Attachment is obsolete: true
Attachment #8539540 - Flags: review?(florian)
Can we record the default search provider at startup? That value affects, for example, what one-off features are available to the user, allowing us to see how often they get used relative to how frequently they're available. r
Can we record the default search provider at startup? That value affects, for example, what one-off features are available to the user, allowing us to see how often they get used relative to how frequently they're available.
I think some of the points in comment 0 are going to be hard to interpret unless we also know the default search engine. Are people who switch back to Google more likely to use the search bar or (conversely) are people who don't switch more likely to use the one click options? Can we add some kind of thing that we can use to correlate the telemetry data here with default search engine?
This isn't exactly at startup, but should be close enough…
Attachment #8539540 - Attachment is obsolete: true
Attachment #8539540 - Flags: review?(florian)
Attachment #8539735 - Flags: review?(florian)
Comment on attachment 8539735 [details] [diff] [review] A version of the patch which also logs default (and current) search engine. Review of attachment 8539735 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/BrowserUITelemetry.jsm @@ +553,5 @@ > result.visibleTabs = visibleTabs; > result.hiddenTabs = hiddenTabs; > > + result.currentSearchEngine = Services.search.currentEngine; > + result.defaultSearchEngine = Services.search.defaultEngine; These two values are always the same thing on desktop Firefox since bug 860560. See also bug 1101491.
Comment on attachment 8539735 [details] [diff] [review] A version of the patch which also logs default (and current) search engine. Review of attachment 8539735 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks reasonable overall. I'm not very familiar with telemetry so if someone who is could have another look I would appreciate it, but if that's not possible in the near future I'm ok with r+'ing the next version myself. ::: browser/base/content/urlbarBindings.xml @@ +924,5 @@ > > // Check for unmodified left-click, and use default behavior > + var searchBar = BrowserSearch.searchBar; > + searchBar.telemetrySearchDetails = { > + selection: { Why do you need this "selection" object? telemetrySearchDetails doesn't seem to ever contain anything else, can't telemetrySearchDetails just directly contain the index and kind properties? You could also rename to telemetrySearchSelection if you want to keep the "selection" word somewhere to match the parameter of recordSearchInHealthReport. ::: browser/components/search/content/search.xml @@ +543,5 @@ > let engine = aEngine || this.currentEngine; > var submission = engine.getSubmission(aData, null, "searchbar"); > + let telemetrySearchDetails = this.telemetrySearchDetails || {}; > + this.telemetrySearchDetails = null; > + BrowserSearch.recordSearchInHealthReport(engine, "searchbar", telemetrySearchDetails.selection); telemetrySearchDetails.selection here will trigger a warning of using an undefined property if telemetrySearchDetail is just {}. My suggestion above would avoid this. ::: browser/modules/BrowserUITelemetry.jsm @@ +553,5 @@ > result.visibleTabs = visibleTabs; > result.hiddenTabs = hiddenTabs; > > + result.currentSearchEngine = Services.search.currentEngine; > + result.defaultSearchEngine = Services.search.defaultEngine; Can you please explain what this does that isn't already covered or remove it if the answer is "nothing"?
Attachment #8539735 - Flags: review?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #18) > The patch looks reasonable overall. I'm not very familiar with telemetry so > if someone who is could have another look I would appreciate it, but if > that's not possible in the near future I'm ok with r+'ing the next version > myself. Felipe? :) > ::: browser/base/content/urlbarBindings.xml > @@ +924,5 @@ > > + searchBar.telemetrySearchDetails = { > > + selection: { > Why do you need this "selection" object? telemetrySearchDetails doesn't seem > to ever contain anything else, can't telemetrySearchDetails just directly > contain the index and kind properties? Yeah, I was thinking I would pass in extra data there, but I ended up not doing that, so I've removed the selection sub-object and moved its properties into the telemetrySearchDetails object. > ::: browser/modules/BrowserUITelemetry.jsm > @@ +553,5 @@ > > + result.currentSearchEngine = Services.search.currentEngine; > > + result.defaultSearchEngine = Services.search.defaultEngine; > Can you please explain what this does that isn't already covered or remove > it if the answer is "nothing"? Removed!
Attachment #8539735 - Attachment is obsolete: true
Attachment #8540293 - Flags: review?(felipc)
Comment on attachment 8540293 [details] [diff] [review] The next version of the patch, with Florian's comments implemented. Review of attachment 8540293 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, but the usage of BrowserSearch in toolkit/widgets/autocomplete.xml is problematic because it would add a dependency of browser (BrowserSearch) in toolkit. Is there anywhere else that you could use to hook up that information?
Attachment #8540293 - Flags: review?(felipc) → feedback+
Attached patch A better version of the patch. (obsolete) — — Splinter Review
Okay, let me know what you'ld like changed in this version of the patch! :)
Attachment #8540293 - Attachment is obsolete: true
Attachment #8540360 - Flags: review?(felipc)
Iteration: 37.2 → 37.3
Comment on attachment 8540360 [details] [diff] [review] A better version of the patch. Review of attachment 8540360 [details] [diff] [review]: ----------------------------------------------------------------- r+, just see the two comments below: Can you rename throughout the kind "key" to be "keyboard"? ::: browser/components/search/content/search.xml @@ +924,5 @@ > let engine; > let oneOff = this.getSelectedOneOff(); > if (oneOff) > engine = oneOff.engine; > + if (this.mEnterEvent && this._selectionDetails && why do you check here for mEnterEvent? Don't you want this data recorded for mouse clicks too?
Attachment #8540360 - Flags: review?(felipc) → review+
(Just popping in to say that I'll probably be fixing these next week when I'm back at work. And thank you for the review!)
(In reply to :Felipe Gomes from comment #22) > Can you rename throughout the kind "key" to be "keyboard"? I totally agree that "keyboard" is better than "key", but changing it here would make it different from the code in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js#183 Would you like me to change that code as well, or can I leave it as "key" here to be consistent for now, and file a follow-up bug to change both of them later? > ::: browser/components/search/content/search.xml > @@ +924,5 @@ > > let engine; > > let oneOff = this.getSelectedOneOff(); > > if (oneOff) > > engine = oneOff.engine; > > + if (this.mEnterEvent && this._selectionDetails && > > why do you check here for mEnterEvent? Don't you want this data recorded for > mouse clicks too? The mouse clicks go through another code path to record this data (and can't really go through this code path, because the data we need is lost by the time we get here, if I remember correctly).
Flags: needinfo?(felipc)
Since "key" is already used somewhere and collecting data, you can leave it named as "key" here too.
Flags: needinfo?(felipc)
Keywords: checkin-needed
Try link? :)
Keywords: checkin-needed
(While I was here, I also updated the patch to cleanly apply to the latest revision. Thanks Ryan!)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/368f0748e882
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Steps to verify: For all steps below, the choice of search engine will change the engine name in the event. For the suggestion steps below, the choice of index will change the number in the event. Note: it seems to take a while for the events to show up in about:telemetry. ------------------------ Select a search suggestion with the mouse. Should log a "search" > "selection" > "searchbar" > "1" > "mouse" event. Select a search suggestion with the arrows and enter key. Should log a "search" > "selection" > "searchbar" > "3" > "key" event. Select a search engine with the mouse. Should log a "search-oneoff" > "google.oneoff" > "mouse" > "current" event. Select a search engine with the mouse while holding down ctrl/cmd. Should log a "search-oneoff" > "duckduckgo.oneoff" > "mouse" > "tab-background" event. Select a search engine with the enter key. Should log a "search-oneoff" > "bing.oneoff" > "key" > "current" event. Select a search engine with the enter key while holding down alt. Should log a "search-oneoff" > "twitter.oneoff" > "key" > "tab" event. Select the header with the mouse. Should log a "search-oneoff" > "yahoo.header" > "mouse" > "current" event. Select the header with the mouse while holding down ctrl/cmd. Should log a "search-oneoff" > "yahoo.header" > "mouse" > "tab-background" event. Select the "Change Search Settings" footer with the mouse. Should log a "click-builtin-item" > "searchbar" > "search-settings" event. ------------------------
QA Contact: petruta.rasa
Depends on: 1119597
Depends on: 1120389
(In reply to Blake Winton (:bwinton) from comment #31) > Note: it seems to take a while for the events to show up in about:telemetry. Blake, how long should I wait until I can see the results? So far I tried 2014-01-11 Nightly 37.0a1 builds under Win 7 64-bit and Mac OSX 10.9.5, but on about:telemetry I only see what search engine was used to perform a search and not all the events you described. I watched Keyed Histrograms section from about:telemetry. Thanks :)
Petruta, so, it's kind of complicated. ;) All of these events should show up in the Simple Measures section (not the Keyed Histograms section), but that section was broken by this patch. It got fixed in bug 1119327, which landed on fx-team over the weekend, but hasn't been merged back into mozilla-central yet. Hopefully that'll happen today, but if you follow that bug you should be able to see exactly when it gets merged. Sorry about that!
Depends on: 1120957
Blocks: 1121067
Blake, please see below my results. I used Firefox Developer Edition 37.0a2 2015-01-14 under Win 7 64-bit. The first and the sixth lines (put a star before the lines) below look different from the expected results. The last one also it's a little different. Could you please take a look, I'm not so familiar with telemetry data. Thank you! (In reply to Blake Winton (:bwinton) from comment #31) > Select a search suggestion with the mouse. > Should log a "search" > "selection" > "searchbar" > "1" > "mouse" event. * "search":{"searchbar":1} > Select a search suggestion with the arrows and enter key. > Should log a "search" > "selection" > "searchbar" > "3" > "key" event. "search":{"searchbar":2,"selection":{"searchbar":{"3":{"key":1}}}} > Select a search engine with the mouse. > Should log a "search-oneoff" > "google.oneoff" > "mouse" > "current" event. "search-oneoff":{"google.oneoff":{"mouse":{"current":1}}} > Select a search engine with the mouse while holding down ctrl/cmd. > Should log a "search-oneoff" > "duckduckgo.oneoff" > "mouse" > > "tab-background" event. "search-oneoff":{"ddg.oneoff":{"mouse":{"tab-background":1}}} > Select a search engine with the enter key. > Should log a "search-oneoff" > "bing.oneoff" > "key" > "current" event. "search-oneoff":{"bing.oneoff":{"key":{"current":1}}} > Select a search engine with the enter key while holding down alt. > Should log a "search-oneoff" > "twitter.oneoff" > "key" > "tab" event. * "search-oneoff":{"twitter.oneoff":{"key":{"current":1}}} > Select the header with the mouse. > Should log a "search-oneoff" > "yahoo.header" > "mouse" > "current" event. "search-oneoff":{"yahoo.header":{"mouse":{"current":1}}} > Select the header with the mouse while holding down ctrl/cmd. > Should log a "search-oneoff" > "yahoo.header" > "mouse" > "tab-background" > event. "ddg.header":{"mouse":{"tab-background":1}}} > Select the "Change Search Settings" footer with the mouse. > Should log a "click-builtin-item" > "searchbar" > "search-settings" event. "click-builtin-item":{"tabbrowser-tabs":{"left":2},"searchbar":{"left":2,"search-settings":1},"urlbar-reload-button":{"left":1}}
Depends on: 1120236
Petruta: Yeah, I can confirm it's not logging the right stuff on Windows. Could you file a followup bug, and cc me on it?
Blake, I logged bug 1123657 for all OSs. Indeed, the first two are different on Windows, but the results on all platforms are not the same as expected. I believe it's ok to mark this bug as verified. Please modify the status, if not.
Status: RESOLVED → VERIFIED
Seems great to me! Thank you very much, Petruta!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: