Closed Bug 1115002 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Search, defect)

35 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- verified
firefox38 --- verified

People

(Reporter: avaida, Assigned: abdelrahman, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Flags: qe-verify+
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.
Flags: firefox-backlog+
Mentor: florian
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.
(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.
Attached patch rev 1 - searchmenu (obsolete) — Splinter Review
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.
Attachment #8551694 - Flags: review?(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();
  });
}
Attachment #8551694 - Flags: review?(florian) → feedback+
Attached patch rev 2 - searchmenu (obsolete) — Splinter Review
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.
Attachment #8551694 - Attachment is obsolete: true
(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?
(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.
(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?
(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?
(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
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?
(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.
Attached patch rev 3 - searchmenu (obsolete) — Splinter Review
> 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
Attachment #8552928 - Attachment is obsolete: true
Attachment #8556323 - Flags: review?(florian)
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 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");
Attachment #8556323 - Flags: review?(florian) → feedback+
Assignee: nobody → a.ahmed1026
The test failed on OS X 10.6 opt but succeed on OS X 10.6 debug!
Attachment #8556323 - Attachment is obsolete: true
Attachment #8557404 - Flags: review?(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 :-).
Attachment #8557404 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/8542e80cd8cd
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8542e80cd8cd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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.
Attachment #8557404 - Flags: approval-mozilla-aurora?
Iteration: --- → 38.2 - 9 Feb
Comment on attachment 8557404 [details] [diff] [review]
rev 4 - searchmenu

Simple fix for a UI polish issue. Aurora+
Attachment #8557404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.