Closed Bug 1357523 Opened 6 years ago Closed 6 years ago

When searching using the field on about:newtab with alternative search engine, Firefox searches highlighted suggestion instead of typed text

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Version:
52 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox55 --- affected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: upwinxp, Assigned: prathiksha)

References

Details

(Keywords: ux-control, Whiteboard: [fxsearch])

Attachments

(2 files)

Although I have typed "krava", a search for "krvavec" is performed
311.40 KB, image/gif
Details
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.
59 bytes, text/x-review-board-request
nhnt11
: review+
ritu
: approval-mozilla-beta+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

1. Open the new tab page (about:newtab).
2. Type something into the search box on the page. Suggestions should appear below.
3. Hover the mouse cursor over a suggestion so that it becomes highlighted (but not copied to the search box).
4. Select one of the non-default search engines at the bottom of the dropdown menu.


Actual results:

Firefox searches for the highlighted suggestion instead of text typed into the address bar. (see the attached GIF)


Expected results:

The search query should be what user has typed into the box. (The search bar in the toolbar has no problems.)
confirmed on Nightly55
Status: UNCONFIRMED → NEW
status-firefox55: --- → affected
Component: Untriaged → New Tab Page
Ever confirmed: true
This bug can be reproduced after the landing of bug 1171344.
Blocks: 1171344
Keywords: ux-control
Component: New Tab Page → Search
seems bug 1196677 did not fix anything
Blocks: 1196677
Flags: needinfo?(nhnt11)
The in-content results panel needs the same treatment as the URL and search bar panels from bug 1295458.
Flags: needinfo?(nhnt11)
Priority: -- → P2
Whiteboard: [fxsearch]
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Attachment #8903513 - Flags: feedback?(nhnt11)
Attachment #8903513 - Flags: review?(nhnt11)
Comment on attachment 8903513 [details]
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.

https://reviewboard.mozilla.org/r/175350/#review181606

Thanks for looking at this!

While we should definitely land this simple fix, I want to note that the one-off search UI has evolved since it was implemented for in-content search: hovering the mouse on a suggestion does not mark it selected. Porting that to in-content search is relevant to this issue I think, in that it at least reduces confusion around the interaction.

::: browser/base/content/contentSearchUI.js:251
(Diff revision 3)
>      let searchText = this.input;
>      let searchTerms;
>      if (this._table.hidden ||
>          aEvent.originalTarget.id == "contentSearchDefaultEngineHeader" ||
> -        aEvent instanceof KeyboardEvent) {
> +        aEvent instanceof KeyboardEvent ||
> +        this._engines.map((engine) => { return engine.name; }).includes(aEvent.originalTarget.title)) {

Let's avoid the map() here: we have direct access to the names of the default engine as well as the engine we're going to be using so we can just compare those (see the selectedEngineName getter).
Attachment #8903513 - Flags: review?(nhnt11)
Comment on attachment 8903513 [details]
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.

https://reviewboard.mozilla.org/r/175350/#review187366

Thanks for the patch! Behavior is now more similar to the main search bar, so that's good.

Just some minor comments, but we can't r+ and land this yet: the behavior changes are almost certainly going to break some tests! Let's do a try push to get an idea of how bad the breakage is. I don't know if you've worked with try server or mochitests before, so you may have some questions! Feel free to contact me.

::: browser/base/content/contentSearchUI.js
(Diff revision 4)
>  
>      let allElts = [...this._suggestionsList.children,
>                     ...this._oneOffButtons,
>                     document.getElementById("contentSearchSettingsButton")];
> -    // If we are selecting a suggestion and a one-off is selected, don't deselect it.
> +
> -    let excludeIndex = idx < this.numSuggestions && this.selectedButtonIndex > -1 ?

Hmm, I was going to say this excludeIndex business was intentional to allow keyboard users to select a suggestion and a one-off engine independently, but it seems that your patch actually matches the current behavior in the main search bar, so never mind!

::: browser/base/content/contentSearchUI.js:250
(Diff revision 4)
>      let searchTerms;
>      if (this._table.hidden ||
>          (aEvent.originalTarget &&
>            aEvent.originalTarget.id == "contentSearchDefaultEngineHeader") ||
> -        aEvent instanceof KeyboardEvent) {
> +        aEvent instanceof KeyboardEvent ||
> +        aEvent.originalTarget.title == this.selectedEngineName) {

Actually, it seems like we don't need this condition anymore, with the other behavioral changes in this patch - as long as we keep the || searchText.value that you removed a couple of lines down.

One can argue that this condition avoids the cost of calling this.suggestionAtIndex() and null-checking the return value, but I think the code readability benefit of avoiding this condition outweighs that by far.

::: browser/base/content/contentSearchUI.js:253
(Diff revision 4)
>            aEvent.originalTarget.id == "contentSearchDefaultEngineHeader") ||
> -        aEvent instanceof KeyboardEvent) {
> +        aEvent instanceof KeyboardEvent ||
> +        aEvent.originalTarget.title == this.selectedEngineName) {
>        searchTerms = searchText.value;
>      } else {
> -      searchTerms = this.suggestionAtIndex(this.selectedIndex) || searchText.value;
> +      searchTerms = this.suggestionAtIndex(this.selectedIndex);

Please see what I said above about keeping this.
Attachment #8903513 - Flags: review?(nhnt11)
Comment on attachment 8903513 [details]
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.

https://reviewboard.mozilla.org/r/175350/#review192862

Thanks, LGTM!

::: browser/base/content/test/general/browser_contentSearchUI.js:513
(Diff revision 5)
>  
>    await promiseTab();
>    await setUp();
>  
>    // Test selecting a suggestion, then clicking a one-off without deselecting the
> -  // suggestion.
> +  // suggestion, with buttons.

I think you meant, with the keyboard? "with buttons" is ambiguous :)
Attachment #8903513 - Flags: review?(nhnt11) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e029590936c5
Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines. r=nhnt11
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e029590936c5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Verified fixed using the latest Nightly (2017-10-18) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
Comment on attachment 8903513 [details]
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1171344
[User impact if declined]: about:home and about:newtab search will behave unexpectedly when we search using non-default search engines.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: changes are well tested
[String changes made/needed]: no
Attachment #8903513 - Flags: approval-mozilla-beta?
Comment on attachment 8903513 [details]
Bug 1357523 - Make about:home and about:newtab search box search for the value in the search textbox when we use non-default search engines.

While this isn't a new regression, it's certainly a core scenario, one-line low risk fix, Beta57+
Attachment #8903513 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Reproduced the initial issue using old Nightly from 2017-04-26, verified that the issue is fixed using Firefox 57 beta 12 across platforms (Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit).
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.