597981 - When Google VKB is opened, Key-Repeat does not work! (On latest gtk2 platforms, keydown events are not dispatched by auto repeat)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jeremias Bosch (:jbos)]

STEPS LEADING TO PROBLEM: 

1. Open google.com page, HWK is active.
2. Open Google VKB 
3. Press and keep any normal button on HKW
4. Look at the google input filed

EXPECTED OUTCOME:
When button is pressed and kept character should be repeated as long as is
pressed.

ACTUAL OUTCOME:
It requires pressing and releasing every time new character should be entered,
so automatic repetition when key is pressed for longer time doesn't work in
Browser when google VKB is opened. 

FREQUENCY OF OCCURRENCE: 
always

Comment 1

[Matt Brubeck (:mbrubeck)]

This might not be a Fennec bug; I cannot get key repeat to work while Google Virtual Keyboard is displayed in Minefield either:

Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100919 Firefox/4.0b7pre

Comment 2

[Matt Brubeck (:mbrubeck)]

Key repeat using the hardware keyboard on the Google Virtual Keyboard demo works in both Chromium and Opera, but not Firefox 3.6 or Firefox 4 nightlies:
http://www.google.com/webelements/virtualkeyboard/

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

pretty sure this is a platform bug

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't understand this report, do you think that this bug is caused by bug 340149?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like there is no problem on Linux (Ubuntu 9.10) build too. What's this about?

Comment 6

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

(In reply to comment #2)
> Key repeat using the hardware keyboard on the Google Virtual Keyboard demo
> works in both Chromium and Opera, but not Firefox 3.6 or Firefox 4 nightlies:
> http://www.google.com/webelements/virtualkeyboard/

Sounds like a bug in the web app.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I can reproduce this bug on Ubuntu 10.4.

Comment 8

[Juha Seppänen]

WONTFIX?

Comment 9

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

well, someone should look at what the webapp is doing and then
we need to figure out if the bug is in gecko or in the web app.

Comment 10

[Jeremias Bosch (:jbos)]

I have the impression that its releated to this bug 577630. So maybe we need to figure out whats going on with this and see if it fixes this issue. For my opinion - with that bug we just see a inconsistency in handling the keyevents in the system.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, probably, this is a behavior change of GTK2 or IM. I'll post a patch.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

The cause of this bug is, Ubuntu 10.4 doesn't send native keyrelease event during pressing a key but Ubuntu 9.10 sent it. Therefore, the dispatched DOM key events are changed at auto repeat.
9.10:  KEYDOWN -> KEYPRESS -> KEYUP -> KEYDOWN...
10.4:  KEYDOWN -> KEYPRESS -> KEYPRESS -> KEYPRESS...

However, our KeyDown event must be dispatched when native keypress event is received. (DOM3 event spec said so, and our editor listens keydown event)

Note that the keydown flags can be removed on current build, but I think that we shouldn't do it because DOM3 key events have repeat property. When we'll implement it, the flags is needed.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, but I'm not sure why the software keyboard kills the auto repeat.

And following test case doesn't work fine on current trunk build on Ubuntu10.4.
> data:text/html,<script type="text/javascript">function putlog(event) { var log = document.getElementById("log"); log.innerHTML = event.type + "\n" + log.innerHTML; }</script><textarea onkeydown="putlog(event); event.preventDefault();" onkeypress="putlog(event);"></textarea><pre id="log"></pre>

At keydown event, this calls preventDefault(), then, the next keypress event doesn't cause text input. The patch also fixes this bug.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0.1

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 477159 [details] [diff] [review]
Patch v1.0.1

fix related comment.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 477159 [details] [diff] [review]
Patch v1.0.1

The new behavior should be reviewed by Smaug too.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 477159 [details] [diff] [review]
Patch v1.0.1

Someone went to some trouble to preserve the single keydown behavior, but
http://www.w3.org/TR/DOM-Level-3-Events/#events-keyboard-event-order is clear that it wants multiple keydowns and it sounds like this is what's provided for win32, so this looks fine to me.

Can you remove the now unused mKeyDownFlags and related code, please?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> Can you remove the now unused mKeyDownFlags and related code, please?

Okay.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1 for 1.9.2 branch

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1 for 1.9.1branch

Comment 22

[Jeremias Bosch (:jbos)]

can you take a look on qt port too? or should someone else make the patch for qt

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #22)
> can you take a look on qt port too? or should someone else make the patch for
> qt

Can I build firefox (not fennec) with qt? If so, I'll look the qt code. Otherwise, I cannot do it.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Smaug, would you review the patch?

Comment 25

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

IMO, it would be very scary to take this kind of change to branches.

Comment 26

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

...but reviewing...

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #25)
> IMO, it would be very scary to take this kind of change to branches.

But the branch's behavior has been changed by the platform's behavior change...

Comment 28

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

The patch will change our gtk widget code to dispatch keydown for the repeated
key events. That is rather major change. The repeated key event handling has been
there since 2001.
The change in behavior is good (and it makes gtk to work the same way as
windows and OSX, right? Please test.).

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 477397 [details] [diff] [review]
Patch v1.1

GTK2 has changed its key event spec. The old GTK2 sent a set of keypress event -> keyrelease event for every repeated key input. Therefore, we dispatched a set of DOMKeydown -> DOMKeypress -> DOMKeyup for every input.

However, current GTK2 platform sends only keypress event for every key input. Therefore, we dispatched only DOMKeypress event for input. Therefore, web apps cannot cancel the input by DOMKeydown.preventDefault() and some web application may be broken by this behavior change.

This patch dispatches DOMKeydown event before DOMKeypress event. Therefore, builds with this patch dispatch a set of DOMKeydown -> DOMKeypress for every input. This behavior conforms with DOM3 key event spec.

Comment 30

[Daniel Veditz [:dveditz]]

Scary change to compensate for a regression that's not our fault -- definitely not blocking branch releases. Once this has landed on trunk and gotten reasonable exposure and testing (how many gtk2 beta users do we have?) we might consider this for the older branches. Meanwhile Ubuntu users can stick with 9.10 or upgrade to Firefox 4 betas (after the fix lands) if the behavior really bothers them.

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to comment #29)
> GTK2 has changed its key event spec. The old GTK2 sent a set of keypress event
> -> keyrelease event for every repeated key input. Therefore, we dispatched a
> set of DOMKeydown -> DOMKeypress -> DOMKeyup for every input.

I'm surprised to hear that was happening on Ubuntu 9.10.
GTK has been suppressing KeyRelease events during autorepeat since at least 2.12 (2007), and the code seems to have been written expecting that.
I don't know why the Ubuntu behavior is different.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that bug 599887 says our behavior isn't consistent between platforms, I don't think this should block branches, or even go on them.

But now that DOM 3 events has specified this (although I don't like what it has specified), we should support it.

We should really support the .repeat attribute on key events, though...

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also see bug 517495, which reports a different behavior.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 477397 [details] [diff] [review]
Patch v1.1

canceling a2.0? due to beta8+.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #32)
> We should really support the .repeat attribute on key events, though...

I filed bug 600117.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, sorry for resetting the flags unexpectedly.

Would -'ing the blocking 1.9.2 and the blocking 1.9.1?

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/6cb6bb98e269

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://developer.mozilla.org/en/DOM/Event/UIEvent/KeyEvent#Order_of_key_events