1899352 - Update BrowserUsageTelemetry for vertical tab events

Status: RESOLVED FIXED

(Firefox :: Sidebar, task, P2)

Description

[Sarah Clements [:sclements]]

We'll want to differentiate between horizontal and vertical tabstrip events in BrowserUsageTelemetry. The events that DS is interested in is, although if there's anything else that could be useful for tab/tab context menu usage that would also be good:

tabs_context
tab_open_event_count
max_concurrent_tab_count
max_concurrent_tab_pinned_count
tab_pinned_event_count

Edit: tabs_bar is the tabs-toolbar and we won't have that element in vertical tabs mode so we'll need to register a new one.

Comment 1

[Jonathan Sudiaman [:jsudiaman]]

I'm a bit confused on how we want to differentiate between horizontal and vertical tabstrip events. Should vertical events use a new probe name? Or Glean instead of Telemetry? etc.

Comment 2

[Sarah Clements [:sclements]]

(In reply to Jonathan Sudiaman [:jsudiaman] from comment #1)

I'm a bit confused on how we want to differentiate between horizontal and vertical tabstrip events. Should vertical events use a new probe name? Or Glean instead of Telemetry? etc.

We're just going to update the existing legacy telemetry to add a new scalar (so adding a "vertical_tab" for all those events, ie browser_ui_interaction_vertical_tabs_context for tab context menu events which are registered here.)

This is where the probe string gets concatenated is here. Probably helpful to add a breakpoint there in the devtools debugger to check various tab interactions.

For determining when to report events as "vertical_tab" you could check for the sidebar.verticalTab prefs or potentially tabContainer.verticalMode (if a node points to the parent container).

I edited the list slightly because I realized the "tabs-bar" doesn't apply with vertical tabs - we'll need to register a new area so its either "tabs-bar" or "vertical-tabs-container" and this only seems relevant for use of the close tab button from what I can see. If this starts to get complicated you could spin this one off to another bug.

Comment 3

[Jonathan Sudiaman [:jsudiaman]]

Attachment: Bug 1899352 - Update BrowserUsageTelemetry for vertical tab events r?#sidebar-reviewers

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97a51d1d23ba
Update BrowserUsageTelemetry for vertical tab events r=sidebar-reviewers,Gijs,nsharpley

Comment 5

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/97a51d1d23ba

Comment 6

[:Gijs (he/him)]

I think I missed something when reviewing this.

https://hg.mozilla.org/mozilla-central/rev/97a51d1d23ba48ea4f2c913a451c140b9a586d7c#l2.78

--- a/browser/modules/BrowserUsageTelemetry.sys.mjs
+++ b/browser/modules/BrowserUsageTelemetry.sys.mjs
@@ -80,16 +109,17 @@ const UI_TARGET_COMPOSED_ELEMENTS_MAP = 
 
 // The containers of interactive elements that we care about and their pretty
 // names. These should be listed in order of most-specific to least-specific,
 // when iterating JavaScript will guarantee that ordering and so we will find
 // the most specific area first.
 const BROWSER_UI_CONTAINER_IDS = {
   "toolbar-menubar": "menu-bar",
   TabsToolbar: "tabs-bar",
+  "vertical-tabs": "vertical-tabs-container",
   PersonalToolbar: "bookmarks-bar",
   "appMenu-popup": "app-menu",
   tabContextMenu: "tabs-context",
   contentAreaContextMenu: "content-context",
   "widget-overflow-list": "overflow-menu",
   "widget-overflow-fixed-list": "pinned-overflow-menu",
   "page-action-buttons": "pageaction-urlbar",
   pageActionPanel: "pageaction-panel",

These containers are meant to correspond to telemetry scalars. See e.g. this comparable patch which added the extensions panel. So I think this needs a follow-up, either for the area to be removed (if we don't care about the interactions) or for the scalar to be added, in order for the interactions with buttons in this area to be recorded in telemetry (and, in that case, to add an automated test).

Comment 7

[Jonathan Sudiaman [:jsudiaman]]

(In reply to :Gijs (he/him) from comment #6)

I think I missed something when reviewing this.

https://hg.mozilla.org/mozilla-central/rev/97a51d1d23ba48ea4f2c913a451c140b9a586d7c#l2.78

--- a/browser/modules/BrowserUsageTelemetry.sys.mjs
+++ b/browser/modules/BrowserUsageTelemetry.sys.mjs
@@ -80,16 +109,17 @@ const UI_TARGET_COMPOSED_ELEMENTS_MAP = 
 
 // The containers of interactive elements that we care about and their pretty
 // names. These should be listed in order of most-specific to least-specific,
 // when iterating JavaScript will guarantee that ordering and so we will find
 // the most specific area first.
 const BROWSER_UI_CONTAINER_IDS = {
   "toolbar-menubar": "menu-bar",
   TabsToolbar: "tabs-bar",
+  "vertical-tabs": "vertical-tabs-container",
   PersonalToolbar: "bookmarks-bar",
   "appMenu-popup": "app-menu",
   tabContextMenu: "tabs-context",
   contentAreaContextMenu: "content-context",
   "widget-overflow-list": "overflow-menu",
   "widget-overflow-fixed-list": "pinned-overflow-menu",
   "page-action-buttons": "pageaction-urlbar",
   pageActionPanel: "pageaction-panel",

These containers are meant to correspond to telemetry scalars. See e.g. this comparable patch which added the extensions panel. So I think this needs a follow-up, either for the area to be removed (if we don't care about the interactions) or for the scalar to be added, in order for the interactions with buttons in this area to be recorded in telemetry (and, in that case, to add an automated test).

This was added for the special case of handling tabs_context from the vertical tabs bar. We want to track the entrypoint for the tabs context menu commands (e.g. right click tab -> click "New Tab Below"), and it will only track vertical tabs container if it is added to BROWSER_UI_CONTAINER_IDS. My patch had a test for this as well:

info("Open a new tab using the context menu.");
await openAndWaitForContextMenu(contextMenu, gBrowser.selectedTab, () => {
  document.getElementById("context_openANewTab").click();
});

const keyedScalars = TelemetryTestUtils.getProcessScalars("parent", true);
TelemetryTestUtils.assertKeyedScalar(
  keyedScalars,
  "browser.ui.interaction.tabs_context_entrypoint",
  "vertical-tabs-container",
  1
);

If we still want to document vertical-tabs-container and add it as a scalar, or handle this a different way somehow, I'm happy to do that in a follow-up bug.

Comment 8

[:Gijs (he/him)]

OK, here's the issue I'm seeing, to be a bit clearer:

1. open new profile
2. open about:telemetry
3. search for tabs-newtab-button - no hits
4. click new tab button
5. go back to about:telemetry and refresh the page

Now you see a keyed scalar for the tabstrip (browser.ui.interaction.tabs_bar) that has incremented tabs-newtab-button to 1. Repeated clicks would increase the number further (of course).

6. switch to vertical tabs
7. click new tab button again (in vertical tabs toolbar)
8. go back to about:telemetry and refresh the page

Nothing has changed.

9. open browser console

there's an error message:

browser.ui.interaction.vertical_tabs_container - Unknown scalar.

I think we should (a) not have the warning and (b) we probably want to make sure that we still track clicks on tabstrip widgets when they happen in the vertical tabs container somehow.

Comment 9

[Jonathan Sudiaman [:jsudiaman]]

Got it, thanks. Created Bug 1921789 to address this.