Closed Bug 1103455 Opened 9 years ago Closed 9 years ago

Context menu click on search box opens suggestions

Categories

(Firefox :: Search, defect)

35 Branch
x86_64
All
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox33 --- unaffected
firefox34 - wontfix
firefox35 + verified
firefox36 + verified
firefox37 + verified
firefox-esr31 --- unaffected

People

(Reporter: alice0775, Assigned: mossop)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Attached image screenshot
[Tracking Requested - why for this release]: bug regression 

STR

1. Right click on SearchBar

Actual Results
See attached screenshot
STR:
1. Type something in the searchbox
2. Move the focus to somewhere else (eg. the location bar)
3. Right click the searchbox.

Expected result:
Opens the context menu.

Actual result:
Opens the context menu first and then the suggestion panel above it.
This is wontfix for Firefox 34. I have tracked for 35+.
Bug 1105259 adds that right click on the magnifying glass (search icon) located in the search bar does the same as reported here.
Flags: firefox-backlog? → firefox-backlog+
OS: Windows 7 → All
Summary: glich of context menu of Searchbar → Context menu click on search box opens suggestions
Assignee: nobody → dtownsend
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
There are two pieces here:

For right clicking the magnifying glass icon, we just have to only open the suggestions when clicking with the left mouse button. Quite simple.

For right clicking the text the problem is a little harder. Right clicking focuses the text box and the focus handler opens the suggestions if there is any text there. I don't see a way to get to the original event that caused a focus so this just wallpapers over it by making us ignore the next focus event after a mousedown of the right mouse button.

Before I go writing tests can you think of a better option here florian?
Attachment #8536891 - Flags: feedback?(florian)
Comment on attachment 8536891 [details] [diff] [review]
patch

