Closed Bug 939705 Opened 6 years ago Closed 6 years ago

Defect - Arrow keys not working in context menu

Categories

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

All
Windows 8
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: cornel_ionce, Assigned: rsilveira)

References

Details

(Whiteboard: [block28] feature=defect c=tbd u=tbd p=2)

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
Summary: Defect - Arrow keys not working in selected text/hyperlink → Defect - Arrow keys not working in context menu
Assignee: nobody → rsilveira
Blocks: metrov1it20
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=2
Attached patch WIP (obsolete) — Splinter Review
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
(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.
(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.
Attached patch Patch v1 (obsolete) — Splinter Review
Solution by proxying keypress events. I need a shower.
Attachment #8338955 - Attachment is obsolete: true
Attachment #8342745 - Flags: review?(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?
Attachment #8342745 - Flags: review?(mbrubeck) → review+
Attached patch Patch v2Splinter Review
Patch with comments addressed.

There's a consistent failure in browser_selection_caretfocus.js test with this patch applied though.
Attachment #8342745 - Attachment is obsolete: true
Attachment #8343328 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/3ef4df94f3dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Depends on: 950075
Product: Tracking → Tracking Graveyard
You need to log in before you can comment on or make changes to this bug.