555074 - Key events in the URL bar are forwarded from the chrome process to the content process

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

To see this, run fennectrolysis with MOZ_IPC_MESSAGE_LOG=1 and type in the URL bar.

stechz tells me that this is not intended.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I don't have a E10s-Fennec build so can't test.

Sounds like a bug in Fennec code.

Btw, is this a Qt or Gtk build? They have different focus handling, 
and I have no idea how it works in Qt.
(key events go in general to the focused element.)

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, missed the question: I saw this in a Gtk build, haven't tested Qt.

Comment 3

[Frederic Plourde (:fred23)]

Attachment: patch v.1

I investigated this. The problem clearly lives in Fennec InputHandler.
No matter where the key event came from, the keyModule would dispatch to our custom Key Handler (crossProcess key send...).

This quick fix seems to solve the problem.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 439325 [details] [diff] [review]
patch v.1

Sorry, I know next to nothing about Fennec's code.

Comment 5

[Frederic Plourde (:fred23)]

sorry, my bad ;-)

Comment 6

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

Comment on attachment 439325 [details] [diff] [review]
patch v.1

>diff -r b798302abc67 chrome/content/InputHandler.js

>   handleEvent: function handleEvent(evInfo) {
>     if (evInfo.event.type == "keydown" || evInfo.event.type == "keyup" || evInfo.event.type == "keypress") {
>-      this._keyer = this._browserViewContainer.customKeySender;
>-      if (this._keyer) {
>-        this._keyer._dispatchKeyEvent(evInfo.event);
>+      if (evInfo.event.target == this._browserViewContainer) {
>+        this._keyer = this._browserViewContainer.customKeySender;
>+        if (this._keyer) {
>+          this._keyer._dispatchKeyEvent(evInfo.event);
>+        }

instead of:

if (evInfo.event.target == this._browserViewContainer)

what about just moving the keypress, keydown, and keyup events back under browserViewContainer

this.listenFor(browserViewContainer, "keyup");
this.listenFor(browserViewContainer, "keydown");
this.listenFor(browserViewContainer, "keypress");

Here http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/file/aa1050b285bc/chrome/content/InputHandler.js#l142

Comment 7

[Frederic Plourde (:fred23)]

Attachment: patch v.2

much simpler like that, indeed.

Comment 8

[Frederic Plourde (:fred23)]

Attachment: patch v.3

Fixed bitrot

Comment 9

[Frederic Plourde (:fred23)]

Seems I had commiter's right to http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s after all.

Pushed http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/rev/e1d062de26a7

Comment 10

[Tony Chung [:tchung]]

martijn can you verify this when you test fennectrolysis?

Comment 11

[Doug Turner (:dougt)]

http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/rev/9040a85af814

this regressed fennec -- i could not get any key events to content.  reopening.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Add tabindex=-1

The last patch had the right idea I think: use the event model to handle who gets what key events instead of doing it ourselves.  The only problem was that tile-container can't receive focus!

tabindex="-1" fixes this problem.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 441182 [details] [diff] [review]
Add tabindex=-1

wfm

Comment 14

[Frederic Plourde (:fred23)]

Pushed:
http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/rev/7fbce1943ff8

Comment 15

[Doug Turner (:dougt)]

so... not completely wfm....

i am noticing now that when i press the delete key, we move back in session history.

Comment 16

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

(In reply to comment #15)
> so... not completely wfm....
> 
> i am noticing now that when i press the delete key, we move back in session
> history.

What element has focus? The URLBar? or content?

Comment 17

[Benjamin Stover (:stechz)]

Wasn't stopping prop and preventing default.  Pushed fix.