583783 - Keyboard shortcuts do not work in remote tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Enable pref browser.tabs.remote. (It is enabled by default on trunk.)
2. Open a new tab.
3. Close the awesomescreen and click in the content area.
4. Type Control+L.

Expected results: urlbar is focused.
Actual results: nothing happens.

It looks like we rely on key events bubbling from content to chrome, which doesn't work in e10s remote browsers.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

related to the fix that went in for bug 582560. It would be nice if we could test for a shortcut _before_ sending the keys to the content

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to comment #1)
> related to the fix that went in for bug 582560. It would be nice if we could
> test for a shortcut _before_ sending the keys to the content

That might work for control+key shortcuts, but not for backspace.  We need to give content a chance to handle backspace first.

(Note: The escape key still works, because we handle it in the capturing phase.)

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This is one approach I tried.  The messages are received, but the fake key events do not actually trigger any <key> commands; not sure why.  (I tried both windowUtils.sendKeyEvent and window.dispatchEvent.)

Comment 4

[[:fabrice] Fabrice Desré]

It looks like the key event in the process content does not always have the ctrlKey field correctly set. At least that's what I'm seing there, with a  very inconsistent behavior between runs.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

This patch fixes the modifier problem (comment 4).  It still does not work for unmodified keys (like backspace).  And we might want to do as Mark suggests and avoid sending the cross-process key event to begin with, when the accelerator key is held down.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: part 1

Cleaned up for checkin.  This still doesn't solve the problem of shortcuts with no modifiers (like backspace), but it does work for control-key shortcuts and fixes some related bugs too.  (For example, editing shortcuts like control+a were broken in web content form fields.)

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (WIP)

To handle backspace correctly, I think we need to replace sendCrossProcessKeyEvent with something like this which checks the return value from nsIDOMWindowUtils.SendKeyEvent on the content side.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch is simpler and works with both control+key and single key shortcuts.  It replaces sendCrossProcessKeyEvent with an equivalent message whose handler checks whether the event was consumed in the content process; if it was not consumed then it sends it back to the parent process for handling.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 464078 [details] [diff] [review]
patch


>+      case "Browser:KeyEvent":
>+        let defaultAction = Util.getWindowUtils(content).sendKeyEvent(
>+          json.type, json.keyCode, json.charCode, modifiers);

I'm fine with keeping this on one line. Better readability. Or make a local for the windowUtil

>+          sendAsyncMessage("Browser:KeyPress", {
>+            ctrlKey:  json.modifiers & masks.CONTROL_MASK,
>+            shiftKey: json.modifiers & masks.SHIFT_MASK,
>+            metaKey:  json.modifiers & masks.META_MASK,
>+            keyCode:  json.keyCode,
>+            charCode: json.charCode

Don't feel the need to align "json" :)

We should be able to remove this code when we kill tiles.

Comment 10

[Matt Brubeck (:mbrubeck)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/663a2f6aa05a

Bug 585759 filed for moving this into platform code.

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.0b4pre Fennec/2.0a1pre