Update BrowserUsageTelemetry for vertical tab events
Categories
(Firefox :: Sidebar, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox132 | --- | fixed |
People
(Reporter: sclements, Assigned: jsudiaman)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fidefe-sidebar])
Attachments
(1 file)
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.
Updated•10 months ago
|
Updated•6 months ago
|
Assignee | ||
Comment 1•6 months ago
|
||
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.
Reporter | ||
Comment 2•6 months ago
|
||
(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.
Assignee | ||
Updated•6 months ago
|
Assignee | ||
Comment 3•6 months ago
|
||
Comment 5•6 months ago
|
||
bugherder |
Comment 6•5 months ago
|
||
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).
Assignee | ||
Comment 7•5 months ago
•
|
||
(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•5 months ago
•
|
||
OK, here's the issue I'm seeing, to be a bit clearer:
- open new profile
- open
about:telemetry
- search for
tabs-newtab-button
- no hits - click new tab button
- 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).
- switch to vertical tabs
- click new tab button again (in vertical tabs toolbar)
- go back to about:telemetry and refresh the page
Nothing has changed.
- 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.
Assignee | ||
Comment 9•5 months ago
|
||
Got it, thanks. Created Bug 1921789 to address this.
Description
•