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

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cirdeiliviu, Assigned: Gijs)

Tracking

({regression})

52 Branch
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52+ verified, firefox53 verified)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
[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.
Assignee

Updated

3 years ago
See Also: → 1115009
Assignee

Comment 1

3 years ago
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!)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Flags: needinfo?(florian)
Comment hidden (mozreview-request)

Comment 4

3 years ago
mozreview-review
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).
Attachment #8811224 - Flags: review?(florian) → review+
tracking as regression in 52

Comment 6

3 years ago
mozreview-review
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.
I don't know what happened with the mozreview -> bugzilla attachment flow here. Is that an r+, Nihanth?
Flags: needinfo?(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.
Flags: needinfo?(nhnt11) → needinfo?(gijskruitbosch+bugs)
Assignee

Comment 9

3 years ago
(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 hidden (mozreview-request)
Assignee

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 11

3 years ago
mozreview-review
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! :)
Attachment #8811224 - Flags: review?(nhnt11) → review+
Assignee

Comment 12

3 years ago
mozreview-review
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

3 years ago
mozreview-review-reply
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
Assignee

Comment 14

3 years ago
mozreview-review
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

3 years ago
"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
Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1123442, 1106101

Comment 16

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

Comment 17

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

3 years ago
Pushed by gijskruitbosch@gmail.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9dba33b4cd8c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee

Updated

3 years ago
Flags: qe-verify?
Assignee

Comment 20

3 years ago
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.
Attachment #8811224 - Flags: approval-mozilla-aurora?
It would be good to have this verified prior to uplift, see comment 0.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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
Attachment #8811224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 25

3 years ago
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
Flags: needinfo?(gijskruitbosch+bugs)
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]
Assignee

Updated

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