Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 14 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 14 years ago

Smaug:

Should I create a new interface like nsIDOM3KeyEvent? Or change nsIDOMKeyEvent?

	Xidorn Quan [:xidorn] UTC+11
	Comment 4 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 11 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 6 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 11 years ago

Depends on the patch for bug 768287 on GTK.

I restart to work on this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 11 years ago

I don't test this patch, actually, though.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 11 years ago

Since I cannot build it on my environment, I don't test this patch actually.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 15 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 11 years ago

FYI: test builds will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4f9a0ffa2621

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 17 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 18 • 11 years ago

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).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 19 • 11 years ago

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.

	Michael Wu [:mwu]
	Comment 20 • 11 years ago

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//

	Karl Tomlinson (:karlt)
	Comment 21 • 11 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 11 years ago

(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 :-(

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 24 • 11 years ago

Oops, it might be better to make mKeyboardState static.

	Karl Tomlinson (:karlt)
	Comment 25 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 26 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/27c3093d6539
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0708417745
https://hg.mozilla.org/integration/mozilla-inbound/rev/3faeb00e56cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0b5a83799d
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5361e02d97
https://hg.mozilla.org/integration/mozilla-inbound/rev/c056b6fed0dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e542b50b324
https://hg.mozilla.org/integration/mozilla-inbound/rev/37677db9213e

Thank you, everybody!

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/27c3093d6539
https://hg.mozilla.org/mozilla-central/rev/4d0708417745
https://hg.mozilla.org/mozilla-central/rev/3faeb00e56cd
https://hg.mozilla.org/mozilla-central/rev/0b0b5a83799d
https://hg.mozilla.org/mozilla-central/rev/de5361e02d97
https://hg.mozilla.org/mozilla-central/rev/c056b6fed0dd
https://hg.mozilla.org/mozilla-central/rev/0e542b50b324
https://hg.mozilla.org/mozilla-central/rev/37677db9213e

	Jean-Yves Perrier [:teoli]
	Comment 28 • 9 years ago

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.