Closed Bug 1257843 Opened 8 years ago Closed 8 years ago

Probe to measure when user enters text selection UI

Categories

(Firefox for Android Graveyard :: Metrics, defect)

defect
Not set
normal

Tracking

(firefox48 fixed, fennec47+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
fennec 47+ ---

People

(Reporter: barbara, Assigned: capella)

References

Details

Attachments

(1 file, 2 obsolete files)

This probe will help us measure usage of this feature and will let us compare it once with the new UI, once https://bugzilla.mozilla.org/show_bug.cgi?id=1171110 lands.
Sorry, maybe a bit confusing, this _should_ land before 1171110 lands, so we have something to compare to.
We should write a simple probe for this and uplift it to 47 to compare before/after the new text selection action bar ships.
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → 47+
Actually, I'm not super familiar with this logic anymore.

Capella or Sebastian, could one of you help pick this up? I think we just need to create an event when we show the text selection UI, maybe something like:

("show.1", "content", "text_selection")

Barbara, does that make sense to you for an event here?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(markcapella)
Flags: needinfo?(bbermes)
Sounds good, makes sense
Flags: needinfo?(bbermes)
Yeah, I can take this one.

Note that the new UI is only available on Android 6+. So to compare old vs. new UI we can only look at users with Android 6 or higher.

Are there any specific questions we try to answer with that probe? For the new UI we are "just" following the Android platform and are not necessarily trying to improve the feature itself. But it's good to have telemetry here anyways.

The "show" telemetry is actually a bit tricky. We are showing/hiding the floating toolbar all the time while scrolling, touching the handles etc. But I guess we want to treat this as just one "show" event here (so that we can compare it to the old implementation).
Assignee: margaret.leibovic → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Flags: needinfo?(markcapella)
ActionBarHandler was designed to trigger "TextSelection:ActionbarInit" on start of a new "logical" selection, with (visibility state changes / selection updates ) occuring as needed until "TextSelection:ActionbarUninit".

Wouldn't that initial message serve the purpose here?
(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> Yeah, I can take this one.
> 
> Note that the new UI is only available on Android 6+. So to compare old vs.
> new UI we can only look at users with Android 6 or higher.
> 
> Are there any specific questions we try to answer with that probe? For the
> new UI we are "just" following the Android platform and are not necessarily
> trying to improve the feature itself. But it's good to have telemetry here
> anyways.
> 
> The "show" telemetry is actually a bit tricky. We are showing/hiding the
> floating toolbar all the time while scrolling, touching the handles etc. But
> I guess we want to treat this as just one "show" event here (so that we can
> compare it to the old implementation).

I know this blocks bug 1171110, but I think Barbara was actually interested in text selection UI overall. So interested in knowing how much text selection usage changes before/after the gecko carets change as well.

I was thinking we would put this probe somewhere like `startSelection`.
Attached patch bug1257843.diff (obsolete) — Splinter Review
The issue there is the old Java |SelectionHandler.startSelection()/.attachCaret()| triggers the ActionBar UI in TextSelection.java, and |ActionBarHandler._init()| triggers either ActionBar UI in TextSelection.java or FloatingToolbar UI in FloatingToolbar.java.

My thought (if I can jump in on sebastian) was to catch all three at "Init" message like the attached.
Attached patch bug1257843.diff (obsolete) — Splinter Review
Sorry, quick grabbed wrong WIP
Attachment #8744300 - Attachment is obsolete: true
Assignee: s.kaspari → markcapella
Comment on attachment 8744301 [details] [diff] [review]
bug1257843.diff

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

::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java
@@ +153,5 @@
>              public void run() {
>                  try {
>                      if (event.equals("TextSelection:ShowHandles")) {
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.SHOW,
> +                            TelemetryContract.Method.CONTENT, "actionbar_textSelection_java");

Why do we need this here too?

Also what's up with the "_java" suffix?

::: mobile/android/base/java/org/mozilla/gecko/text/FloatingToolbarTextSelection.java
@@ +116,5 @@
>  
>      private void handleOnMainThread(final String event, final JSONObject message) {
>          if ("TextSelection:ActionbarInit".equals(event)) {
> +            Telemetry.sendUIEvent(TelemetryContract.Event.SHOW,
> +                TelemetryContract.Method.CONTENT, "floatingtoolbar_textSelection");

I wonder if we need two different extras. We could just filter by Android version, I guess.
Attachment #8744301 - Flags: feedback+
Comment on attachment 8744301 [details] [diff] [review]
bug1257843.diff

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

::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java
@@ +213,5 @@
>                      } else if (event.equals("TextSelection:ActionbarInit")) {
>                          // Init / Open the action bar. Note the current selectionID,
>                          // cancel any pending actionBar close.
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.SHOW,
> +                            TelemetryContract.Method.CONTENT, "actionbar_textSelection");

We'll need an adapted patch to get this into 47 too.
Attached patch bug1257843.diffSplinter Review
This gets us both both ActionBar and FloatingToolbar UI's associated with AccessibleCarets which we should be able to get into 48 today.

The |actionbar_textSelection_java| intent was just a placeholder, denoting the previous "blocky-orange" java/PanZoomController based layerviews associated with the ActionBar. Let me know how you'd like that defined instead.

Once m-c/48 moves to aurora, I'll post the adapted patch with the corrected placeholder for beta/47.
Attachment #8744301 - Attachment is obsolete: true
Attachment #8744325 - Flags: review?(s.kaspari)
Comment on attachment 8744325 [details] [diff] [review]
bug1257843.diff

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

::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java
@@ +210,5 @@
>                      } else if (event.equals("TextSelection:ActionbarInit")) {
>                          // Init / Open the action bar. Note the current selectionID,
>                          // cancel any pending actionBar close.
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.SHOW,
> +                            TelemetryContract.Method.CONTENT, "actionbar_textSelection");

Talked to barbara and margaret on IRC: We should just use "text_selection" for both. We can filter by Android version if we want to look at the different implementations.
Attachment #8744325 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/21c9846c176b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Follow up ping
Flags: needinfo?(markcapella)
Filed: Bug 1267383 - Followup Telemetry from Bug 1257843, Text Selection Probe
Flags: needinfo?(markcapella)
Can we request uplift for 47? We'd like to get data for before the text selection UI changes.
Flags: needinfo?(markcapella)
With bug 1257843, I think everything is currently in place / available. Beta v47 should now be reporting on TextSelection as implemented in Release v46 (ugly/block-y orange carets).

Aurora v48 should now be providing the same Telemetry for the new AccessibleCarets, either via ActionBar or by FloatingToolbar.
Flags: needinfo?(markcapella)
(In reply to Mark Capella [:capella] from comment #20)
> With bug 1257843, I think everything is currently in place / available. Beta
> v47 should now be reporting on TextSelection as implemented in Release v46
> (ugly/block-y orange carets).

I'm a bit confused, you're referring to this bug here. Which only landed in 48.
Bah, I'm sorry. I should just have referred you to comment 18 above ... the followup bug was 
Bug 1267383 - Followup Telemetry from Bug 1257843, Text Selection Probe
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.