944145 - Collect UITelemetry on default toolbar button usage

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

In particular, we'd like to:

* Count the number of times the bookmarks menu button is opened
* Count the number of times a bookmark is opened from the bookmarks menu button

This applies to both pre and post-Australis.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

We're interested in collecting click counts on all default toolbar buttons that might move into the panel for Australis.

This does not include buttons provided by add-ons - only the built-in buttons.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8341882 [details] [diff] [review]
Patch v1

Curious to know what you think about my expando property hack, Gijs...

I'm trying to make it easier for us to detect things like clicks on the star-button without incrementing the click count on the urlbar-container - so if a child handles the event, mark the event so that we ignore it while it bubbles up. Seems probably safer than trying to prevent the event from bubbling, which might mess up add-ons or other bits of code.

Is there a better way of doing what I'm trying to do?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8341882 [details] [diff] [review]
Patch v1

Review of attachment 8341882 [details] [diff] [review]:
-----------------------------------------------------------------

I would try to add a listener to the container. If that's too complex or has downsides I've not figured out yet, this approach could work, too, but I'd like to try that first.

::: browser/modules/BrowserUITelemetry.jsm
@@ +235,5 @@
>      }
> +
> +    for (let itemID of ALL_BUILTIN_ITEMS) {
> +      // Attempt to get the item in the document, and if it's not there,
> +      // check the palette. getElementsByAttribute("id", someID)[0] will

Nit: s/someID/itemID/

@@ +239,5 @@
> +      // check the palette. getElementsByAttribute("id", someID)[0] will
> +      // be undefined if no such element exists with id someID.
> +      let item =
> +        document.getElementById(itemID) ||
> +        aWindow.gNavToolbox.palette.getElementsByAttribute("id", itemID)[0];

Nit: intermediate for the palette, then keep the assignment spread over 2 lines instead of 3.

@@ +241,5 @@
> +      let item =
> +        document.getElementById(itemID) ||
> +        aWindow.gNavToolbox.palette.getElementsByAttribute("id", itemID)[0];
> +
> +      if (item) {

So, rather than adding an event listener to each individual item, why don't you add a listener to the container toolbar/panel? You can then check the originalTarget (and if necessary, walk up from there) to figure out for which item the event fired, and you'll be sure that you catch each event exactly once.

You'd need to walk the ancestor chain until you find the toolbar or the menu panel, and then keep a list of which containers you've already added a listener to, but that seems somewhat less hacky than the expando, and will also mean the number of event listeners will be lower and the code simpler.

If you add them with the system event listener service, I think you will even be able to get these if items catch the mouseup event themselves and stopPropagate/preventDefault it.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Yes, putting the mouseup event listeners on the toolbars was a way, way better idea.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8343100 [details] [diff] [review]
Patch v2

Review of attachment 8343100 [details] [diff] [review]:
-----------------------------------------------------------------

This looks alright but I'm concerned about the direct id check vs. a loop. Can you assuage my concerns or am I right here? :-)

::: browser/modules/BrowserUITelemetry.jsm
@@ +278,5 @@
>  
>    _handleMouseUp: function(aEvent) {
> +    let targetID = aEvent.currentTarget.id;
> +
> +    switch(targetID) {

Nit: space before the (

@@ +285,5 @@
> +        break;
> +      default:
> +        // Perhaps we're seeing one of the default toolbar items
> +        // being clicked.
> +        if (ALL_BUILTIN_ITEMS.indexOf(aEvent.originalTarget.id) != -1) {

Are you sure this works? I suspect that the originalTarget might also be a descendant of the item, e.g. with the stuff inside the URL bar, or with the elements inside the search bar, etc.

I suspect you need a loop here to walk the ancestor chain up to the toolbar (at which point, if we've not got anything, we should give up). Am I just wrong? (could be!)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #6)
> Are you sure this works? I suspect that the originalTarget might also be a
> descendant of the item, e.g. with the stuff inside the URL bar, or with the
> elements inside the search bar, etc.

So right now, ALL_BUILTIN_ITEMS includes IDs for the items in the URL bar... haven't added any for the search input, since at this point UX hasn't expressed interest in it (plus, the children use classes instead of IDs... *sigh*). So I think I agree that we should do the loop up to the toolbar if our first look in ALL_BUILTIN_ITEMS turns up negative.

I'll update the patch.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8343100 [details] [diff] [review]
Patch v2

Review of attachment 8343100 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing this for now per comment 7

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2.1

So now after we get a mouseup event on one of the toolbars, we first check the ALL_BUILTIN_ITEMS array for the item ID. If it's not in the array, we check for the first ID'd ancestor (up to the toolbar), and see if that result is in the ALL_BUILTIN_ITEMS.

If neither of those things yields results, we bail out.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2.2 - for non-Australis

Fixed silly typo.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8343156 [details] [diff] [review]
Patch v2.2 - for non-Australis

Review of attachment 8343156 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: browser/modules/BrowserUITelemetry.jsm
@@ +320,5 @@
>  
>      return "unrecognized";
>    },
>  
> +  _checkForBuiltinItem: function(aEvent) {

Nit: you use aEvent.originalTarget several times, and in one case it causes the line to wrap onto the next line. If you stick it in an intermediate (say, 'item', seeing as that's the term we're using (prefixed) in the rest of the variable names), then you can save some redundancy and unwrap the wrapped line.

@@ +334,5 @@
> +    let toolbarID = aEvent.currentTarget.id;
> +    // If not, we need to check if one of the ancestors of the clicked
> +    // item is in our list of built-in items to check.
> +    let candidate = getIDBasedOnFirstIDedAncestor(aEvent.originalTarget,
> +                                                  toolbarID);

Hrm. This will work assuming there's no nested IDed things we care about. Technically, the search-container has an id and then the searchbar element has one, too, but I suspect it'll be almost impossible to hit the latter but not the former (and certainly not if you actually want to click into the focusable part of it). I don't *think* there's any others.

Note that regarding the search bar and URL bar, maybe we want to catch ctrl+l/ctrl-k as well? That can be a followup, though.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8343156 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8343156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM!
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +320,5 @@
> >  
> >      return "unrecognized";
> >    },
> >  
> > +  _checkForBuiltinItem: function(aEvent) {
> 
> Nit: you use aEvent.originalTarget several times, and in one case it causes
> the line to wrap onto the next line. If you stick it in an intermediate
> (say, 'item', seeing as that's the term we're using (prefixed) in the rest
> of the variable names), then you can save some redundancy and unwrap the
> wrapped line.
> 

Good idea. I'll do that before I land.

> @@ +334,5 @@
> > +    let toolbarID = aEvent.currentTarget.id;
> > +    // If not, we need to check if one of the ancestors of the clicked
> > +    // item is in our list of built-in items to check.
> > +    let candidate = getIDBasedOnFirstIDedAncestor(aEvent.originalTarget,
> > +                                                  toolbarID);
> 
> Hrm. This will work assuming there's no nested IDed things we care about.
> Technically, the search-container has an id and then the searchbar element
> has one, too, but I suspect it'll be almost impossible to hit the latter but
> not the former (and certainly not if you actually want to click into the
> focusable part of it). I don't *think* there's any others.

Good point. I don't think there are any others either, but I'll keep an eye out.

> 
> Note that regarding the search bar and URL bar, maybe we want to catch
> ctrl+l/ctrl-k as well? That can be a followup, though.

I will indeed file that follow-up.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Landed on Holly as https://hg.mozilla.org/projects/holly/rev/bd9075540b41

Filed follow-up bug about Ctrl-L / Ctrl-K as bug 946977.

Leaving open since we'll put the Australis version of this measurement in here too.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8343156 [details] [diff] [review]
Patch v2.2 - for non-Australis

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

None. This just gives BrowserUITelemetry the capability of counting click events on the whitelisted set of built-in toolbar items on customizable toolbars.


User impact if declined: 

None.


Testing completed (on m-c, etc.): 

Lots of manual testing on Holly, which has since merged to Aurora.


Risk to taking this patch (and alternatives if risky): 

None.


String or IDL/UUID changes made by this patch:

None.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/bb07aea479a8

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for Australis

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350354 [details] [diff] [review]
Patch v1 - for Australis

Review of attachment 8350354 [details] [diff] [review]:
-----------------------------------------------------------------

Thoughts on this, jaws?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8350354 [details] [diff] [review]
Patch v1 - for Australis

Review of attachment 8350354 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/BrowserUITelemetry.jsm
@@ +114,5 @@
> +    "copy-button",
> +    "paste-button",
> +    "zoom-out-button",
> +    "zoom-reset-button",
> +    "zoom-in-button",

Do you need to add the bookmarks-menu-button's toolbarbutton child and the dropmarker child?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #19)
> Comment on attachment 8350354 [details] [diff] [review]
> Patch v1 - for Australis
> 
> Review of attachment 8350354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +114,5 @@
> > +    "copy-button",
> > +    "paste-button",
> > +    "zoom-out-button",
> > +    "zoom-reset-button",
> > +    "zoom-in-button",
> 
> Do you need to add the bookmarks-menu-button's toolbarbutton child and the
> dropmarker child?

Thanks jaws! No, we don't need those because the bookmarks-menu-button is fully special-cased via _bookmarksMenuButtonMouseUp.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Landed on fx-team as https://hg.mozilla.org/integration/fx-team/rev/ffc142af3ca9

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ffc142af3ca9