947991 - BrowserUITelemetry should take note of which mouse button was used to click an item

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We have an event listener set on all of the toolbars to listen for mouseup events so that we can count clicks on the default toolbar items.  We are, however, not filtering by the mouse button being used to click the button.

I believe UX / UR are only interested in left-click counting for now, so we should ignore all other clicks.

Comment 1

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

Redirecting a bit - we're thinking of recording which mouse button was used to click each button.

Comment 2

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

I don't think I'm going to have time to land this on Holly before the uplift, so requesting tracking.

Comment 3

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

This work was completed in bug 944114 for mozilla-central - I'll create a patch to port it over to Holly / Aurora.

Comment 4

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

Attachment: Patch v1

Comment 5

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

Attachment: Patch v1.1

Ok, this seems to do the right thing.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8345522 [details] [diff] [review]
Patch v1.1

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

This looks fine, but see below nits

::: browser/modules/BrowserUITelemetry.jsm
@@ +287,5 @@
> +    let countObject = this._ensureObjectChain([aCategory, aAction], 0);
> +    countObject[aAction]++;
> +  },
> +
> +  _countMouseUpEvent: function(aCategory, aAction, aMouseUpEvent) {

Nit: I think I'd prefer if this passed aMouseButton (aEvent.button in the callsites)

@@ +431,5 @@
>    },
>  
>    _starButtonMouseUp: function(aEvent) {
>      let starButton = aEvent.originalTarget;
>      if (starButton.hasAttribute("starred")) {

Not sure how I missed this, but can you just make this do:

let action = starButton.hasAttribute("starred") ? "edit" : "add";
this._countMouseUpEvent("click-star-button", action, aEvent.button); // see above for why aEvent.button rather than aEvent

Rather than the if/else ? :-)

Comment 7

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

Sure, I can do that. And I'll push a follow-up to make the same change on mozilla-central as well.

Comment 8

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

Attachment: Patch v1.2 (r+'d by Gijs)

Thanks Gijs - done (and landed a follow-up patch on mozilla-central - see https://bugzilla.mozilla.org/show_bug.cgi?id=944114#c23).

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

Comment 9

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

Comment on attachment 8345576 [details] [diff] [review]
Patch v1.2 (r+'d by Gijs)

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

None. This just gives BrowserUITelemetry more specificity in terms of click counting, in that it records which button was used to click an item.


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 10

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

Comment on attachment 8345576 [details] [diff] [review]
Patch v1.2 (r+'d by Gijs)

We'll want this on Aurora as well - the risks are the same for Beta.

Comment 11

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

Removing Australis:P1 whiteboard tag because these already block an Australis:P1 and being redundant isn't helpful.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

no need to track, go ahead and uplift.

Comment 13

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

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

Comment 14

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

Landed in mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/2f2832819e88