Last Comment Bug 600117 - Implement DOM3 KeyboardEvent.repeat
: Implement DOM3 KeyboardEvent.repeat
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla28
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
:
Mentors:
https://bug622245.bugzilla.mozilla.or...
Depends on: 768287
Blocks: 680829
  Show dependency treegraph
 
Reported: 2010-09-27 20:15 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2015-09-30 01:53 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (work in progress) (8.36 KB, patch)
2010-09-28 20:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v0.1.1 (work in progress) (9.62 KB, patch)
2010-09-28 21:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (13.49 KB, patch)
2013-06-05 08:28 PDT, Xidorn Quan [:xidorn] (UTC+10)
masayuki: feedback-
Details | Diff | Splinter Review
part.1 Implement KeyboardEvent.repeat (7.68 KB, patch)
2013-10-23 12:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
jst: superreview+
Details | Diff | Splinter Review
part.2 Implement KeyboardEvent.repeat on Windows (1.85 KB, patch)
2013-10-23 12:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.3 Implement KeyboardEvent.repeat on Mac (1.04 KB, patch)
2013-10-23 12:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
part.4 Implement KeyboardEvent.repeat on Android (1.12 KB, patch)
2013-10-23 12:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.5 Implement KeyboardEvent.repeat on Gonk (3.34 KB, patch)
2013-10-23 12:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.6 Implement KeyboardEvent.repeat on OS/2 (1.26 KB, patch)
2013-10-23 12:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
daveryeo: review+
Details | Diff | Splinter Review
part.7 Implement KeyboardEvent.repeat on Qt (1.12 KB, patch)
2013-10-23 12:54 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
romaxa: review+
Details | Diff | Splinter Review
part.8 Implement KeyboardEvent.repeat on GTK (7.97 KB, patch)
2013-10-23 12:57 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
karlt: review-
Details | Diff | Splinter Review
part.5 Implement KeyboardEvent.repeat on Gonk (3.34 KB, patch)
2013-10-23 13:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
mwu.code: review+
Details | Diff | Splinter Review
part.4 Implement KeyboardEvent.repeat on Android (1.14 KB, patch)
2013-10-23 14:49 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
nchen: review+
Details | Diff | Splinter Review
part.8 Implement KeyboardEvent.repeat on GTK (12.23 KB, patch)
2013-11-06 18:07 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
karlt: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-09-27 20:15:27 PDT
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-repeat

GTK2 doesn't have repeat state in GdkEventKey. So, we need some hack for GTK2.

I'll post patches after bug 597981 and bug 599887.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-09-27 20:29:09 PDT
Smaug:

Should I create a new interface like nsIDOM3KeyEvent? Or change nsIDOMKeyEvent?
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-09-28 20:59:34 PDT
Created attachment 479287 [details] [diff] [review]
Patch v0.1 (work in progress)
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-09-28 21:31:30 PDT
Created attachment 479297 [details] [diff] [review]
Patch v0.1.1 (work in progress)
Comment 4 Xidorn Quan [:xidorn] (UTC+10) 2013-06-05 08:28:39 PDT
Created attachment 758604 [details] [diff] [review]
Patch

This patch is based on Masayuki Nakano's work. It is modified for the current codebase, and implements KeyboardEvent.repeat for one additional framework - cocoa, which is the only one I actually tested. I also modified the implementation for windows according to its document on MSDN: http://msdn.microsoft.com/en-us/library/windows/desktop/ms646280%28v=vs.85%29.aspx

Since GTK2 does not provide such a similar interface like other frameworks, I suggest we create a new bug for it individually.

Feedback is welcome.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-05 18:48:15 PDT
Comment on attachment 758604 [details] [diff] [review]
Patch

GTK is tier-1 platform of Firefox. So, it's important to implement new feature on GTK at first time. So, you must implement the mechanism for GTK first. It's what the blocker of this bug. See bug 768287 comment 7, perhaps, we may be able to physical key state with each keypress/keyrelease events with X's API. I believe that we should make such framework before this bug.
Comment 6 Karl Tomlinson (:karlt) 2013-06-06 18:41:54 PDT
With GDK, autorepeat keypresses are usually detectable because they follow a previous GDK_KEY_PRESS event with the same hardware_keycode.

i.e.

On key press when key event is not for a modifier:
   1. if hardware_keycode matches saved previous keycode,
      then set repeat on the event.
   2. save hardware_keycode as previous

On key release when key event is not for a modifier:
   1. if hardware_keycode matches saved previous keycode,
      then reset saved keycode to 0.

      (Release of a different key does not cancel key repeat,
       but may change the keysym/keyval)

A limitation with this approach is that we don't get key events when we don't have focus, so the saved previous keycode needs to be reset on focus out.
That means we won't be able to tell whether the first key event after focus in is a repeat or not.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-08-28 00:15:07 PDT
Depends on the patch for bug 768287 on GTK.

