1316863 - Middle-clicking on a result/oneoff from Search bar closes the search box

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Liviu Cirdei]

[Affected versions]:
Nightly 52.0a1 Build ID 20161110030211
 
[Affected platforms]:
Ubuntu 16.04 x64, Windows 10 x64, Mac OS X 10.10

[Steps to reproduce]:
1. Type something in Search bar.
2. Middle click on a result or oneoff from dropdown.
 
[Expected result]:
The search result is opened in a new tab and the search box remains open.

[Actual result]:
Search is made in a new tab (ok) but the search box is closed. 

[Regression]:
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9d6b5736f9c8feee1b11cce37e3b7b6e5c7c116f&tochange=d5a021318c9a4393b9ff40da7110daf88522aa24

Probably regressed by bug 1313403.

Comment 1

[:Gijs (he/him)]

I can't reproduce on OS X using the context menu, only with cmd-click. I expect there's some kind of click handling that gets triggered via the descendant that we're clicking, but doesn't trigger for the context menu. I'll try and look at it in more detail next week. Florian, I don't suppose you have ideas off-hand ("no" is a valid answer!)

Comment 2

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #1)
> Florian, I don't suppose
> you have ideas off-hand ("no" is a valid answer!)

No idea without looking at the code unfortunately, sorry.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

Review commit: https://reviewboard.mozilla.org/r/93414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93414/

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review93464

Removing the hidePopup call seems reasonable to me, so I'm r+'ing but I would like us to wait to hear from nhnt11 too before this lands.

I tested the edge case mentioned in the removed comment and in bug 1110771, but the behavior I see is that the first click closes the context menu without triggering a search, and it's only the second click that triggers a search (and that correctly closes the panel, both with and without the explicit hidePopup call).

Comment 5

[Julien Cristau [:jcristau]]

tracking as regression in 52

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review96396

::: browser/components/search/content/search.xml
(Diff revision 1)
> -        // in the current tab - so we hide it manually. Some focusing magic
> -        // that happens when a search is loaded ensures that the popup is opened
> -        // again if it needs to be, so we don't need to worry about which cases
> -        // require manual hiding.
> -        this.popup.hidePopup();
> -

Indeed, the original issue that this was trying to solve seems not to exist anymore.

FWIW, it appears that this patch fixes the bug for one-off buttons but middle clicking a search result still causes the popup to close.

Sorry for the late review.

Comment 7

[Andrew Overholt [:overholt]]

I don't know what happened with the mozreview -> bugzilla attachment flow here. Is that an r+, Nihanth?

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Andrew Overholt [:overholt] from comment #7)
> I don't know what happened with the mozreview -> bugzilla attachment flow
> here. Is that an r+, Nihanth?

No, it's not, because middle clicking a result still closed the popup when I tested the patch.

Comment 9

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #8)
> (In reply to Andrew Overholt [:overholt] from comment #7)
> > I don't know what happened with the mozreview -> bugzilla attachment flow
> > here. Is that an r+, Nihanth?
> 
> No, it's not, because middle clicking a result still closed the popup when I
> tested the patch.

It would have made sense to clear review or mark r- in that case... I'll try and look before Hawaii but I might not be able to.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93414/diff/1-2/

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review97616

Thanks! :)

Comment 12

[:Gijs (he/him)]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review97624

