Keyboard shortcuts do not work in remote tabs

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
--
major
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
tracking-fennec: --- → ?
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
(Assignee)

Comment 2

8 years ago
(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.)

Updated

8 years ago
tracking-fennec: ? → 2.0b1+
(Assignee)

Comment 3

8 years ago
Created attachment 462599 [details] [diff] [review]
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.)
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.
(Assignee)

Comment 5

8 years ago
Created attachment 463930 [details] [diff] [review]
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.
Attachment #462599 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
Created attachment 463932 [details] [diff] [review]
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.)
Attachment #463930 - Attachment is obsolete: true
Attachment #463932 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

8 years ago
Created attachment 463974 [details] [diff] [review]
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.
(Assignee)

Comment 8

8 years ago
Created attachment 464078 [details] [diff] [review]
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.
Attachment #463932 - Attachment is obsolete: true
Attachment #463974 - Attachment is obsolete: true
Attachment #464078 - Flags: review?(mark.finkle)
Attachment #463932 - Flags: review?(mark.finkle)
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.
Attachment #464078 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 10

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

Bug 585759 filed for moving this into platform code.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.