I restart to work on this.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:44:53 PDT
Created attachment 821218 [details] [diff] [review]
part.1 Implement KeyboardEvent.repeat
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:47:08 PDT
Created attachment 821221 [details] [diff] [review]
part.2 Implement KeyboardEvent.repeat on Windows
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:47:40 PDT
Created attachment 821224 [details] [diff] [review]
part.3 Implement KeyboardEvent.repeat on Mac
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:48:09 PDT
Created attachment 821225 [details] [diff] [review]
part.4 Implement KeyboardEvent.repeat on Android
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:48:35 PDT
Created attachment 821228 [details] [diff] [review]
part.5 Implement KeyboardEvent.repeat on Gonk
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:52:36 PDT
Created attachment 821234 [details] [diff] [review]
part.6 Implement KeyboardEvent.repeat on OS/2

I don't test this patch, actually, though.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:54:09 PDT
Created attachment 821236 [details] [diff] [review]
part.7 Implement KeyboardEvent.repeat on Qt

Since I cannot build it on my environment, I don't test this patch actually.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:57:36 PDT
Created attachment 821240 [details] [diff] [review]
part.8 Implement KeyboardEvent.repeat on GTK

This is using your approach.

Storing hardware keycode if the pressed key is auto repeatable key. Otherwise, not stores it. Additionally, at losing focus, resetting the repeat flag. This patch avoids unnecessary case of the focus change event as far as possible.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 12:58:39 PDT
FYI: test builds will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4f9a0ffa2621
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 13:43:36 PDT
Created attachment 821264 [details] [diff] [review]
part.5 Implement KeyboardEvent.repeat on Gonk

I'm not sure if this patch actually works. However, as far as I've checked, there is no any way to check if the key event is caused by long press or auto repeat.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 13:45:18 PDT
Comment on attachment 821224 [details] [diff] [review]
part.3 Implement KeyboardEvent.repeat on Mac

Oops, sorry for the spam. I requested the review to wrong person (smaug).
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-10-23 14:49:32 PDT
Created attachment 821303 [details] [diff] [review]
part.4 Implement KeyboardEvent.repeat on Android

Test build is here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c6a415ba6ed9

With the RepeatCount(), it works with hardware keyboard. But it doesn't work with iWnn and Google Japanese input if I use software keyboard, though.
Comment 20 Michael Wu [:mwu] 2013-11-04 09:18:12 PST
Comment on attachment 821264 [details] [diff] [review]
part.5 Implement KeyboardEvent.repeat on Gonk

Review of attachment 821264 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay reviewing. Looks fine to me.

