939705 - Defect - Arrow keys not working in context menu

Status: VERIFIED FIXED

(Tracking Graveyard :: Metro Operations, defect, P2)

Description

[Cornel Ionce [:noni]]

This defect has been logged as a follow up from bug 919232

STR:

1. Open Firefox in Metro mode.
2. Go to http://en.wikipedia.org/wiki/Main_Page (or any webpage containing text/hyperlinks).
3. Select text and right click / right click on a hyperlink.
4. Use the arrow keys to navigate through options.

Expected result:
The arrows highlight/change which option is highlighted.

Actual result:
The context menu will disappear if using arrows to navigate.

Comment 1

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: WIP

Fix is to handle keyboard events down in MenuPopup.

I can't figure out why the event handlers on listbox.xml[1] aren't being triggered. It works when I proxy them as in this patch. Other option is to handle keyboard events by hand, Rambo style, like I did in bug 919232.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml#547

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Rodrigo Silveira [:rsilveira] from comment #1)
> I can't figure out why the event handlers on listbox.xml[1] aren't being
> triggered.

I suspect it's either because the listbox doesn't have keyboard focus, and/or because MenuPopup.prototype.handleEvent is capturing the events and calling stopPropagation.

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> I suspect it's either because the listbox doesn't have keyboard focus,
> and/or because MenuPopup.prototype.handleEvent is capturing the events and
> calling stopPropagation.

I tried removing the keypress listener from MenuPopup and calling focus() on the listbox element but it didn't help. There might be something funny with focus on chrome elements.

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

Solution by proxying keypress events. I need a shower.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8342745 [details] [diff] [review]
Patch v1

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

I can live with this.  Some minor nits below:

::: browser/metro/base/content/helperui/MenuUI.js
@@ +516,5 @@
> +          null,              //  in nsIDOMAbstractView viewArg,  Specifies UIEvent.view. This value may be null.
> +          false,             //  in boolean ctrlKeyArg,
> +          false,             //  in boolean altKeyArg,
> +          false,             //  in boolean shiftKeyArg,
> +          false,             //  in boolean metaKeyArg,

Shouldn't we copy the modifier keys from the original event?

@@ +538,5 @@
> +          aEvent.stopPropagation();
> +          aEvent.preventDefault();
> +        }
> +
> +        if (!this._wantTypeBehind && !Util.isNavigationKey(aEvent)) {

Please change this line to "else if (!this._wantTypeBehind) {", for conciseness/clarity.

::: browser/metro/modules/ContentUtil.jsm
@@ +88,5 @@
>      // Return the modified object
>      return target;
> +  },
> +
> +  isNavigationKey: function (aEvent) {

Could you add a comment explaining that these are keys used for navigating lists?

Also, could you change this to take a keycode instead of an event for its argument, to make re-use and testing slightly easier?

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2

Patch with comments addressed.

There's a consistent failure in browser_selection_caretfocus.js test with this patch applied though.

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

Problem was that KeyboardEvent didn't work when calling utils from FormHelper because it's a frame script. Using Components.interfaces.nsIDOMKeyEvent fixed it and enabled me to move the method back into ContentUtil.jsm.

https://hg.mozilla.org/integration/fx-team/rev/3ef4df94f3dd

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ef4df94f3dd

Comment 9

[Cornel Ionce [:noni]]

Mozilla/5.0 (Windows NT 6.2; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

Confirming this is now working on latest Nightly (Build ID:20131209053402) using the steps from comment 0.
Marking as verified.