Closed Bug 1817208 Opened 1 year ago Closed 1 year ago

Sometimes selected_result is unknown even if there's results

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

VERIFIED FIXED
113 Branch
Tracking Status
firefox112 --- verified
firefox113 --- verified

People

(Reporter: mak, Assigned: daisuke)

References

Details

Attachments

(1 file, 1 obsolete file)

I see events where selected_result is unknown, but results contain autofill_origin or search_engine.
There is a string typed.
engagement_type is enter.

I found a possible case where this happens, if the user types a string and then moves with the keyboard to a search shortcut, and confirms with SHIFT+ENTER (skip search mode) then we record unknown as selected_result, we should probably annotate the user has picked a shortcut button

Another case is typing a search, pressing ESC to close the results panel, then confirm the search with Enter, should return input_field as selected_result, instead it returns unknown.
Also why is selected_result not empty if there's no selected result? unknown should only be used when there is a selected result but we can't recognize it, maybe there's a catch-all setting to unknown even if there isn't a selected result.

Assignee: nobody → daisuke
Status: NEW → ASSIGNED

Thank you very much!

(In reply to Marco Bonardo [:mak] from comment #1)

I found a possible case where this happens, if the user types a string and then moves with the keyboard to a search shortcut, and confirms with SHIFT+ENTER (skip search mode) then we record unknown as selected_result, we should probably annotate the user has picked a shortcut button

Yes, I also could reproduce this. In the above case, as no element is selected, such is the record.
The easiest way to fix it might be that selects the first row (I think this behavior is the same as selecting the first row) if view.visibleResults is not empty, at below.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarSearchOneOffs.sys.mjs#213
But, selecting a row changes the UI (very short time though), so it might not be right way. Or, we may be able to just use the first row as the selected result, before taking the telemetry.

And also, one thing. How about the following STR?

  1. Set 0 to browser.urlbar.maxRichResults.
  2. Seach with @google mozilla.

The result of the telemetry now is:

search_mode:"", 
n_results:0, 
selected_result: "input_field",
provider: null,

In this case, actually, we show a temporary result created below.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarInput.sys.mjs#713-723
I think we better take the above result, what do you think?

(In reply to Marco Bonardo [:mak] from comment #2)

Another case is typing a search, pressing ESC to close the results panel, then confirm the search with Enter, should return input_field as selected_result, instead it returns unknown.

I also could reproduce this as well.
It seems that the reason is that even if we close the results by ESC, the view.visibleResults still remain. So, we may need to check view.isOpen first here.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarController.sys.mjs#1096

Also why is selected_result not empty if there's no selected result? unknown should only be used when there is a selected result but we can't recognize it, maybe there's a catch-all setting to unknown even if there isn't a selected result.

This happens since the results still remain, I think.

Flags: needinfo?(mak)

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

(In reply to Marco Bonardo [:mak] from comment #2)

Another case is typing a search, pressing ESC to close the results panel, then confirm the search with Enter, should return input_field as selected_result, instead it returns unknown.

I also could reproduce this as well.
It seems that the reason is that even if we close the results by ESC, the view.visibleResults still remain. So, we may need to check view.isOpen first here.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarController.sys.mjs#1096

Ah, no, it is not so simple. Since the results will be closed before here in most cases.
Perhaps, we may need to reset visibleResults when closing by ESC.

(In reply to Daisuke Akatsuka (:daisuke) from comment #4)

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

(In reply to Marco Bonardo [:mak] from comment #2)

Another case is typing a search, pressing ESC to close the results panel, then confirm the search with Enter, should return input_field as selected_result, instead it returns unknown.

I also could reproduce this as well.
It seems that the reason is that even if we close the results by ESC, the view.visibleResults still remain. So, we may need to check view.isOpen first here.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarController.sys.mjs#1096

Ah, no, it is not so simple. Since the results will be closed before here in most cases.
Perhaps, we may need to reset visibleResults when closing by ESC.

Or, we may better clear it when Enter and if the view is closed.

clearing stuff in the context object is not a good idea, we risk to break other consumers.
It'd be better to detect the case when we collect telemetry.

Flags: needinfo?(mak)

Yes, thanks.

How about this case?

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

And also, one thing. How about the following STR?

  1. Set 0 to browser.urlbar.maxRichResults.
  2. Seach with @google mozilla.

The result of the telemetry now is:

search_mode:"", 
n_results:0, 
selected_result: "input_field",
provider: null,

In this case, actually, we show a temporary result created below.
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/browser/components/urlbar/UrlbarInput.sys.mjs#713-723
I think we better take the above result, what do you think?

Should we add search_mode at least?

Flags: needinfo?(mak)
Attachment #9322044 - Attachment description: Bug 1817208: Clear UrlbarView::visibleResults when closing results if needed → Bug 1817208: Check wheather the view is opening when recording

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

And also, one thing. How about the following STR?

  1. Set 0 to browser.urlbar.maxRichResults.
  2. Seach with @google mozilla.

The result of the telemetry now is:

search_mode:"", 
n_results:0, 
selected_result: "input_field",
provider: null,

For correctness we can set search_mode yes, though that is a low priority, because to have meaningful analysis I'd suggest to exclude users who set maxRichResults = 0. That's a really edge case. Of course if the fix is simple it won't hurt.

Flags: needinfo?(mak)
Attachment #9322293 - Attachment description: Bug 1817208: Record appropriate suggestion when immediate search under search mode without selected row → Bug 1817208: Introduce search_shortcut_button type for immediate search on oneoff button
Blocks: 1822210

Comment on attachment 9322044 [details]
Bug 1817208: Check wheather the view is opening when recording

Revision D172098 was moved to bug 1822210. Setting attachment 9322044 [details] to obsolete.

Attachment #9322044 - Attachment is obsolete: true
Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f49451cff74
Introduce search_shortcut_button type for immediate search on oneoff button r=mak
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

I've managed to reproduce the issue using an Fx 112.0a1 from 2023-03-06.
The issue is verified fixed using the latest Fx 113.0a1 on Windows 10 and Ubuntu 22.04. The selected_result for the engagement telemetry ping is now "search_shortcut_button" when the user performs the search using shift+enter with a search shortcut selected.

Status: RESOLVED → VERIFIED

uplift to 112, just in case we want to measure there

Flags: needinfo?(mak)
Flags: needinfo?(mak) → needinfo?(daisuke)

Comment on attachment 9322293 [details]
Bug 1817208: Introduce search_shortcut_button type for immediate search on oneoff button

Beta/Release Uplift Approval Request

  • User impact if declined: This change is for telemetry. So, no user impact.
    But, we want to know how the user uses the urlbar and the results more correctly to improve the user experience.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This affects only telemetry.
  • String changes made/needed: None
  • Is Android affected?: Unknown
Flags: needinfo?(daisuke)
Attachment #9322293 - Flags: approval-mozilla-beta?

Comment on attachment 9322293 [details]
Bug 1817208: Introduce search_shortcut_button type for immediate search on oneoff button

Approved for 112.0b4

Attachment #9322293 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

The issue is verified fixed using the latest Fx 112.0b5 on Windows 10 and Ubuntu 22. The selected_result for the engagement telemetry ping is now "search_shortcut_button" when the user performs the search using shift+enter with a search shortcut selected.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: