1115002 - The context menu overlaps the search panel if called via dedicated keyboard button

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on: Firefox 35.0b6 (20141222200458), Aurora 36.0a2 (2014-12-23), Nightly 37.0a1 (2014-12-23).

Affected platforms: Windows 8.1 64-bit and Ubuntu 12.04 LTS 32-bit.

Steps to reproduce:
0. Make sure you have a keyword that has a dedicated context menu button.
1. Launch Firefox with a clean profile.
2. Select the Search Bar from the toolbar and type in a search term - e.g. example.
3. Press the dedicated keyboard context menu button.

Expected result:
2. The search suggestions pane is displayed for that search term.
3. The search suggestions pane is dismissed and the context menu is displayed instead.

Actual result:
3. The search suggestions pane is *not* dismissed and as a result, the context menu overlaps it.

Screenshot: http://i.imgur.com/44WkTL2.png.

Notes:
- This issue does *not* affect Mac OS X.
- Example of a dedicated context menu keyboard button: http://en.wikipedia.org/wiki/Menu_key#mediaviewer/File:Kontextmen%C3%BC.jpg.

Comment 1

[Dave Townsend [:mossop]]

Probably the most straightforward way to fix this would be to add an onpopupshowing event handler to the context menu and in there make sure to close the suggestions box. I guess there is a question over whether the suggestions should open again after the context menu goes away.

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Hello,
I have edited search.xml (http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/components/search/content/search.xml#l598) and replaced this line
> <handler event="popupshowing" action="this.rebuildPopupDynamic();"/>
with
><handler event="popupshowing">
><![CDATA[
>  this.rebuildPopupDynamic();
>]]></handler>
but I have a problem to determine the method responsible for hiding the suggestionPanel or the xul element.

Comment 3

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed from comment #2)
> Hello,

Hi :-)

> I have edited search.xml
> (http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/components/
> search/content/search.xml#l598) and replaced this line
> > <handler event="popupshowing" action="this.rebuildPopupDynamic();"/>

Unless I read some of the code too quickly, it seems this popupshowing handler will be executed when the search suggestion panel appears, not when the context menu appears.

It seems there's already a popupshowing event listener for the context menu at http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/components/search/content/search.xml#l812 so you can probably add some code there.

> but I have a problem to determine the method responsible for hiding the
> suggestionPanel or the xul element.

From the searchbox, .popup gives you the suggestion panel, and the panel has a hidePopup method.

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - searchmenu

Thanks Florian.

(In reply to Dave Townsend [:mossop] from comment #1)
> I guess there is a question over whether the
> suggestions should open again after the context menu goes away.
This is done in this patch.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8551694 [details] [diff] [review]
rev 1 - searchmenu

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

(In reply to Abdelrhman Ahmed from comment #4)
> Created attachment 8551694 [details] [diff] [review]
> rev 1 - searchmenu
> 
> Thanks Florian.

Thanks for the updated patch.

Is there any chance you could update the browser/components/search/test/browser_searchbar_openpopup.js test to also test the case handled in this bug? The keyboard event that would need to be synthesized is VK_CONTEXT_MENU.

> (In reply to Dave Townsend [:mossop] from comment #1)
> > I guess there is a question over whether the
> > suggestions should open again after the context menu goes away.
> This is done in this patch.

After testing the patch with and without this part, I don't think we should do this. The reasons are:
- the Paste & Search context menu item starts a search, so the panel shouldn't reopen after that action.
- most context menu actions (copy, ...) will cause the search panel to reappear on its own, due to the events caused in the text field.
- most other ways to dismiss the context menu (clicking outside of it, escape key) would also dismiss the search panel, so reopening it doesn't seem helpful.

::: browser/components/search/content/search.xml
@@ +813,5 @@
> +          if (BrowserSearch.searchBar._textbox.popupOpen)
> +            this._openSuggestion = true;
> +          else
> +            this._openSuggestion = false;
> +          BrowserSearch.searchBar._textbox.closePopup();

Assuming we decided to go with this approach of reopening the panel after the context menu is closed, we could avoid the _openSuggestion field by doing something like this:

if (...popupOpen) {
  cxmenu.addEventListener("popuphiding", function cxmenu_hiding() {
    cxmenu.removeEventListener("popuphiding", cxmenu_hiding);
    openPanel();
  });
}

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - searchmenu

Sorry for being late, I needed to use windows build to check if test works fine or not
because this test doesn't work under linux(browser/components/search/test/browser.ini)
> [browser_searchbar_openpopup.js]
> skip-if = os == "linux" || e10s # Linux has different focus behaviours and e10s seems to have timing issues.

and also 
> EventUtils.synthesizeKey("VK_CONTEXT_MENU", {});
in this patch, VK_CONTEXT_MENU didn't work(open context menu) when I ran test on windows.

Comment 7

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed from comment #6)

Thanks for the updated patch! :-)

> > EventUtils.synthesizeKey("VK_CONTEXT_MENU", {});
> in this patch, VK_CONTEXT_MENU didn't work(open context menu) when I ran
> test on windows.

I'm a little confused by this sentence. Is this new patch ready for review (from a quick look it seemed good) or does the test need debugging?

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> (In reply to Abdelrhman Ahmed from comment #6)
> I'm a little confused by this sentence. Is this new patch ready for review
> (from a quick look it seemed good) or does the test need debugging?

If you are sure that EventUtils.synthesizeKey("VK_CONTEXT_MENU", {}); will press on menu button, this patch is ready for review.
I'm saying that because I have ran automated test(mochitest) for this file on windows to check the test case added, It seems VK_CONTEXT_MENU doesn't behave correctly.

Comment 9

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed from comment #8)
> (In reply to Florian Quèze [:florian] [:flo] from comment #7)
> > (In reply to Abdelrhman Ahmed from comment #6)
> > I'm a little confused by this sentence. Is this new patch ready for review
> > (from a quick look it seemed good) or does the test need debugging?
> 
> If you are sure that EventUtils.synthesizeKey("VK_CONTEXT_MENU", {}); will
> press on menu button, this patch is ready for review.
> I'm saying that because I have ran automated test(mochitest) for this file
> on windows to check the test case added, It seems VK_CONTEXT_MENU doesn't
> behave correctly.

I haven't tested EventUtils.synthesizeKey("VK_CONTEXT_MENU", {}) specifically; what I did before writing comment 5 is I dumped the keyCode in a keypress handler, and saw the code when pressing the menu button on my Windows machine was 93, which according to https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Constants_for_keyCode_value is DOM_VK_CONTEXT_MENU.

Maybe we should just push the patch to the try server to check how the test behaves there? Do you have try server access?

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Maybe we should just push the patch to the try server to check how the test
> behaves there? Do you have try server access?

I haven't used it, How can I check my permissions or push something for test on it?

Comment 11

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed from comment #10)
> (In reply to Florian Quèze [:florian] [:flo] from comment #9)
> > Maybe we should just push the patch to the try server to check how the test
> > behaves there? Do you have try server access?
> 
> I haven't used it, How can I check my permissions or push something for test
> on it?

If you've never requested it, you don't have a mercurial account to access the try server (the process is explained at https://www.mozilla.org/en-US/about/governance/policies/commit/).

I just pushed it to try for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b35a9de5be

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Thanks Florian. I have requested to get account but I need to be vouched (https://bugzilla.mozilla.org/show_bug.cgi?id=1126305)

> I just pushed it to try for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b35a9de5be
In the test results, It seems VK_CONTEXT_MENU doesn't behave correctly so it causes Test timed out and that affects next test, Is there any alternatives?

Comment 13

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed from comment #12)

> In the test results, It seems VK_CONTEXT_MENU doesn't behave correctly so it
> causes Test timed out and that affects next test, Is there any alternatives?

This is an acceptable alternative:
    EventUtils.synthesizeMouseAtCenter(textbox, { type: "contextmenu", button: null });

It would be nice to include above it a comment saying that synthesizeKey doesn't work with VK_CONTEXT_MENU, and to include a bug number.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - searchmenu

> This is an acceptable alternative:
>     EventUtils.synthesizeMouseAtCenter(textbox, { type: "contextmenu",
> button: null });

I have tested it locally and it worked fine so I have pushed this patch 
to try to check all cases work fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13d8d862828

Comment 15

[Abdelrahman Ahmed [:abdelrahman]]

I canceled last try because of unittests are not specified correctly
and this the new try with correct unittest.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ef740ffa675

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8556323 [details] [diff] [review]
rev 3 - searchmenu

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

2 details in the test (and I'm still waiting for the Mac results from your last try server push). Looks great otherwise :).

::: browser/components/search/test/browser_searchbar_openpopup.js
@@ +237,5 @@
> +  is(textbox.selectionEnd, 3, "Should have selected all of the text");
> +
> +  promise = promiseEvent(searchPopup, "popuphidden");
> +
> +  //synthesizeKey does not work with VK_CONTEXT_MENU (bug 1115002)

When I said it would be nice to include a bug number, I meant it would be useful to file a bug in https://bugzilla.mozilla.org/enter_bug.cgi?product=Testing&component=Mochitest to get the problem fixed. And mentioning the bug number here is so that it's easy to find and remove this workaround once the other bug is fixed.

@@ +242,5 @@
> +  EventUtils.synthesizeMouseAtCenter(textbox, { type: "contextmenu", button: null });
> +
> +  yield promise;
> +
> +  let contextPopup = document.getAnonymousElementByAttribute(textbox.inputField.parentNode, "anonid", "input-box-contextmenu");

nit: this long line would look better with a line break:
  let contextPopup = document.getAnonymousElementByAttribute(textbox.inputField.parentNode,
                                                             "anonid", "input-box-contextmenu");

or if you want to stay under 80 characters per line, you can even do:
  let contextPopup =
    document.getAnonymousElementByAttribute(textbox.inputField.parentNode,
                                            "anonid", "input-box-contextmenu");

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - searchmenu

The test failed on OS X 10.6 opt but succeed on OS X 10.6 debug!

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8557404 [details] [diff] [review]
rev 4 - searchmenu

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

Looks ready for check-in now, thanks!

(In reply to Abdelrhman Ahmed from comment #17)

> The test failed on OS X 10.6 opt but succeed on OS X 10.6 debug!

It's the browser/components/sessionstore/test/browser_formdata.js test that failed on OS X 10.6 opt, not yours so I don't think it's related. I retriggered the bc1 run there just to be sure, and the second run was green :-).

Comment 19

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/8542e80cd8cd

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8542e80cd8cd

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8557404 [details] [diff] [review]
rev 4 - searchmenu

Approval Request Comment
[Feature/regressing bug #]: this fixes an edge case of bug 1103455 (which was uplifted all the way to 35), which was fixing something from bug 1088660 (new searchbar UI).
[User impact if declined]: pressing the 'context' key on the keyboard results in the ugly display in http://i.imgur.com/44WkTL2.png
[Describe test coverage new/current, TreeHerder]: the patch contains a test.
[Risks and why]: low risk fix, but also not a very visible bug, so I don't think we we should uplift to beta this late in the cycle; waiting until 37 seems OK.
[String/UUID change made/needed]: none.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557404 [details] [diff] [review]
rev 4 - searchmenu

Simple fix for a UI polish issue. Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b85afc7a726a

Comment 24

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 37.0a2 (2015-02-12) and Nightly 38.0a1 (2015-02-12), using Ubuntu 12.04 (32 bit) and Windows 8.1 (64 bit).