Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement DOM3 KeyboardEvent.repeat

RESOLVED FIXED in mozilla28

Status

()

Core
DOM: Events
--
enhancement
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({dev-doc-complete})

Trunk
mozilla28
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-], URL)

Attachments

(8 attachments, 6 obsolete attachments)

7.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.85 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.04 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Dave Yeo
: review+
Details | Diff | Splinter Review
1.12 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
3.34 KB, patch
mwu
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jchen
: review+
Details | Diff | Splinter Review
12.23 KB, patch
karlt
: review+
Details | Diff | Splinter Review
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.
Smaug:

Should I create a new interface like nsIDOM3KeyEvent? Or change nsIDOMKeyEvent?
Created attachment 479287 [details] [diff] [review]
Patch v0.1 (work in progress)
Created attachment 479297 [details] [diff] [review]
Patch v0.1.1 (work in progress)
Attachment #479287 - Attachment is obsolete: true
Summary: Implement DOM3 KeyEvent::repeat → Implement DOM3 KeyboardEvent.repeat
Blocks: 680829
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.
Attachment #758604 - Flags: feedback?(masayuki)
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.
Attachment #758604 - Flags: feedback?(masayuki) → feedback-
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.
Depends on the patch for bug 768287 on GTK.

I restart to work on this.
Depends on: 768287
Created attachment 821218 [details] [diff] [review]
part.1 Implement KeyboardEvent.repeat
Attachment #479297 - Attachment is obsolete: true
Attachment #758604 - Attachment is obsolete: true
Attachment #821218 - Flags: review?(bugs)
Created attachment 821221 [details] [diff] [review]
part.2 Implement KeyboardEvent.repeat on Windows
Attachment #821221 - Flags: review?(jmathies)
Created attachment 821224 [details] [diff] [review]
part.3 Implement KeyboardEvent.repeat on Mac
Created attachment 821225 [details] [diff] [review]
part.4 Implement KeyboardEvent.repeat on Android
Created attachment 821228 [details] [diff] [review]
part.5 Implement KeyboardEvent.repeat on Gonk
Created attachment 821234 [details] [diff] [review]
part.6 Implement KeyboardEvent.repeat on OS/2

I don't test this patch, actually, though.
Attachment #821234 - Flags: review?(daveryeo)
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.
Attachment #821236 - Flags: review?(romaxa)
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.
Attachment #821240 - Flags: review?(karlt)
FYI: test builds will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4f9a0ffa2621
Attachment #821224 - Flags: review?(bugs)
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.
Attachment #821228 - Attachment is obsolete: true
Attachment #821264 - Flags: review?(mwu)
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).
Attachment #821224 - Flags: review?(bugs) → review?(smichaud)
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.
Attachment #821225 - Attachment is obsolete: true
Attachment #821303 - Flags: review?(nchen)

Updated

4 years ago
Attachment #821221 - Flags: review?(jmathies) → review+
Attachment #821303 - Flags: review?(nchen) → review+
Attachment #821224 - Flags: review?(smichaud) → review+

Updated

4 years ago
Attachment #821218 - Flags: review?(bugs) → review+
Attachment #821218 - Flags: superreview?(jst)
Keywords: dev-doc-needed
Attachment #821236 - Flags: review?(romaxa) → review+

Updated

4 years ago
Attachment #821218 - Flags: superreview?(jst) → superreview+

Updated

4 years ago
Attachment #821234 - Flags: review?(daveryeo) → review+

Comment 20

4 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//
Attachment #821264 - Flags: review?(mwu) → review+
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
Attachment #821240 - Flags: review?(karlt) → review-
(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 :-(
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.
Attachment #821240 - Attachment is obsolete: true
Attachment #828396 - Flags: review?(karlt)
Oops, it might be better to make mKeyboardState static.
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.
Attachment #828396 - Flags: review?(karlt) → review+
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!
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.