Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Firefox 64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 months ago
In the urlbarBindings in handleCommand, we have this code:

```

// Determine whether to use the selected one-off search button.  In
// one-off search buttons parlance, "selected" means that the button
// has been navigated to via the keyboard.  So we want to use it if
// the triggering event is not a mouse click -- i.e., it's a Return
// key -- or if the one-off was mouse-clicked.
let selectedOneOff = this.popup.oneOffSearchButtons.selectedButton;
if (selectedOneOff &&
    isMouseEvent &&
    event.originalTarget != selectedOneOff) {
  selectedOneOff = null;
}

// Do the command of the selected one-off if it's not an engine.
if (selectedOneOff && !selectedOneOff.engine) {
  selectedOneOff.doCommand();
  return;
}

```

I've done some digging around and I just can't see how that second part is triggered - the preferences button on the one-offs bar doesn't get to the handleCommand method, and the right-click menu is doesn't trigger it either.

Try also shows that mochitests pass without it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b

Therefore, I think it can be removed, which will make the code slightly simpler and make it easier to figure out what to do with the new address bar implementation.

I'll attach a patch, Drew please let me know if you think I'm wrong.
The second part is hit when you press the return key while the preferences button is selected.
(In reply to Mark Banner (:standard8) from comment #0)
> Try also shows that mochitests pass without it:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b

It's possible we don't have a test that specifically synthesizes an enter keypress on the settings button.
(Assignee)

Comment 4

7 months ago
(In reply to Drew Willcoxon :adw from comment #3)
> (In reply to Mark Banner (:standard8) from comment #0)
> > Try also shows that mochitests pass without it:
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b
> 
> It's possible we don't have a test that specifically synthesizes an enter
> keypress on the settings button.

It certainly looks like it.

Do you know why we add a telemetry result to "FX_URLBAR_SELECTED_RESULT_METHOD" for category "enter" when this happens? We don't appear to do that for a click.

Could there be something else this is handling which would need to be logged as well? The other one-off buttons don't hit this case.

If we just need to re-route the command, and not log telemetry, this would make it easier for the newer urlbar design.
Flags: needinfo?(adw)
The fact that telemetry is recorded when you hit the return key on the settings button is just an accident I think, due to the fact that the return keypress gets routed through handleCommand.  I don't think we really need to record that.
Flags: needinfo?(adw)
(Assignee)

Comment 6

7 months ago
Ok, lets mutate this into not logging that telemetry, and I'll also add test coverage for the settings button.
Summary: Remove probably unnecessary search command fallback from urlbarBindings.xml handleCommands → Stop logging telemetry when clicking the one-off search settings button
(Assignee)

Updated

7 months ago
Summary: Stop logging telemetry when clicking the one-off search settings button → Stop logging telemetry when pressing enter on the one-off search settings button
(Assignee)

Updated

7 months ago
Summary: Stop logging telemetry when pressing enter on the one-off search settings button → Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar
Attachment #9014813 - Attachment is obsolete: true
(Assignee)

Comment 7

7 months ago
Also add tests for the search settings button in the urlbar.

Comment 8

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/107392a74e8a
Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar. r=adw

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/107392a74e8a
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Updated

7 months ago
Blocks: 1497904
You need to log in before you can comment on or make changes to this bug.