::: browser/components/search/content/search.xml:993
(Diff revision 2)
>  
> +            // leave the popup open for background tab loads
> +            if (!(where == "tab" && params.inBackground)) {
> +              // close the autocomplete popup and revert the entered search term
> +              this.closePopup();
> +              controller.handleEscape();

Thinking about it, the `handleEscape` should probably be outside the `if` statement.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review97624

> Thinking about it, the `handleEscape` should probably be outside the `if` statement.

Why? That actually breaks it for me, probably because nsAutoCompleteController::HandleEscape also calls ClosePopup(): https://dxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#375

Comment 14

[:Gijs (he/him)]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

https://reviewboard.mozilla.org/r/93414/#review97698

::: browser/components/search/content/search.xml:993
(Diff revision 2)
>  
> +            // leave the popup open for background tab loads
> +            if (!(where == "tab" && params.inBackground)) {
> +              // close the autocomplete popup and revert the entered search term
> +              this.closePopup();
> +              controller.handleEscape();

OK. The reason is that we'd want to make sure that the search field's contents are still reset (ie don't just get set to the autocomplete entry's content) when opening a background tab with the suggested value.

Comment 15

[arni2033]

"Actual result" described in comment 0 is not a regression. It's expected behavior that was regressed long ago in bug 1106101 (see bug 1115009) and recently fixed in bug 1313403.
For many years searchbar was working consistent and nice, until UX team and Florian Queze decided to break it beyond repair (If they didn't, numerous searchbar regressions would be fixed now).
Ever since then, it didn't work corectly. Old searchbar behaved as follows:
- Middle-click on suggestion opens search in a new selected tab, and suggestions list is closed
- Shift+Middle-click on suggestion opens search in a new background tab, and suggestions are closed
"Expected result" is inconsistent with urlbar behavior and searchbar behavior on good old Firefox 28
(or 4). It's also inconsistent with other popup menus that allow user to open links in background tabs, such as bookmarks panel and history panel (because they hide after Shift+Middle-click).
After fixing bug 1123442 (it's about time?) "Expected result" will be impossible anyway.
So please don't waste your efforts: there're many real, long-standing bugs/regressions out there.

Actually, there IS a way to make it all good and consistent: to keep all said popup menus open when user opens link in background tab. I can't really tell how useful is keeping popup menu open, because
I never tested such behavior, and also nobody (in bug 1106101; in this bug, before making decision) investigated how often users open the same menu after opening a link from that menu.

The original report (comment 0) can only be explained by the fact that some new users already started
relying on the buggy behavior of Release version (even though suggestions list was blinking these ~1.5
years (bug 1115009) and was really annoying for the eyes). That's a really bad sign for developers IMO

Comment 16

[fireattack [:fireattack]]

Since we're on this topic, can we fix 1123442? It has been almost 2 years.

Comment 17

[:Gijs (he/him)]

(In reply to arni2033 from comment #15)
> "Actual result" described in comment 0 is not a regression.

It is if you expected the search popup to be open after opening the link. It's annoying that it 'blinked' before, and that should be fixed after this patch. The popup will remain open if and only if you open an item in a new background tab (so not for new foreground tabs) which seems like the correct thing to do.

(In reply to arni2033 from comment #15)
> Actually, there IS a way to make it all good and consistent: to keep all
> said popup menus open when user opens link in background tab.

That's what we're doing in this bug, for the search popup and for search completion items in the URL bar. It isn't feasible to change the history / bookmarks popups right now, but I would consider taking a patch in a separate bug.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dba33b4cd8c
stop manually closing the search popup to solve issues that seem to not exist anymore, r=florian,nhnt11

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9dba33b4cd8c

Comment 20

[:Gijs (he/him)]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1313403
[User impact if declined]: middle clicking some items in the search bar autocomplete popup closes the popup
[Is this code covered by automated tests?]: there are general tests for the feature, but not for this specific aspect
[Has the fix been verified in Nightly?]: nope, but I set qe-verify?
[Needs manual test from QE? If yes, steps to reproduce]: probably should. Steps are in comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: fairly low-risk
[Why is the change risky/not risky?]: 3 of us have looked at the code by now, so we're pretty sure it should be OK, and there's reasonable test coverage of the search field in general so it seems unlikely we've catastrophically broken anything.
[String changes made/needed]: nope.

Comment 21

[Julien Cristau [:jcristau]]

It would be good to have this verified prior to uplift, see comment 0.

Comment 22

[ovidiu boca[:Ovidiu]]

Tested with FF Nighlty 53.0a1(2016-12-16) on Mac OS X 10.10 and Ubuntu 16.04 x64 and with FF Nighlty 53.0a1(2016-12-15) on Windows 10 x64, and I can confirm the fix.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8811224 [details]
Bug 1316863 - stop manually closing the search popup to solve issues that seem to not exist anymore,

don't close search popup when opening background tab, for aurora52

Comment 24

[Carsten Book [:Tomcat]]

needs rebasing

Comment 25

[:Gijs (he/him)]

It... would have been nice to majorly move/refactor these bindings *before* ESR branched instead of straight after, but oh well. :-(

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/315219009f706d386fb26430e72eaa6e947bce1d

Comment 26

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-11-11) (64-bit) on Windows 7, 64 Bit !

This bug's fix is verified with latest Developer Edition (Aurora)

Build ID   :  20161228004005
User Agent :  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
[bugday-20161228]