(In reply to Dave Townsend [:mossop] from comment #11)

> I don't see a way to get to the original event that caused a
> focus so this just wallpapers over it by making us ignore the next focus
> event after a mousedown of the right mouse button.

There's another case where we would like to be able to get to the original event that triggered the focus so that we can ignore it:
1. Focus a non-empty searchbar (you'll have the suggestion panel open)
2. Focus another window.
3. Focus the browser window again, eg. by clicking in the text field in the middle of about:home

Surprise! When the browser window gets focused, a focus event is triggered on the node that had focus before, and we reopen the suggestion panel :-(.

If you could find an easy way to extend your fix to also address that case, that would be awesome. If not, no worries, I think there's another bug on file for that case.

> Before I go writing tests can you think of a better option here florian?

I don't see anything obviously better. Please include a comment somewhere in the code explaining why we ignore that focus event :).
Attachment #8536891 - Flags: feedback?(florian) → feedback+
In what circumstance do we actually want the suggestions panel to pop up on focus? I tend to just find that behaviour really annoying. Why can't we remove it outright? :-)
(In reply to :Gijs Kruitbosch from comment #13)
> In what circumstance do we actually want the suggestions panel to pop up on
> focus? I tend to just find that behaviour really annoying. Why can't we
> remove it outright? :-)

This is a behavior Philipp explicitly requested, so I guess you want to ask him.

My guess is that the case where we really want the panel to open is when the user clicks a non-empty searchbox.
Flags: needinfo?(philipp)
(In reply to :Gijs Kruitbosch from comment #13)
> In what circumstance do we actually want the suggestions panel to pop up on
> focus? I tend to just find that behaviour really annoying. Why can't we
> remove it outright? :-)

yes, it should be remeved, it caused Bug 1103457.
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> There's another case where we would like to be able to get to the original
> event that triggered the focus so that we can ignore it:
> 1. Focus a non-empty searchbar (you'll have the suggestion panel open)
> 2. Focus another window.
> 3. Focus the browser window again, eg. by clicking in the text field in the
> middle of about:home
> 
> Surprise! When the browser window gets focused, a focus event is triggered
> on the node that had focus before, and we reopen the suggestion panel :-(.
> 
> If you could find an easy way to extend your fix to also address that case,
> that would be awesome. If not, no worries, I think there's another bug on
> file for that case.

Looks like textbox uses a similar hack to solve that problem. I should be able to add that in easily enough.
(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > In what circumstance do we actually want the suggestions panel to pop up on
> > focus? I tend to just find that behaviour really annoying. Why can't we
> > remove it outright? :-)
> 
> This is a behavior Philipp explicitly requested, so I guess you want to ask
> him.
> 
> My guess is that the case where we really want the panel to open is when the
> user clicks a non-empty searchbox.

Yes, the panel opening more often is intentional.
The desired behavior (not fully implemented at the moment) actually even goes a few steps further. The panel should be visible whenever both of the following conditions are true:
- the search box is not empty
- the search box has the focus

Intentionally pressing the esc key would be the only way to have a focused search box without the panel.

The reason for this is that it is way to easy to lose the panel at the moment with no apparent way to bring it back. Since having the search field focused is a pretty clear indication that the user wants to do something search-related, it makes sense to show search options when that happens.
Flags: needinfo?(philipp)
I'm not a Firefox developer, but I'd like to give my 2¢:
If you want more usable software (e.g. Firefox), wouldn't it be good to use »de facto standard« UI behavior, so that people can generalize UI concepts for multiple programs, and not learn different behavior for Firefox and everything else? As a consequence wouldn't it be rather logical to introduce new behaviors to generic UI libraries like GTK/Gnome (first)?

And the obvious way to bring an auto complete dropdown back is to type something, just like with every other browser and every other software that has auto completion dropdowns. Users are not THAT dumb. And if you need to open an auto completion dropdown to change a setting, your're obviously doing something wrong with your UI design.
Blocks: 1102032
(In reply to Philipp Sackl [:phlsa] on PTO until Jan 7 from comment #17)
> Yes, the panel opening more often is intentional.
> The desired behavior (not fully implemented at the moment) actually even
> goes a few steps further. The panel should be visible whenever both of the
> following conditions are true:
> - the search box is not empty
> - the search box has the focus

Does this mean that the case described in comment 12 is not actually a bug? Following those steps we get back to the main window with the search box focused so your logic suggests that opening the suggestions is the right thing to do.
Flags: needinfo?(philipp)
Since Phil is not around...
Flags: needinfo?(shorlander)
Blocks: 1102050
(In reply to Dave Townsend [:mossop] from comment #19)
> (In reply to Philipp Sackl [:phlsa] on PTO until Jan 7 from comment #17)
> > Yes, the panel opening more often is intentional.
> > The desired behavior (not fully implemented at the moment) actually even
> > goes a few steps further. The panel should be visible whenever both of the
> > following conditions are true:
> > - the search box is not empty
> > - the search box has the focus
> 
> Does this mean that the case described in comment 12 is not actually a bug?
> Following those steps we get back to the main window with the search box
> focused so your logic suggests that opening the suggestions is the right
> thing to do.

No the case in comment 12 is still a bug. The cases Philipp outlined are cases where someone would directly target the Search Field (most likely on the currently focused window) indicating that they are about to do a search, or reuse a search.

The switching window case is different in that you probably don't remember where your focus was and you might be switching back to the window to do something completely non-search related and you end up with a search pop-up in your face.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
(In reply to Florian Quèze [:florian] [:flo] from comment #12)

> There's another case where we would like to be able to get to the original
> event that triggered the focus so that we can ignore it:
> 1. Focus a non-empty searchbar (you'll have the suggestion panel open)
> 2. Focus another window.
> 3. Focus the browser window again, eg. by clicking in the text field in the
> middle of about:home
> 
> Surprise! When the browser window gets focused, a focus event is triggered
> on the node that had focus before, and we reopen the suggestion panel :-(.

Bug 1102933 is a variation on this. Instead of focusing another window, the user just starts dragging the browser window.
Severity: blocker → normal
Attached patch patch (obsolete) — Splinter Review
This implements Philipp's vision of the suggestions box generally staying open until the user presses escape or focuses something else in the window. It also fixes the issues with stealing button presses, opening suggestions when context clicking and the panel not appearing when clicking.

A simple hack copied from textbox.xml lets us ignore focus events caused by switching back from another window.

Hopefully the comments in the code should be explanatory.

Linux is the exception. The behaviour is much better with this patch but there is some odd behaviour after focusing the search box with the keyboard. After that sometimes clicks in the search bar open and close the suggestions box. I plan to investigate that in a follow-up, as well as getting the tests working on linux and in e10s.

Whichever of you wants to review this first...
Attachment #8536891 - Attachment is obsolete: true
Attachment #8539558 - Flags: review?(florian)
Attachment #8539558 - Flags: review?(felipc)
(In reply to Dave Townsend [:mossop] from comment #26)

> Linux is the exception. The behaviour is much better with this patch but
> there is some odd behaviour after focusing the search box with the keyboard.
> After that sometimes clicks in the search bar open and close the suggestions
> box. I plan to investigate that in a follow-up

Sounds like bug 1107503.
(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> (In reply to Dave Townsend [:mossop] from comment #26)
> 
> > Linux is the exception. The behaviour is much better with this patch but
> > there is some odd behaviour after focusing the search box with the keyboard.
> > After that sometimes clicks in the search bar open and close the suggestions
> > box. I plan to investigate that in a follow-up
> 
> Sounds like bug 1107503.

No, in this case you click in the search box, the popup opens. If you click in the box again to move the caret the popup closes. But it sees to only get into this state randomly. I've also seen cases where opening the popup then mousing over it seems to lose focus from the rest of the window and anywhere you click does nothing except close the popup (bug 1102050), but again this is random, mostly it doesn't do that.
Comment on attachment 8539558 [details] [diff] [review]
patch

Review of attachment 8539558 [details] [diff] [review]:
-----------------------------------------------------------------

Great progress here, and thanks for writing all these tests! :-)

I see you've handled testing that the popup opens when tabbing from the url bar to the search bar. Would you mind adding a test for bug 1104142 while you are at it? (Focus in the urlbar, tab to the searchbox, panel opens, press shift tab, expect panel to be closed and focus to be on the urlbar).

::: browser/base/content/browser.xul
@@ +140,5 @@
>      <!-- for search and content formfill/pw manager -->
>      <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
>  
>      <!-- for search with one-off buttons -->
> +    <panel type="autocomplete" id="PopupSearchAutoComplete" noautohide="true" noautofocus="true" hidden="true"/>

Can you remind me which problem you are fixing with the noautohide here? I think you mentioned it on IRC but I don't remember the details.

My guess from my current testing of the patch is that this prevents the panel from closing when moving the caret in the text field using the mouse. Unfortunately, moving the caret with the left/right arrows still closes the panel.

Other issues here:
- if the search panel is open and I click on the Loop button or the hamburger button, these panels open behind the search panel which doesn't automatically close.
- if my selected tab contains the in-content preferences, clicking there in an empty area or on a label doesn't dismiss the search panel.

::: browser/components/search/test/browser.ini
@@ +36,5 @@
>  [browser_yahoo_behavior.js]
>  skip-if = e10s # Bug ?????? - some issue with progress listeners [JavaScript Error: "req.originalURI is null" {file: "chrome://mochitests/content/browser/browser/components/search/test/browser_bing_behavior.js" line: 127}]
>  [browser_abouthome_behavior.js]
>  skip-if = e10s || true # Bug ??????, Bug 1100301 - leaks windows until shutdown when --run-by-dir
> +[browser_openpopup.js]

Could you please change this name to something that makes it obvious it's searchbar related?

::: browser/components/search/test/browser_openpopup.js
@@ +26,5 @@
> +          },
> +          onError: function (errCode) {
> +            ok(false, "addEngine failed with error code " + errCode);
> +            reject();
> +          },

nit: trailing comma.

@@ +58,5 @@
> +// Simulates the full set of events for a context click
> +function context_click(target) {
> +  EventUtils.synthesizeMouseAtCenter(target, { type: "mousedown", button: 2 });
> +  EventUtils.synthesizeMouseAtCenter(target, { type: "contextmenu", button: 2 });
> +  EventUtils.synthesizeMouseAtCenter(target, { type: "mouseup", button: 2 });

Tempting to simplify with:
for (let event of ["mousedown", "contextmenu", "mouseup"])
  EventUtils.synthesizeMouseAtCenter(target, { type: event, button: 2 });

@@ +98,5 @@
> +
> +  EventUtils.synthesizeMouseAtCenter(textbox, {});
> +  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
> +  is(textbox.selectionStart, 0, "Should have selected all of the text");
> +  is(textbox.selectionEnd, 0, "Should have selected all of the text");

These last 2 lines don't seem to be testing much in this case, but they won't hurt I guess :).

@@ +101,5 @@
> +  is(textbox.selectionStart, 0, "Should have selected all of the text");
> +  is(textbox.selectionEnd, 0, "Should have selected all of the text");
> +});
> +
> +// Left clicking in the search box when unfocused should focus it and open the popup.

This comment should say it's a non-empty search box.

@@ +288,5 @@
> +  promise = promiseEvent(searchbar, "focus");
> +  newWin.close();
> +  yield promise;
> +
> +  // Wait a tick to allow any focus handlers to show the popup if they are going to.

Should this say "a few ticks"?

Do we need the same trick in add_no_popup_task?

::: toolkit/content/widgets/textbox.xml
@@ +246,5 @@
>          this._maybeSelectAll();
>          // see bug 576135 comment 4
>          let box = this.inputField.parentNode;
>          let menu = document.getAnonymousElementByAttribute(box, "anonid", "input-box-contextmenu");
> +        if (menu.state != "closed")

What is this test doing?
Attachment #8539558 - Flags: review?(florian)
Attachment #8539558 - Flags: review?(felipc)
Attachment #8539558 - Flags: review-
Given the issues with noautofocus here I'm probably going to remove that piece and the tests that verify that part so we can at least fix the context menu problems.

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> Comment on attachment 8539558 [details] [diff] [review]
> patch
> 
> Review of attachment 8539558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great progress here, and thanks for writing all these tests! :-)
> 
> I see you've handled testing that the popup opens when tabbing from the url
> bar to the search bar. Would you mind adding a test for bug 1104142 while
> you are at it? (Focus in the urlbar, tab to the searchbox, panel opens,
> press shift tab, expect panel to be closed and focus to be on the urlbar).
> 
> ::: browser/base/content/browser.xul
> @@ +140,5 @@
> >      <!-- for search and content formfill/pw manager -->
> >      <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
> >  
> >      <!-- for search with one-off buttons -->
> > +    <panel type="autocomplete" id="PopupSearchAutoComplete" noautohide="true" noautofocus="true" hidden="true"/>
> 
> Can you remind me which problem you are fixing with the noautohide here? I
> think you mentioned it on IRC but I don't remember the details.
> 
> My guess from my current testing of the patch is that this prevents the
> panel from closing when moving the caret in the text field using the mouse.
> Unfortunately, moving the caret with the left/right arrows still closes the
> panel.

Yes, that is right, not sure why text events are closing the suggestions panel but hopefully we could fix that. noautohide also fixes bug 1102050 for some reason, we aren't sure why at the moment since consumeoutsideclicks should be fixing that already.

> Other issues here:
> - if the search panel is open and I click on the Loop button or the
> hamburger button, these panels open behind the search panel which doesn't
> automatically close.
> - if my selected tab contains the in-content preferences, clicking there in
> an empty area or on a label doesn't dismiss the search panel.

Hmm, probably means the search box isn't losing focus in those cases. We probably need to rethink this piece.

> @@ +288,5 @@
> > +  promise = promiseEvent(searchbar, "focus");
> > +  newWin.close();
> > +  yield promise;
> > +
> > +  // Wait a tick to allow any focus handlers to show the popup if they are going to.
> 
> Should this say "a few ticks"?
> 
> Do we need the same trick in add_no_popup_task?

Apparently not, this seemed tied to switching back to a window.

> ::: toolkit/content/widgets/textbox.xml
> @@ +246,5 @@
> >          this._maybeSelectAll();
> >          // see bug 576135 comment 4
> >          let box = this.inputField.parentNode;
> >          let menu = document.getAnonymousElementByAttribute(box, "anonid", "input-box-contextmenu");
> > +        if (menu.state != "closed")
> 
> What is this test doing?

Forgot about this, when right clicking in a textbox this code attempts to update the commands in the context menu. This code was running when right clicking the magnifying glass and was logging an error to the console because the context menu wasn't opened (we open something else instead). The error seems harmless except the test harness was flagging it as a failure so this works around that case. I'll add comments to that effect.
No longer blocks: 1102050
No longer going to fix bug 1102050 here and I've filed bug 1114707 to track keeping the suggestions open as long as the user is interacting with the search box.
Attached patch patchSplinter Review
This removes the noautofix part so just fixes the context menu conflicts and popping up the suggestions when switching back to the window.

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> I see you've handled testing that the popup opens when tabbing from the url
> bar to the search bar. Would you mind adding a test for bug 1104142 while
> you are at it? (Focus in the urlbar, tab to the searchbox, panel opens,
> press shift tab, expect panel to be closed and focus to be on the urlbar).

I added this to focus_change_closes_popup.
Attachment #8539558 - Attachment is obsolete: true
Attachment #8540290 - Flags: review?(florian)
Attachment #8540290 - Flags: review?(felipc)
Comment on attachment 8540290 [details] [diff] [review]
patch

Review of attachment 8540290 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I didn't test locally this version of the patch.

::: browser/components/search/test/browser_searchbar_openpopup.js
@@ +158,5 @@
> +  is(textbox.selectionEnd, 3, "Should have selected all of the text");
> +
> +  promise = promiseEvent(searchPopup, "popuphidden");
> +  EventUtils.synthesizeKey("VK_TAB", { shiftKey: true });
> +  yield promise;

Would be nice to check either that the url bar is focused, or that the searchbox is no longer focused.
Attachment #8540290 - Flags: review?(florian) → review+
Comment on attachment 8540290 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1088660
[User impact if declined]: User can't use the context menu on the search box.
[Describe test coverage new/current, TBPL]: Added a large number of automated tests
[Risks and why]: The code is very low risk, it is well contained within the search bar and well tested by new automated tests. we're just adjusting which events specifically pop open the search suggestions. If there are bugs in the changes it could only affect whether or not the suggestions box pops open and there are automated tests for all the common cases I could think of.
[String/UUID change made/needed]: None
Attachment #8540290 - Flags: review?(felipc)
Attachment #8540290 - Flags: approval-mozilla-beta?
Attachment #8540290 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/475377e005de
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8540290 [details] [diff] [review]
patch

Approving uplift as these appear to have been merged to nightly in https://hg.mozilla.org/mozilla-central/rev/d5167dc0ded3
Attachment #8540290 - Flags: approval-mozilla-beta?
Attachment #8540290 - Flags: approval-mozilla-beta+
Attachment #8540290 - Flags: approval-mozilla-aurora?
Attachment #8540290 - Flags: approval-mozilla-aurora+
Verified fixed on Firefox 35.0b6 (20141222200458) using Ubuntu 12.04 LTS 32-bit, Windows 8.1 64-bit and Mac OS X 10.9.5.

There's an edge case here implying keyboards that have a button designated for the context menu [1]. If you type a search term in the search bar and then press the context menu keyboard button, the suggestions panel will not be dismissed and the context menu will overlap it. It happens *only* on Windows and Linux, here's a screenshot [2]. Dave, any thoughts on this?

[1] http://en.wikipedia.org/wiki/Menu_key#mediaviewer/File:Kontextmen%C3%BC.jpg 
[2] http://i.imgur.com/44WkTL2.png
Status: RESOLVED → VERIFIED
Flags: needinfo?(dtownsend)
(In reply to Andrei Vaida, QA [:avaida] from comment #39)

> There's an edge case here implying keyboards that have a button designated
> for the context menu [1]. If you type a search term in the search bar and
> then press the context menu keyboard button, the suggestions panel will not
> be dismissed and the context menu will overlap it. It happens *only* on
> Windows and Linux, here's a screenshot [2]. Dave, any thoughts on this?

Please file a bug blocking this one. Like you said, it's an edge case, so not something we should worry about for 35, but it would be good to fix it anyway :-).
Flags: needinfo?(dtownsend)
Depends on: 1115002
Blocks: 1103457
Blocks: 1102045
Verified as fixed using Developer Edition 36.0a2 and Nightly 37.0a1 2014-01-05 under Windows 7 64-bit and Mac OS X 10.9.5.
Depends on: 1118135
Blocks: 1119450
Depends on: 1126804
Depends on: 1129401
Sorry to bump, but could this please be re-inspected? Still appears bugged to me under FireFox 39.

Although right-clicking on the text area or icons within the unfocused search box does now function correctly, doing so above or below the area where the pointer switches to a text cursor still incorrectly reveals two menus.

Likewise if left clicking the text and then right-clicking in the brief moment before the "suggestions" menu appears.
Depends on: 1190311
(In reply to Jeffrey Alexander from comment #43)
> Sorry to bump, but could this please be re-inspected? Still appears bugged
> to me under FireFox 39.
> 
> Although right-clicking on the text area or icons within the unfocused
> search box does now function correctly, doing so above or below the area
> where the pointer switches to a text cursor still incorrectly reveals two
> menus.

This is an edge case of what we fixed here months ago; at this point it's better to figure this out in a separate bug; I filed bug 1190311 to track this issue.
You need to log in before you can comment on or make changes to this bug.