::: widget/gonk/nsAppShell.cpp
@@ +555,5 @@
>      }
> +    case UserInputData::KEY_DATA: {
> +        bool isPress = data.action == AKEY_EVENT_ACTION_DOWN;
> +        bool isRepeat =
> +            isPress && ((data.flags & AKEY_EVENT_FLAG_LONG_PRESS) != 0);

nit: Don't compare against 0. s/ != 0//
Comment 21 Karl Tomlinson (:karlt) 2013-11-04 14:09:53 PST
Comment on attachment 821240 [details] [diff] [review]
part.8 Implement KeyboardEvent.repeat on GTK

>+    gdk_window_remove_filter(NULL, (GdkFilterFunc)FilterEvents, this);

nullptr

>+            XKeyboardState keyboardState;
>+            if (!XGetKeyboardControl(xEvent->xkey.display, &keyboardState)) {
>+                break;
>+            }

Here you want this information to know whether this key will affect the
currently repeating key.
However, XGetKeyboardControl() requires a round trip to the X server, and I
don't want to do that for every key press.

It is possible to listen for XkbControlsNotifyEvents to detect when the
per_key_repeat information changes.  Use XkbSelectEventDetails with
XkbControlsNotify and XkbPerKeyRepeatMask to request these events [1].  It is
not actually necessary to consider global_auto_repeat because press events
will always have release events and the logic below won't set the state to
REPEATING if global_auto_repeat is false.

>+            if (sLastRepeatableHardwareKeyCode != xEvent->xkey.keycode) {
>+                // This case means the key release event is caused by
>+                // a non-repeatable key such as Shift.

Or by a repeatable key that was pressed before sLastRepeatableHardwareKeyCode
was pressed.

>+            if (xEvent->xfocus.mode != NotifyNormal) {

Other focus modes can also shift focus to another application, during which
the key state might change without any notification.

Was there a reason for selecting only NotifyNormal?

>+            if (XEventsQueued(xEvent->xfocus.display, QueuedAfterReading)) {
>+                XEvent nextEvent;
>+                XPeekEvent(xEvent->xfocus.display, &nextEvent);
>+                // We won't be deactivated if the next event is focus event.
>+                if (nextEvent.type == FocusIn &&
>+                    nextEvent.xfocus.mode == NotifyNormal) {
>+                    break;
>+                }

There won't be any key events received if focus has moved to another
application, so the next event can still be a FocusIn, even if there are key
state changes while focus was in the other app.

I don't think it is worthwhile having this block of code as it introduces the
possibility of a false positive.  Without the block, there are only false
negatives.

I wouldn't expect detecting repeat across windows to be important, but if you
still think it is important then the false positives could be avoided by
checking that the timestamps match.  Would they match in the situations that
you are trying to catch here?

[1] http://www.x.org/releases/current/doc/libX11/XKB/xkblib.html#Tracking_Changes_to_Keyboard_Controls
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-11-05 18:16:28 PST
(In reply to Karl Tomlinson (:karlt) from comment #21)
> Comment on attachment 821240 [details] [diff] [review]
> part.8 Implement KeyboardEvent.repeat on GTK
> >+            XKeyboardState keyboardState;
> >+            if (!XGetKeyboardControl(xEvent->xkey.display, &keyboardState)) {
> >+                break;
> >+            }
> 
> Here you want this information to know whether this key will affect the
> currently repeating key.
> However, XGetKeyboardControl() requires a round trip to the X server, and I
> don't want to do that for every key press.
> 
> It is possible to listen for XkbControlsNotifyEvents to detect when the
> per_key_repeat information changes.  Use XkbSelectEventDetails with
> XkbControlsNotify and XkbPerKeyRepeatMask to request these events [1].  It is
> not actually necessary to consider global_auto_repeat because press events
> will always have release events and the logic below won't set the state to
> REPEATING if global_auto_repeat is false.

I'm still researching about this...

> >+            if (xEvent->xfocus.mode != NotifyNormal) {
> 
> Other focus modes can also shift focus to another application, during which
> the key state might change without any notification.
> 
> Was there a reason for selecting only NotifyNormal?
> 
> >+            if (XEventsQueued(xEvent->xfocus.display, QueuedAfterReading)) {
> >+                XEvent nextEvent;
> >+                XPeekEvent(xEvent->xfocus.display, &nextEvent);
> >+                // We won't be deactivated if the next event is focus event.
> >+                if (nextEvent.type == FocusIn &&
> >+                    nextEvent.xfocus.mode == NotifyNormal) {
> >+                    break;
> >+                }
> 
> There won't be any key events received if focus has moved to another
> application, so the next event can still be a FocusIn, even if there are key
> state changes while focus was in the other app.

I wrote these code for preventing to forget key pressing state by clicking on window's border or title bar. When I mousedown and mosueup on there, we receive focus events which are not needed for us.

> I don't think it is worthwhile having this block of code as it introduces the
> possibility of a false positive.  Without the block, there are only false
> negatives.

Indeed, actually, it's rare case...

> I wouldn't expect detecting repeat across windows to be important, but if you
> still think it is important then the false positives could be avoided by
> checking that the timestamps match.  Would they match in the situations that
> you are trying to catch here?

The code is not detecting repeat across windows. I cannot find a way for it.

If GDK or X had focus in/out events in process level, it would be simpler, though :-(
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-11-06 18:07:31 PST
Created attachment 828396 [details] [diff] [review]
part.8 Implement KeyboardEvent.repeat on GTK

Storing the XKeyboardState. And removing the extra code in FocusOut. I'll use time stamp hack if somebody reports this bug and it's worthwhile to do it.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-11-06 18:09:50 PST
Oops, it might be better to make mKeyboardState static.
Comment 25 Karl Tomlinson (:karlt) 2013-11-06 23:02:18 PST
Comment on attachment 828396 [details] [diff] [review]
part.8 Implement KeyboardEvent.repeat on GTK

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24)
> Oops, it might be better to make mKeyboardState static.

Keeping mKeyboardState a member variable is nice because it is the
KeymapWrapper that initializes and updates the variable.  I guess a file-static variable would mean that XKBlib.h wouldn't need to be included.
I don't mind.

>+    memset(&mKeyboardState, 0, sizeof(mKeyboardState));

The count argument should be 1, but PodZero(&mKeyBoardState) is better,
because it works these things out for you.

>+    if (!XGetKeyboardControl(display, &mKeyboardState)) {

>+    if (!XkbSelectEventDetails(display, XkbUseCoreKbd, XkbControlsNotify,
>+                               XkbPerKeyRepeatMask, XkbPerKeyRepeatMask)) {

Please reverse the order of these.  Although the race condition is unlikely to
matter, registering for notification of changes should happen before getting
the current state.

++    gdk_window_remove_filter(nullptr, (GdkFilterFunc)FilterEvents, this);

I would prefer declaring FilterEvents with a gpointer third argument and static_cast<KeymapWrapper*>, so this (GdkFilterFunc) cast can be removed, which would be a little more type-safe.

++            if (xkbEvent->any.xkb_type != XkbControlsNotify) {
++                break;
++            }
++            if (!XGetKeyboardControl(xkbEvent->any.display,
++                                     &aSelf->mKeyboardState)) {

Please test xkbEvent->changed_ctrls & XkbPerKeyRepeatMask before calling
XGetKeyboardControl, because its a simple check and other code in Gecko or
libraries may register for other XkbControlsNotify events.
Comment 28 Jean-Yves Perrier [:teoli] 2015-09-30 01:53:48 PDT
cvrebert created the docs:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat
and
https://developer.mozilla.org/en-US/Firefox/Releases/28#InterfacesAPIsDOM was up-to-date for a very long time.

Note You need to log in before you can comment on or make changes to this bug.