Closed Bug 630813 Opened 14 years ago Closed 13 years ago

Store the native modifier mask information in GdkKeymap's wrapper class and use it instead of GDK_MOD*_MASK

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: [inbound])

Attachments

(9 files, 44 obsolete files)

part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap
6.48 KB, patch
karlt
: review+
Details | Diff | Splinter Review
part.4 Implement a modifier keys initializer for nsInputEvent
13.33 KB, patch
karlt
: review+
Details | Diff | Splinter Review
part.5 Move DOMKeyCodeToGdkKeyCode() and DOMKeyCodeToGdkKeyCode() into KeymapWrapper
11.36 KB, patch
Details | Diff | Splinter Review
part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper
10.49 KB, patch
karlt
: review+
Details | Diff | Splinter Review
part.2 Store native modifier key informationpart.2 Store native modifier key information
12.74 KB, patch
karlt
: review+
Details | Diff | Splinter Review
part.6 nsKeyEvent should be initialized by mozilla::widget::KeymapWrapper
30.66 KB, patch
Details | Diff | Splinter Review
part.7 Implement IsKeyPressEventNecessary() in mozilla::widget::KeymapWrapper
3.20 KB, patch
Details | Diff | Splinter Review
part.8 IsCtrlAltTab() should use KeymapWrapper
4.65 KB, patch
karlt
: review+
Details | Diff | Splinter Review
part.9 Should compute modifier key's DOM keycode from unshifted GDK keyval
5.91 KB, patch
karlt
: review+
Details | Diff | Splinter Review
We're converting native keycode GDK_Meta_(L|R) to DOM keycode NS_VK_META. However, isMeta is computed with GDK_MOD4_MASK. When I press "Super" key which is Windows key on physical key on Ubuntu and Fedora, the bit is true. I think that the keycode mapping is true, but the modifier state is wrong. And I suspect that we shouldn't use GDK_MOD*_MASK if it's possible. I mean that if we can know which mod mask is mapped to alt, super, hyper and meta, we should check it. See also bug 630811. js handlers should be able to check it by new DOM3's method what modifier keys are being pressed.
Attached patch Patch v1.0 (obsolete) — — Splinter Review
Apply the patches for bug 630810 and bug 630817 before you apply this patch.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Blocks: 631165
Depends on: 630810, 630817
Attached patch Patch v1.1 (obsolete) — — Splinter Review
Attachment #510515 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — — Splinter Review
Attachment #510518 - Attachment is obsolete: true
Attached patch Patch v1.3 (obsolete) — — Splinter Review
Attachment #510527 - Attachment is obsolete: true
Attached patch Patch v1.4 (obsolete) — — Splinter Review
Attachment #510926 - Attachment is obsolete: true
Comment on attachment 522587 [details] [diff] [review] Patch v1.4 This patch implements a wrapper class for GdkKeymap, the name is KeymapWrapper. It is created only one instance per GdkKeymap instance. And this patch supports multi-GdkKeymap instance environment (I'm not sure there is such environment). And KeymapWrapper manages which key is mapped to what modifier key flags. If a key mapped to two or more modifier keys, KeymapWrapper ignores the non-highest-priority modifier keys for the key. KeymapWrapper is a refcountable class. All nsWindow instances reference it.
Attachment #522587 - Flags: review?(karlt)
Attached patch Patch v1.4.1 (obsolete) — — Splinter Review
updated for latest trunk.
Attachment #522587 - Attachment is obsolete: true
Attachment #528276 - Flags: review?(karlt)
Attachment #522587 - Flags: review?(karlt)
Comment on attachment 528276 [details] [diff] [review] Patch v1.4.1 I'm going to update this patch for latest trunk.
Attachment #528276 - Flags: review?(karlt)
Attached patch Patch v1.5 (obsolete) — — Splinter Review
Attachment #528276 - Attachment is obsolete: true
Attachment #551684 - Flags: review?(karlt)
The super and hyper keys were originally on the space cadet keyboard. http://en.wikipedia.org/wiki/Space-cadet_keyboard Since then, subsequent manufacturers have chosen their own names for the keys (and in some cases charged people to use the new names/symbols). GDK and parts of X11 are stuck with the super and hyper names, but the rest of the world has moved on, so I don't think we should expose the obsolete names to the web. These days, xkb usually uses "Super" to refer to the LWIN and RWIN keys. The "Hyper" virtual modifier is usually tied to the same modifier. Even with Macintosh keyboards, the command keys are called LWIN and RWIN (since 2005), and are mapped to Super keys. The Meta virtual modifier is usually mapped to the same modifier as the ALT keys. I think we should interpret a "Super" key as VK_WIN. It is possible (with the altwin:hyper_win option) to configure LWIN and RWIN as Hyper keys, and that's the only configuration that generates Hyper keys, so could also interpret a "Hyper" key as VK_WIN.
Comment on attachment 551684 [details] [diff] [review] Patch v1.5 The changes in the patch here that are unrelated to the original report make this difficult to review. There is no description of why the changes are proposed, and I suspect they would be better performed in separate changesets. I'll keep reviewing the current patch but the way to speed up the process would be to separate out unrelated changes. > // Don't pass modifiers as NS_KEY_PRESS events. > // TODO: Instead of selectively excluding some keys from NS_KEY_PRESS events, > // we should instead selectively include (as per MSDN spec; no official > // spec covers KeyPress events). >- if (aEvent->keyval == GDK_Shift_L >- || aEvent->keyval == GDK_Shift_R >- || aEvent->keyval == GDK_Control_L >- || aEvent->keyval == GDK_Control_R >- || aEvent->keyval == GDK_Alt_L >- || aEvent->keyval == GDK_Alt_R >- || aEvent->keyval == GDK_Meta_L >- || aEvent->keyval == GDK_Meta_R) { >+ if (KeymapWrapper::IsGdkModifierKeyval(aEvent->keyval)) { > return TRUE; > } Do you expect this to be better than aEvent.is_modifier? Can this change be in a separate patch?
Okay, I separated the patch. The every separated patch can build, but the each implementation may not make sense only in each patch. Such things might make sense with later patches in this bug. This patch just makes KeymapWrapper class which is a wrapper of GdkKeymap. When the keymap is destroyed or changed, the class detaches or resets the instance. The class is created only one instance for an instance of GdkKeymap. That is shared between nsWindows. If there are two or more GdkDisplay, there may be two or more instances of KeymapWrapper. But I'm not sure what environment is so.
Attachment #551684 - Attachment is obsolete: true
Attachment #558392 - Flags: review?(karlt)
Attachment #551684 - Flags: review?(karlt)
This patch stores the relation between bit of modifier mask and modifier keys. The code is stolen from gdk_keyboard_get_modmap_masks() in nsWindow.cpp. It will be removed by next patch.
Attachment #558393 - Flags: review?(karlt)
This patch makes nsWindow::GetToggledKeyState() use KeymapWrapper.
Attachment #558394 - Flags: review?(karlt)
This patch implements an initializer of nsInputEvent's modifier keys.
Attachment #558397 - Flags: review?(karlt)
This implements initializers for nsKeyEvent.
Attachment #558398 - Flags: review?(karlt)
Reimplements IsCtrlAltTab().
Attachment #558400 - Flags: review?(karlt)
Remapping the GDK modifier keys and DOM keycode. And defines ALTGR keycode for some Latin keyboard layouts. Alt and Meta keys on GTK are mapped DOM Alt keycode. Super and Hyper keys on GTK are mapped DOM Win keycode. AltGr key on Windows does NOT exist. On Windows, AltGr equals Ctrl + Alt. When I press AltGr key on Windows, Ctrl keydown and Alt keydown are fired. AltGr key on Mac does NOT exist too. On Mac, Option key is Alt key in DOM event. But it only behaves as AltGr key. So, it might be better to use Alt key code for AltGr like keys on GTK. But I'm not sure. If we do so, web apps cannot distinguish Alt and AltGr... Should we emulate the Windows' behavior on Linux?
Attachment #558401 - Flags: superreview?(Olli.Pettay)
Attachment #558401 - Flags: review?(karlt)
This patch implements a method which checks whether the GDK key event should cause dispatching keypress event or not.
Attachment #558402 - Flags: review?(karlt)
Sorry for the spam. A change which should be in part.7 was in part.9.
Attachment #558401 - Attachment is obsolete: true
Attachment #558403 - Flags: superreview?(Olli.Pettay)
Attachment #558403 - Flags: review?(karlt)
Attachment #558401 - Flags: superreview?(Olli.Pettay)
Attachment #558401 - Flags: review?(karlt)
Adding some comments in the keycode table.
Attachment #558404 - Flags: review?(karlt)
(In reply to Masayuki Nakano (Mozilla Japan) from comment #18) > Remapping the GDK modifier keys and DOM keycode. And defines ALTGR keycode > for some Latin keyboard layouts. > > Alt and Meta keys on GTK are mapped DOM Alt keycode. > Super and Hyper keys on GTK are mapped DOM Win keycode. > > AltGr key on Windows does NOT exist. On Windows, AltGr equals Ctrl + Alt. > When I press AltGr key on Windows, Ctrl keydown and Alt keydown are fired. > > AltGr key on Mac does NOT exist too. On Mac, Option key is Alt key in DOM > event. But it only behaves as AltGr key. > > So, it might be better to use Alt key code for AltGr like keys on GTK. But > I'm not sure. If we do so, web apps cannot distinguish Alt and AltGr... > Should we emulate the Windows' behavior on Linux? I think I agree with your approach of having a separate ALTGR keycode.
Comment on attachment 558392 [details] [diff] [review] Patch part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap (In reply to Masayuki Nakano (Mozilla Japan) from comment #12) > The class is created only one instance for an instance of GdkKeymap. That is > shared between nsWindows. If there are two or more GdkDisplay, there may be > two or more instances of KeymapWrapper. But I'm not sure what environment is > so. Yes, there is one GdkKeymap per GdkDisplay for the lifetime of the display. We only use one GdkDisplay, and there are already places in the code that won't work with multiple displays, so I probably would have just used static storage or a singleton class. However, getting the display from the window is "correct", so we can use your KeymapWrapper per GdkKeymap/GdkDisplay approach, but I'd like to see it simplified somewhat. >+ * |mKeymap| is only for holding the KeymapWrapper instance for the latest >+ * query. DON'T use this member directly because the associated keymap >+ * for this window might be swapped with another keymap. >+ * Use GetKeymap() it when you need KeymapWrapper. >+ */ >+ nsRefPtr<mozilla::widget::KeymapWrapper> mKeymap; >+ mozilla::widget::KeymapWrapper* GetKeymap() >+ { >+ mKeymap = mozilla::widget::KeymapWrapper::GetInstanceFor(mGdkWindow); >+ return mKeymap; Things can be simplified by keeping the KeymapWrapper alive for the lifetime of the GdkKeymap. Then there is no need for the client methods (or the nsWindow) to hold a reference to the KeymapWrapper. nsWindow::mKeymap can be removed and as well as the KeymapWrapper reference counting. KeymapWrapper::OnDestroyKeymap() can delete the KeymapWrapper instead of Detach()ing. >+ /** >+ * sLastUsedInstance caches last used instance of this class. On most >+ * environment, this class is created only one. If we get instance via >+ * GdkKeymap with g_object_get_data(), it might cause performance problem >+ * on slow environment. >+ */ >+ static KeymapWrapper* sLastUsedInstance; // [weak] Don't bother with this optimization. g_object_get_data should not be too slow for keyboard event handling. Simpler code is better than micro-optimization. >+#ifdef MOZ_LOGGING >+#define FORCE_PR_LOG /* Allow logging in the release build */ >+#endif // MOZ_LOGGING There is a lot of logging code in these patches. That's fine for debug builds, but is going to be rarely used in release builds. Can we turn off logging in release builds and ask bug reporters to download a debug (or try server) build if necessary? >+#ifdef MOZ_X11 >+ GdkDisplay* gdkDisplay = >+ gdk_x11_lookup_xdisplay(GDK_WINDOW_XDISPLAY(aGdkWindow)); >+ gdkKeymap = gdk_keymap_get_for_display(gdkDisplay); >+ if (!gdkKeymap) { >+ gdkKeymap = gdk_keymap_get_default(); >+ } >+#else // MOZ_X11 >+ gdkKeymap = gdk_keymap_get_default(); >+#endif // MOZ_X11 gdk_keymap_get_for_display(gdk_drawable_get_display(aGdkWindow)) which will be fine on all platforms and there is no need to null check. >+/* static */ already_AddRefed<KeymapWrapper> >+KeymapWrapper::GetInstanceFor(GdkWindow* aGdkWindow) >+{ >+ GdkKeymap* gdkKeymap = GetGdkKeymap(aGdkWindow); >+ NS_ENSURE_TRUE(gdkKeymap, nsnull); No need to check for null GdkKeymap. >+ g_signal_connect(G_OBJECT(mGdkKeymap), "keys-changed", >+ (GCallback)OnKeysChanged, this); No need for the G_OBJECT cast. >+/* static */ void >+KeymapWrapper::OnDestroyKeymap(KeymapWrapper* aKeymapWrapper) >+{ >+ PR_LOG(gKeymapWrapperLog, PR_LOG_ALWAYS, >+ ("KeymapWrapper: OnDestroyKeymap, aKeymapWrapper=%p", >+ aKeymapWrapper)); >+ NS_ENSURE_TRUE(aKeymapWrapper, ); No need to check for null argument. >+ * all requierments of us, therefore, we need to access lower level APIs. "requierments of us" -> "our needs" >+ * This enum's order is very important. That means the priority of >+ * modifier keys which share one modifier key mask. >+ MOD_INDEX_META, >+ MOD_INDEX_SUPER, >+ MOD_INDEX_HYPER, I haven't grasped exactly how this is used yet, but the priority order that GDK uses is Super, Hyper, Meta, so it may make sense to use the same order.
Attachment #558392 - Flags: review?(karlt) → review-
Comment on attachment 558392 [details] [diff] [review] Patch part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap >+ /** >+ * mModifierKeyMasks is bit masks for each modifier key. >+ * This enum's order is very important. That means the priority of >+ * modifier keys which share one modifier key mask. >+ * See the implementation of InitBySystemSettings() for the detail. >+ */ >+ enum { >+ MOD_INDEX_CAPS_LOCK = 0, >+ MOD_INDEX_NUM_LOCK, >+ MOD_INDEX_SCROLL_LOCK, >+ MOD_INDEX_SHIFT, >+ MOD_INDEX_CTRL, >+ MOD_INDEX_ALT, >+ MOD_INDEX_META, >+ MOD_INDEX_SUPER, >+ MOD_INDEX_HYPER, >+ MOD_INDEX_ALTGR, >+ COUNT_OF_MOD_INDEX, >+ MOD_INDEX_INVALID = COUNT_OF_MOD_INDEX >+ }; >+ guint mModifierKeyMasks[COUNT_OF_MOD_INDEX]; >+ >+ /** >+ * @param aGdkKeyval A GDK defined modifier key value such as >+ * GDK_Shift_L. >+ * @return Returns a index of mModifierKeyMasks for >+ * aGdkKeyval. If the given key code isn't >+ * a modifier key, returns eInvalidIndex. >+ */ >+ static PRUint32 GetIndex(guint aGdkKeyval); The distinction between modifier states and keys that change a modifier states is getting lost a bit here. The enum is a list of modifiers rather than keys, so I suggest referring to these as "modifiers" rather than "modifier keys". That would make the array here "mModifierMasks". Can GetIndex be renamed to GetModifierIndex to make its function clearer? Can the enum be changed from anonymous to named and GetModifierIndex() return that enum type? That would provide a bit more type safety and clarity.
Comment on attachment 558393 [details] [diff] [review] Patch part.2 Store native modifier key masks I'd feel more comfortable with MOD_INDEX_SHIFT, _CTRL, and _CAPS_LOCK hard coded. That is if this is a mapping from virtual modifiers to the 8 real modifiers. (If this is intended as a mapping from KeySyms to modifiers that the keys affect then it seems something more complicated is happening.) It makes sense to decipher mod1 to mod5 using KeySyms, as their meaning is not predefined (though GDK assumes mod1 is Alt), but shift/lock/control are well defined. From my experiments, it seems that the XGetModifierMapping output is not as reliable: If I run the following commands, it changes the output of xmodmap to indicate that Caps_Lock is a control modifier, but Caps_Lock still locks bit 2 of the modifier state (and doesn't set bit 3): xmodmap -e "remove lock = Caps_Lock" xmodmap -e "add control = Caps_Lock" However, if I undo that (by running "setxkbmap", for example), and then run the following commands, Control_L does start to behave as lock modifier (and no longer a control modifier), locking bit 2 of the modifier state. xmodmap -e "remove control = Control_L" xmodmap -e "add lock = Control_L" >+ if (bit >= NS_ARRAY_LENGTH(modKeys)) { >+ continue; >+ } This condition will never be met, so the test is unnecessary. >+ // TODO: User should be customizable which bit is which modifier key by >+ // prefs. The GDK port now only supports X11, so InitByFixedValues will only be used on out-of-memory conditions, and no need to have prefs.
Attachment #558393 - Flags: review?(karlt) → review-
Comment on attachment 558394 [details] [diff] [review] Patch part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper Note that GetToggledKeyState is not currently used and the windows and cocoa ports return different things. The windows version returns the state at the last event, while the cocoa version returns the state right now. Do you have plans to use this method somewhere? I'd be happy to make it return NS_ERROR_NOT_IMPLEMENTED. >+ nsRefPtr<KeymapWrapper> keymapWrapper = GetInstanceFor(aGdkWindow); >+ NS_ENSURE_TRUE(keymapWrapper, PR_FALSE); GetInstanceFor never returns NULL, so no need to check keymapWrapper. >+ /** >+ * There are two IsModifierKeyPressed(). >+ * >+ * One has aGdkWindow in its arguments and static. This method checks >+ * the modifier key state of the keymap which is associated to the >+ * window. >+ * >+ * The other has aModifierState in its arguments. This method just >+ * checks whether aGdkKeyval (MUST be modifier key value) is pressed or >+ * not. >+ * >+ * Note that this returns TRUE even when you set GDK_Shfit_L but actual >+ * pressed key is GDK_Shift_R. >+ * >+ * @param aGdkKeyval A keyval of GDK defined modifier keys such >+ * as GDK_Shift_L. >+ * @param aGdkWindow A GdkWindow which you want to check the >+ * state on. Must not be NULL if you support >+ * multi-keymaps. >+ * @param aModifierState Combination of GdkModifierType which you >+ * want to test with aGdkKeyval. >+ * @return Returns TRUE if aGdkKeyval is a modifier key >+ * value and the modifier key is pressed. >+ * Otherwise, returns FALSE. >+ */ >+ static PRBool IsModifierKeyPressed(guint aGdkKeyval, >+ GdkWindow* aGdkWindow); >+ PRBool IsModifierKeyPressed(guint aGdkKeyval, guint aModifierState); I wonder whether we can clarify that these are not actually testing whether GDK_Shift_L key is down. (It may be the Shift_R key that is down.) How about naming the methods "IsModifierActive"? Would it make sense if these took a modifier index enum as an argument instead of a GDK keyval? I assume we don't have an existing XP modifier enum. Perhaps the enum can be made public.
Attachment #558394 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #26) > Note that GetToggledKeyState is not currently used and the windows and cocoa > ports return different things. The windows version returns the state at the > last event, while the cocoa version returns the state right now. > > Do you have plans to use this method somewhere? > I'd be happy to make it return NS_ERROR_NOT_IMPLEMENTED. Thank you for your review, first. The method is necessary for bug 259059. I'm not working on the bug due to lower priority than current work. However, I'm still thinking that I'd like to fix it ASAP. I guess it's easier to fix it than a couple years ago since we have new GUI parts and multiple layer background, though.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #27) > The method is necessary for bug 259059. OK. Makes sense to keep the method working then, thanks.
Comment on attachment 558403 [details] [diff] [review] Patch part.7 (Re)Mapping Meta, Super, Hyper and other AltGr like keys to DOM keycode > { NS_VK_SHIFT, GDK_Shift_L }, > { NS_VK_SHIFT, GDK_Shift_R }, > { NS_VK_CONTROL, GDK_Control_L }, > { NS_VK_CONTROL, GDK_Control_R }, > { NS_VK_ALT, GDK_Alt_L }, > { NS_VK_ALT, GDK_Alt_R }, >- { NS_VK_META, GDK_Meta_L }, >- { NS_VK_META, GDK_Meta_R }, >+ // The Meta virtual modifier is usually mapped to the same modifier >+ // as the ALT keys. >+ { NS_VK_ALT, GDK_Meta_L }, >+ { NS_VK_ALT, GDK_Meta_R }, Is this true with other browsers? This change is rather, hmm, surprising. > { NS_VK_NONCONVERT, GDK_Muhenkan }, > // { NS_VK_ACCEPT, GDK_XXX }, >- { NS_VK_MODECHANGE, GDK_Mode_switch }, >+ // { NS_VK_MODECHANGE, GDK_XXX }, Could you explain this change?
(In reply to Olli Pettay [:smaug] from comment #29) > Comment on attachment 558403 [details] [diff] [review] [diff] [details] [review] > Patch part.7 (Re)Mapping Meta, Super, Hyper and other AltGr like keys to DOM > keycode > > > > { NS_VK_SHIFT, GDK_Shift_L }, > > { NS_VK_SHIFT, GDK_Shift_R }, > > { NS_VK_CONTROL, GDK_Control_L }, > > { NS_VK_CONTROL, GDK_Control_R }, > > { NS_VK_ALT, GDK_Alt_L }, > > { NS_VK_ALT, GDK_Alt_R }, > >- { NS_VK_META, GDK_Meta_L }, > >- { NS_VK_META, GDK_Meta_R }, > >+ // The Meta virtual modifier is usually mapped to the same modifier > >+ // as the ALT keys. > >+ { NS_VK_ALT, GDK_Meta_L }, > >+ { NS_VK_ALT, GDK_Meta_R }, > Is this true with other browsers? > This change is rather, hmm, surprising. When I press Alt key: Current Gecko Chromium without others 0x12 (DOM_VK_ALT) 0x12 with Shift 0xE0 (DOM_VK_META) 0x5B with Control 0x12 (DOM_VK_ALT) 0x12 with Shift+Control 0xE0 (DOM_VK_META) 0x5B And note that Win key always returns 0x5B on Chromium. That's strange... Anyway, we don't need to worry about the compatibility with other browser for GDK_Meta_* (Shift+Alt). For compatibility with other platform's Gecko, we should set 0x12 (DOM_VK_ALT) when shift key is pressed. > > { NS_VK_NONCONVERT, GDK_Muhenkan }, > > // { NS_VK_ACCEPT, GDK_XXX }, > >- { NS_VK_MODECHANGE, GDK_Mode_switch }, > >+ // { NS_VK_MODECHANGE, GDK_XXX }, > Could you explain this change? GDK_Mode_switch is a key code for a key which switches keyboard layout. VK_MODECHANGE on Windows is a key code for a key which changes IME's input mode. So, they are really different key. It's a mismapping.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30) > GDK_Mode_switch is a key code for a key which switches keyboard layout. Oops, this is wrong. GDK_Mode_switch is an AltGr key.
Hmm, during testing new patches on Ubuntu 11.10, I found a new issue in the design of my patches. In default settings, GDK_Meta_* is mapped on Shifted GDK_Alt_*. And the modifier flag is shared between them. When I choose "Meta is mapped to Left Win", the left win key becomes GDK_Meta_*. And the modifier flag when GDK_Meta_* is pressed is same as GDK_Super_* and GDK_Hyper_*'s flag. I think that we should manage modifiers by each bit. E.g., first bit is activated by what key*s*. And what name is better for the bit.
Ah, but a key might activate two or more bits...
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30) > (In reply to Olli Pettay [:smaug] from comment #29) > > >- { NS_VK_META, GDK_Meta_L }, > > >- { NS_VK_META, GDK_Meta_R }, > > >+ // The Meta virtual modifier is usually mapped to the same modifier > > >+ // as the ALT keys. > > >+ { NS_VK_ALT, GDK_Meta_L }, > > >+ { NS_VK_ALT, GDK_Meta_R }, > > Is this true with other browsers? > > This change is rather, hmm, surprising. > > When I press Alt key: > > Current Gecko Chromium > without others 0x12 (DOM_VK_ALT) 0x12 > with Shift 0xE0 (DOM_VK_META) 0x5B > with Control 0x12 (DOM_VK_ALT) 0x12 > with Shift+Control 0xE0 (DOM_VK_META) 0x5B > > And note that Win key always returns 0x5B on Chromium. That's strange... > > Anyway, we don't need to worry about the compatibility with other browser > for GDK_Meta_* (Shift+Alt). For compatibility with other platform's Gecko, > we should set 0x12 (DOM_VK_ALT) when shift key is pressed. I imagine, in this situation of mapping from keycode to DOM virtual key, the quirky Meta keycode from Alt+Shift could be resolved by considering the unshifted keycode when determining the DOM virtual key. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32) > Hmm, during testing new patches on Ubuntu 11.10, I found a new issue in the > design of my patches. > > In default settings, GDK_Meta_* is mapped on Shifted GDK_Alt_*. And the > modifier flag is shared between them. > > When I choose "Meta is mapped to Left Win", the left win key becomes > GDK_Meta_*. And the modifier flag when GDK_Meta_* is pressed is same as > GDK_Super_* and GDK_Hyper_*'s flag. > > I think that we should manage modifiers by each bit. E.g., first bit is > activated by what key*s*. And what name is better for the bit. I fear that trying to interpret/guess the mapping from keys to modifiers is just going to be too difficult. Is the key-to-modifier mapping needed only for Bug 630817? I think the spec should just recognise that the state of the modifier bits for a modifier key down/up event depends on the platform. On X11, sometimes it will not be possible to reproduce the behavior described in the examples at http://www.w3.org/TR/2006/WD-DOM-Level-3-Events-20060413/keyset.html#Modifiers When the Ctrl/Alt/Shift keys are sticky keys, the modifier bit remains set after the key is first released and becomes unset when the next (any) key is released. I think we have a much better chance of producing a mapping from X11 modifier bits to DOM modifier state, even if we end up choosing some order of priority of virtual modifiers when multiple virtual modifiers (super/hyper/meta) are mapped to a single X11 modifier state bit.
(In reply to Karl Tomlinson (:karlt) from comment #34) > Is the key-to-modifier mapping needed only for Bug 630817? For bug 630811. > I think the spec should just recognise that the state of the modifier bits > for a modifier key down/up event depends on the platform. On X11, sometimes > it will not be possible to reproduce the behavior described in the examples > at > http://www.w3.org/TR/2006/WD-DOM-Level-3-Events-20060413/keyset. > html#Modifiers I agree. > When the Ctrl/Alt/Shift keys are sticky keys, the modifier bit remains set > after the key is first released and becomes unset when the next (any) key is > released. Did you mean that we should ignore the modifier keypress events on other applications? > I think we have a much better chance of producing a mapping from X11 > modifier bits to DOM modifier state, even if we end up choosing some order > of priority of virtual modifiers when multiple virtual modifiers > (super/hyper/meta) are mapped to a single X11 modifier state bit. I'm not sure what you meant here.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35) > (In reply to Karl Tomlinson (:karlt) from comment #34) > > When the Ctrl/Alt/Shift keys are sticky keys, the modifier bit remains set > > after the key is first released and becomes unset when the next (any) key is > > released. > > Did you mean that we should ignore the modifier keypress events on other > applications? I don't understand "other applications" in your question. However, my statement was just an example of a situation where it is hard to guess how key presses will affect modifier state. Sounds like we agree that is difficult. > > I think we have a much better chance of producing a mapping from X11 > > modifier bits to DOM modifier state, even if we end up choosing some order > > of priority of virtual modifiers when multiple virtual modifiers > > (super/hyper/meta) are mapped to a single X11 modifier state bit. > > I'm not sure what you meant here. I think it should be possible to make a reasonable interpretation of X11 modifier state bit-masks to determine DOM modifier states. i.e. We should be able to map from the "state" field in key and mouse events, to values corresponding to possible values of the "keyArg" parameter to getModifierState. > > Is the key-to-modifier mapping needed only for Bug 630817? > > For bug 630811. I don't think bug 630811 needs a key-to-modifier mapping. The "getModifierState" method can only be invoked on an event, and X11 key and mouse events contain a state bitmask. The distinction I'm making is that we need only a bitmask-to-modifier mapping, not a key-to-modifier mapping.
(In reply to Karl Tomlinson (:karlt) from comment #36) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35) > > (In reply to Karl Tomlinson (:karlt) from comment #34) > > > When the Ctrl/Alt/Shift keys are sticky keys, the modifier bit remains set > > > after the key is first released and becomes unset when the next (any) key is > > > released. > > > > Did you mean that we should ignore the modifier keypress events on other > > applications? > > I don't understand "other applications" in your question. > However, my statement was just an example of a situation where it is hard to > guess how key presses will affect modifier state. Sounds like we agree that > is difficult. I imagined that you thought that we should manage modifier state ourselves. When *we* receive a modifier keydown event, we set a bit. When we receive a modifier keyup event, we unset the bit. I.e., if user changes application from/to another application during pressing one or more modifier keys, it causes failing to sync the state. > > > I think we have a much better chance of producing a mapping from X11 > > > modifier bits to DOM modifier state, even if we end up choosing some order > > > of priority of virtual modifiers when multiple virtual modifiers > > > (super/hyper/meta) are mapped to a single X11 modifier state bit. > > > > I'm not sure what you meant here. > > I think it should be possible to make a reasonable interpretation of X11 > modifier state bit-masks to determine DOM modifier states. i.e. We should > be able to map from the "state" field in key and mouse events, to values > corresponding to possible values of the "keyArg" parameter to > getModifierState. I agree. For that, we need to know which bit means which modifier. > > > Is the key-to-modifier mapping needed only for Bug 630817? > > > > For bug 630811. > > I don't think bug 630811 needs a key-to-modifier mapping. The > "getModifierState" method can only be invoked on an event, and X11 key and > mouse events contain a state bitmask. The distinction I'm making is that we > need only a bitmask-to-modifier mapping, not a key-to-modifier mapping. I understand.
(In reply to Karl Tomlinson (:karlt) from comment #11) > Do you expect this to be better than aEvent.is_modifier? Hmm, I realized that Eisu_toggle key's event indicates it TRUE. But we shouldn't think it as modifier... (the key actually doesn't change the state of GdkEventKey too.)
Summary: nsKeyEvent::isMeta and keycode conversion are mismatch → Store the native modifier mask information in GdkKeymap's wrapper class and use it instead of GDK_MOD*_MASK
Printing log is unsupported on release build. And KeymapWrapper is a singleton class.
Attachment #558392 - Attachment is obsolete: true
Attachment #558393 - Attachment is obsolete: true
Attachment #558394 - Attachment is obsolete: true
Attachment #558397 - Attachment is obsolete: true
Attachment #558398 - Attachment is obsolete: true
Attachment #558400 - Attachment is obsolete: true
Attachment #558402 - Attachment is obsolete: true
Attachment #558403 - Attachment is obsolete: true
Attachment #558404 - Attachment is obsolete: true
Attachment #574808 - Flags: review?(karlt)
Attachment #558397 - Flags: review?(karlt)
Attachment #558398 - Flags: review?(karlt)
Attachment #558400 - Flags: review?(karlt)
Attachment #558402 - Flags: review?(karlt)
Attachment #558404 - Flags: review?(karlt)
Attachment #558403 - Flags: superreview?(bugs)
Attachment #558403 - Flags: review?(karlt)
This patch defines widget level modifier as KeymapWrapper::Modifier. For using two or modifiers at once in some methods, KeymapWrapper::Modifiers is defined too. And this patch stores modifier key information for every hardware_keycode which is associated with a modifier keyval of GDK. This is useful for part.6. Also, mModifierMasks stores each modifier's bit mask except Shift, Control and CapsLock which are defined by GDK. This is useful for part.3.
Attachment #574813 - Flags: review?(karlt)
This patch defines KeymapWrapper::AllModifiersActive(). It should be able to check whether all of two or more specified modifiers are active or not. Therefore, I think IsModiferActive() isn't good name for now.
Attachment #574814 - Flags: review?(karlt)
This doesn't change any behavior, just moves them for later patches.
Attachment #574816 - Flags: review?(karlt)
This patch doesn't change the behavior except: 1. InitKeyEvent() uses GetModifierKey() with the hardware_keycode. Therefore, it can know which bit will be activated by the keypress correctly. 2. Using stored modifier masks in keyhell code.
Attachment #574818 - Flags: review?(karlt)
This patch is made from your idea (comment 34). This is only patch to change the DOM application's behavior. I removed all DOM keycode change code from this bug's patches. I think that we should fix this bug before reimplementing keycode computation. The reimplementing is very risky and it needs to land at same time. Therefore, this bug's patches shouldn't be a part of them.
Attachment #574819 - Flags: review?(karlt)
I think that we should check whether the other modifiers are inactive. Ubuntu doesn't do Ctrl+Alt+Tab if other modifiers except shift are active.
Attachment #574820 - Flags: review?(karlt)
Similar to part.8, IsContextMenuKeyEvent() should check whether other modifiers are inactive or not.
Attachment #574821 - Flags: review?(karlt)
See comment 38, we should check the keyval directly.
Attachment #574822 - Flags: review?(karlt)
Comment on attachment 574808 [details] [diff] [review] part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap > #define __nsGdkKeyUtils_h__ > >+#include "nspr.h" I expect nspr.h is probably no longer needed. >+ * all requierments of us, therefore, we need to access lower level APIs. Change "requierments of us" to "our needs". >+ * This is called when XPCOM is being shutdown. English uses two words "shut down" when used as a verb. The compound word "shutdown" is a noun. >+ virtual ~KeymapWrapper(); Does this need to be virtual? >+ if (!sInstance) { >+ return; >+ } >+ delete sInstance; >+} No need for null check and early return because delete is a no-op when sInstance is null. Can you explain why the Shutdown() method is needed? If deletion of the sInstance were to happen only when the GdkKeymap is destroyed, there would be no need to disconnect the signal handlers nor steal the MOZ_MODIFIER_KEYS data.
Comment on attachment 574808 [details] [diff] [review] part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap >+#define MOZ_MODIFIER_KEYS "MozKeymapWrapper" >+ // This is necessary for catching the destroying timing. >+ g_object_set_data_full(G_OBJECT(mGdkKeymap), MOZ_MODIFIER_KEYS, >+ this, (GDestroyNotify)OnDestroyKeymap); >+ g_object_steal_data(G_OBJECT(mGdkKeymap), MOZ_MODIFIER_KEYS); Now that the wrapper class is a singleton and there is no _get_data for MOZ_MODIFIER_KEYS, it would be clearer/tidier to use g_object_weak_ref instead of g_object_set_data_full (and g_object_weak_unref if the Shutdown() method is still required).
> Shutdown() I thought that if we didn't destroy the instance explicitly, the memory leak detector would think it's leaked. But I confirmed that this is wrong. So, we can remove the Shutdown() and make it simpler. And thank you for your advice for some English. It's very helpful for me.
Attachment #574808 - Attachment is obsolete: true
Attachment #574813 - Attachment is obsolete: true
Attachment #574814 - Attachment is obsolete: true
Attachment #574815 - Attachment is obsolete: true
Attachment #574816 - Attachment is obsolete: true
Attachment #574818 - Attachment is obsolete: true
Attachment #574819 - Attachment is obsolete: true
Attachment #581147 - Flags: review?(karlt)
Attachment #574808 - Flags: review?(karlt)
Attachment #574813 - Flags: review?(karlt)
Attachment #574814 - Flags: review?(karlt)
Attachment #574815 - Flags: review?(karlt)
Attachment #574816 - Flags: review?(karlt)
Attachment #574818 - Flags: review?(karlt)
Attachment #574819 - Flags: review?(karlt)
Attachment #581149 - Attachment is patch: true
Attachment #581155 - Attachment is patch: true
Attachment #581147 - Flags: review?(karlt) → review+
Comment on attachment 581150 [details] [diff] [review] part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper >+ * GetCurrentModifierState() returns current modifier key state on >+ * aGdkWindow. The "current" means actual state of hardware keyboard Remove "on aGdkWindow" here because the modifier key state does not depend on the window. The GdkWindow is just for the particular implementation, but see below. >+ * @param aGdkWindow Must not be NULL. >+ * NOTE: Even if you pass NULL, this returns >+ * default keymap's modifier key state. >+ * However, it's not good. You shouldn't >+ * do it. >+/* static */ guint >+KeymapWrapper::GetCurrentModifierState(GdkWindow* aGdkWindow) >+{ >+ NS_PRECONDITION(aGdkWindow, "aGdkWindow should not be NULL"); >+ GdkModifierType modifiers = static_cast<GdkModifierType>(0); >+ gdk_window_get_pointer(aGdkWindow, NULL, NULL, &modifiers); >+ return static_cast<guint>(modifiers); >+} gdk_display_get_pointer(gdk_display_get_default(), NULL, NULL, NULL, &state) can actually get the same result without a GdkWindow, and it is at least as efficient. There's is no need to initialize modifiers, but I see this is just moving code around. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #41) > This patch defines KeymapWrapper::AllModifiersActive(). It should be able to > check whether all of two or more specified modifiers are active or not. > Therefore, I think IsModiferActive() isn't good name for now. How about AreModifiersActive()? >+ * @return TRUE if all of modifieres in aModifiers are >+ * @return TRUE if aGdkModifierType indecates all of >+ * modifieres in aModifier are active. "modifiers" is the correct spelling of "modifiers". Change "indecates" to "indicates". >+ static bool AllModifiersActiveCurrently(Modifiers aModifiers, >+ GdkWindow* aGdkWindow); "AreModifiersCurrentlyActive" would be more natural English. r+ with the changes to comments and method names. Even better if you remove the GdkWindow parameter from GetCurrentModifierState, but you don't *need* to change that.
Attachment #581150 - Flags: review?(karlt) → review+
No need to update subsequent patches on the bug yet. I'll review the other patches as they are.
Comment on attachment 581150 [details] [diff] [review] part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper >+/* static */ bool >+KeymapWrapper::AllModifiersActiveCurrently(Modifiers aModifiers, >+ GdkWindow* aGdkWindow) >+{ >+ KeymapWrapper* keymapWrapper = GetInstance(aGdkWindow); >+ NS_ENSURE_TRUE(keymapWrapper, false); (In reply to Karl Tomlinson (:karlt) from comment #26) > GetInstanceFor never returns NULL, so no need to check keymapWrapper. Missed this one.
karlt: Thanks. But why did you skip part.2?
Comment on attachment 581149 [details] [diff] [review] part.2 Store native modifier key information I haven't really skipped part 2, but I haven't looked at part 6 yet, so I don't yet know what mModifierKeys gets used for. These are my notes so far: -+ // share the Mod4. If such flag is set, we should ignore lower priority -+ // modifier keys for nsIDOMKeyEvent::shiftKey, nsIDOMKeyEvent::ctrlKey, -+ // nsIDOMKeyEvent::altKey and nsIDOMKeyEvent::metaKey. The reason is, -+ // if we don't do so, it breaks javascript applications which have been. -+ // The priority order is, CapsLock, NumLock, ScrollLock, Shift, Control, -+ // Alt, Meta, Super, Hyper. This is same as the enum for index. Can you describe please what your reason was for changing the approach here? With the change, two elements of the mModifierMask (typically Super and Hyper, and ALT and META) can have the same mask bit set. I don't see a need to keep separate SUPER and HYPER modifiers as these are really the WIN keys (comment 10). I worry about treating Alt as a Meta in the DOM because there AIUI Meta usually refers to a Mac command key, which is quite different. The keyboard layout is not designed with the intention that Shift+Alt generates a different modifier to Alt on its own. I think this Meta in the modmap was just added so that applications wanting a Meta key (e.g. emacs) use Mod1 (which is ALT) for this, but the Meta used in these apps is quite different from a Mac Command key. The comment in symbols/pc is "Fake keys for virtual<->real modifiers mapping". >+ Modifier modifiers[8]; >+ for (PRUint32 i = 0; i < ArrayLength(modifiers); i++) { >+ modifiers[i] = NOT_MODIFIER; >+ } >+ Unused, with your change in approach. >+ /** >+ * Modifiers is used for combination of Modifier. >+ * E.g., |Modifiers modifiers = (SHIFT | CTRL);| means Shift and Ctrl. >+ */ >+ typedef PRInt32 Modifiers; Normally we use unsigned types for bit/flag sets like this. >+ inline guint GetModifierMask(Modifier aModifier) const; This looks to me like it would be better not inline.
(In reply to Karl Tomlinson (:karlt) from comment #63) > Comment on attachment 581149 [details] [diff] [review] > part.2 Store native modifier key information > > I haven't really skipped part 2, but I haven't looked at part 6 yet, so I > don't yet know what mModifierKeys gets used for. If you want, I'll post nsGtkKeyUtils.h and nsGtkKeyUtils.cpp which include all patches. > > These are my notes so far: > > -+ // share the Mod4. If such flag is set, we should ignore lower > priority > -+ // modifier keys for nsIDOMKeyEvent::shiftKey, nsIDOMKeyEvent::ctrlKey, > -+ // nsIDOMKeyEvent::altKey and nsIDOMKeyEvent::metaKey. The reason is, > -+ // if we don't do so, it breaks javascript applications which have > been. > -+ // The priority order is, CapsLock, NumLock, ScrollLock, Shift, > Control, > -+ // Alt, Meta, Super, Hyper. This is same as the enum for index. > > Can you describe please what your reason was for changing the approach here? > With the change, two elements of the mModifierMask (typically Super and > Hyper, > and ALT and META) can have the same mask bit set. > > I don't see a need to keep separate SUPER and HYPER modifiers as these are > really the WIN keys (comment 10). SUPER and HYPER, ALT and META are actually different modifier in API level. When I change keyboard layout option of GNOME UI, their values are sometimes different. After we and other browsers implement KeyboardEvent.getModifierState(), we may be able to make it simpler. But for now, for first implementation, I think it's better to use platform dependent structure. > I worry about treating Alt as a Meta in the DOM because there AIUI Meta > usually refers to a Mac command key, which is quite different. The keyboard > layout is not designed with the intention that Shift+Alt generates a > different > modifier to Alt on its own. I think this Meta in the modmap was just added > so > that applications wanting a Meta key (e.g. emacs) use Mod1 (which is ALT) for > this, but the Meta used in these apps is quite different from a Mac Command > key. The comment in symbols/pc is "Fake keys for virtual<->real modifiers > mapping". I agree with the idea about Meta. In my patch, the widget's META and DOM Meta are different. Actually, InitInputEvent() always sets false to aInputEvent.isMeta. I think that we should discuss getModifierState() in the ML. It's a new API, so, I think 'Meta' doesn't need to mean 'Cmd' key of Mac.
Comment on attachment 581147 [details] [diff] [review] part.1 Make KeymapWrapper which is a wrapper class of GdkKeymap >+/* static */ GdkKeymap* >+KeymapWrapper::GetGdkKeymap(GdkWindow* aGdkWindow) >+{ >+ return gdk_keymap_get_for_display(gdk_drawable_get_display(aGdkWindow)); >+} Sorry, this somehow got lost in my notes before. Now that the KeymapWrapper is a singleton, this can be replaced with gdk_keymap_get_default(), which would save having to pass the GdkWindow to a number of places.
Comment on attachment 581149 [details] [diff] [review] part.2 Store native modifier key information >+ Display* display = GDK_WINDOW_XDISPLAY(aGdkWindow); And this can use gdk_x11_display_get_xdisplay(gdk_display_get_default()));
Comment on attachment 581151 [details] [diff] [review] part.4 Implement a modifier keys initializer for nsInputEvent >+ * InitInputEventByCurrentModifiers() initializes the aInputEvent with >+ * GetCurrentModifierState(). So, the aGdkWindow's current modifier state >+ * will be used for isShift, isControl and etc. >+ */ >+ static void InitInputEventByCurrentModifiers(nsInputEvent& aInputEvent, >+ GdkWindow* aGdkWindow); This is not something that we want to encourage and there is only one client function, which is a bit special, so I think it is best that nsWindow::InitDragEvent (which is actually only for destination drag events) calls GetCurrentModifierState() and InitInputEvent(aInputEvent, modifierState) itself. I see nsWindow::InitDragEvent is already using gdk_display_get_pointer, so I think GetCurrentModifierState should also use that function to avoid the GdkWindow parameter. >+ void InitInputEvent(nsInputEvent& aInputEvent, >+ guint aModifierState); With GetInstance(), no longer requiring a GdkWindow, can we make this static? That would simplify many call sites. (Also GetInstance won't return NULL now, so no need to check it.) This patch should be good with those changes, but it's quite a change so I'd like to look over the new version, please.
Attachment #581151 - Flags: review?(karlt)
Attachment #581151 - Flags: review-
Attachment #581151 - Flags: feedback+
Comment on attachment 581153 [details] [diff] [review] part.5 Move DOMKeyCodeToGdkKeyCode() and DOMKeyCodeToGdkKeyCode() into KeymapWrapper In general, I prefer not bracing single line jump statements in "if" blocks, and I prefer not changing existing code just to change brace style, but this is fine for these functions.
Attachment #581153 - Flags: review?(karlt) → review+
Okay, KeymapWrapper should provide only static methods for outer.
Attachment #581147 - Attachment is obsolete: true
Attachment #587985 - Flags: review?(karlt)
Attachment #581154 - Attachment is obsolete: true
Attachment #587993 - Flags: review?(karlt)
Attachment #581154 - Flags: review?(karlt)
Attachment #581155 - Attachment is obsolete: true
Attachment #587994 - Flags: review?(karlt)
Attachment #581155 - Flags: review?(karlt)
Attachment #574820 - Attachment is obsolete: true
Attachment #587995 - Flags: review?(karlt)
Attachment #574820 - Flags: review?(karlt)
Attachment #574821 - Attachment is obsolete: true
Attachment #587996 - Flags: review?(karlt)
Attachment #574821 - Flags: review?(karlt)
Attachment #574822 - Attachment is obsolete: true
Attachment #587997 - Flags: review?(karlt)
Attachment #574822 - Flags: review?(karlt)
Comment on attachment 587995 [details] [diff] [review] part.8 Implement IsCtrlAltTabKeyEvent() in mozilla::widget::KeymapWrapper Ah, part.8 and part.9 may be wrong on current approach. I cancel the review requests.
Attachment #587995 - Flags: review?(karlt)
Attachment #587996 - Flags: review?(karlt)
Attachment #587985 - Flags: review?(karlt) → review+
Comment on attachment 587988 [details] [diff] [review] part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper >+KeymapWrapper::GetCurrentModifierState() >+{ >+ GdkModifierType modifiers; >+ gdk_window_get_pointer(NULL, NULL, NULL, &modifiers); Use gdk_display_get_pointer(gdk_display_get_default(), NULL, NULL, NULL, &modifiers) as gdk_window_get_pointer needs a GdkWindow with GTK3 and gdk_window_get_pointer does some extra work to find the child window, which we don't need.
Comment on attachment 587990 [details] [diff] [review] part.4 Implement a modifier keys initializer for nsInputEvent That looks much tidier, thanks. I'll be taking some vacation over the next week or two so I won't be able to finish off here immediately.
Attachment #587990 - Flags: review?(karlt) → review+
> I'll be taking some vacation over the next week or two so I won't be able to finish off here immediately. No problem, enjoy your vacation.
Attachment #587988 - Attachment is obsolete: true
Attachment #588286 - Flags: review?(karlt)
Comment on attachment 588286 [details] [diff] [review] part.3 nsWindow::GetToggledKeyState() should use mozilla::widget::KeymapWrapper Thanks!
Attachment #588286 - Flags: review?(karlt) → review+
Removed unnecessary modifiers checking. It's too hard and not an important matter of this bug...
Attachment #587995 - Attachment is obsolete: true
Attachment #588321 - Flags: review?(karlt)
Same as part.8.
Attachment #587996 - Attachment is obsolete: true
Attachment #588322 - Flags: review?(karlt)
Comment on attachment 587993 [details] [diff] [review] part.6 nsKeyEvent should be initialized by mozilla::widget::KeymapWrapper > using namespace mozilla; > >-#define MAX_UNICODE 0x10FFFF >+namespace mozilla { >+namespace widget { Does this change mean we can remove the "using namespace mozilla" now? >+ if (aGdkKeyEvent->is_modifier) { >+ ModifierKey* modifierKey = >+ keymapWrapper->GetModifierKey(aGdkKeyEvent->hardware_keycode); >+ if (modifierKey && modifierKey->mMask) { >+ // This key event is caused by pressing or releasing a modifier key. >+ if (aGdkKeyEvent->type == GDK_KEY_PRESS) { >+ // If new modifier key is pressed, add the pressed mod mask. >+ modifierState |= modifierKey->mMask; >+ } else { >+ // XXX If we could know the modifier keys state at the key >+ // release event, we should cut out changingMask from >+ // modifierState. >+ } The modifierKey->mMask test should be removed. I don't think it is ever 0, but if it could be, it is an unnecessary optimization. Moving the type == GDK_KEY_PRESS test to the outer block (with the is_modifier test) would skip some unnecessary searching for the ModifierKey. >+ * GetCharCodeFor() Computes what charcter is inputted by the key event "character" >+ PRUint32 GetCharCodeFor(const GdkEventKey *aGdkKeyEvent); This method can be static. >+ * @return Using level. Typicall, this is 0 or 1. >+ * If failed, this returns -1. "Typically" >+ KeymapWrapper::InitKeyEvent(aEvent, aGdkEvent); > > // The transformations above and in gdk for the keyval are not invertible > // so link to the GdkEvent (which will vanish soon after return from the > // event callback) to give plugins access to hardware_keycode and state. > // (An XEvent would be nice but the GdkEvent is good enough.) > aEvent.pluginEvent = (void *)aGdkEvent; > > aEvent.time = aGdkEvent->time; Would it be reasonable to also move this code to KeymapWrapper::InitKeyEvent()? If so, the nsWindow::InitKeyEvent() method would be unnecessary, as callers can use KeymapWrapper directly.
Attachment #587993 - Flags: review?(karlt) → review+
Comment on attachment 587987 [details] [diff] [review] part.2 Store native modifier key informationpart.2 Store native modifier key information >+ * A physical key may have two or more modifiers. E.g., Shift + Alt key >+ * may be Meta key. >+ */ >+ struct ModifierKey { >+ guint mHardwareKeycode; >+ Modifiers mModifiers; mModifiers seems to be unused now. I assume that means the comment about two or more modifiers and the Shift + Alt example can also be removed. >+ ModifierKey modifierKeyBuff; >+ ModifierKey* modifierKey = GetModifierKey(static_cast<guint>(keycode)); >+ if (!modifierKey) { >+ modifierKey = &modifierKeyBuff; >+ modifierKey->mHardwareKeycode = static_cast<guint>(keycode); >+ } >+ if (modifierKey == &modifierKeyBuff) { >+ mModifierKeys.AppendElement(modifierKeyBuff); >+ } The static_cast from KeyCode to guint should be unnecessary because KeyCode is a typedef for unsigned char. The situation with modifierKeyBuff can be simplified a bit so that this is not necessary, I think: * The ModifierKey constructor could require a hardware keycode parameter. * When GetModifierKey returns null, set modifierKey = mModifierKeys.AppendElement(keyCode)
Attachment #587987 - Flags: review?(karlt) → review-
Comment on attachment 597310 [details] [diff] [review] part.6 nsKeyEvent should be initialized by mozilla::widget::KeymapWrapper Thanks. Can you remove the declaration of nsWindow::InitKeyEvent from nsWindow.h, please?
(In reply to Karl Tomlinson (:karlt) from comment #90) > Can you remove the declaration of nsWindow::InitKeyEvent from nsWindow.h, > please? Oh...
Comment on attachment 597309 [details] [diff] [review] part.2 Store native modifier key informationpart.2 Store native modifier key information >+ modifierKey = mModifierKeys.AppendElement(ModifierKey(keycode)); I expect it is possible to AppendElement(keycode) (or from any single parameter constructor) but perhaps the constructor parameter would need to be of type KeyCode because an implicit conversion might not be performed. Anyway, if you think it is clearer this way, this is fine as is.
Attachment #597309 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #92) > Comment on attachment 597309 [details] [diff] [review] > part.2 Store native modifier key informationpart.2 Store native modifier key > information > > >+ modifierKey = mModifierKeys.AppendElement(ModifierKey(keycode)); > > I expect it is possible to AppendElement(keycode) (or from any single > parameter constructor) but perhaps the constructor parameter would need to > be of type KeyCode because an implicit conversion might not be performed. > > Anyway, if you think it is clearer this way, this is fine as is. Oh, I didn't have such idea. However, I think my code is clearer, so, I keep it.
karlt: The remaining patches are simple (part.7 is a little bit has side effect though), could you review them?
Comment on attachment 587997 [details] [diff] [review] part.10 Implement IsKeyPressEventNecessary() in mozilla::widget::KeymapWrapper Testing based on the DOM key code rather than the GDK keyval makes sense to me as this is effectively used for determining whether DOM events are sent. Given that, I wonder whether it would be better to init the NS_KEY_PRESS event and then check its keyCode rather than to get the keymap wrapper to work this out from the GdkEvent with another ComputeDOMKeyCode call. But this is OK as is if you prefer this.
Attachment #587997 - Flags: review?(karlt) → review+
Comment on attachment 588322 [details] [diff] [review] part.9 Implement IsContextMenuKeyEvent() in mozilla::widget::KeymapWrapper AFAIK there is no need to make this change, and it is losing the ability to test for inactive modifier state.
Attachment #588322 - Flags: review?(karlt) → review-
Comment on attachment 588321 [details] [diff] [review] part.8 Implement IsCtrlAltTabKeyEvent() in mozilla::widget::KeymapWrapper I feel that IsCtrlAltTabKeyEvent is too specific a method to add to KeymapWrapper. I'd prefer that the KeymapWrapper interface be as clean and simple as possible and the static method in nsWindow.cpp use AreModifiersActive() or a public version of this method.
Attachment #588321 - Flags: review?(karlt) → review-
Oh, okay. I just drop part.9 patch. And I'll post a new patch for part.8 which only changes in nsWindow.
Comment on attachment 587994 [details] [diff] [review] part.7 Should compute modifier key's DOM keycode from unshifted GDK keyval While this looks helpful for one case that this method gets wrong, I wonder whether the same approach would also be helpful for non-modifier keys in some situations. Would it make more sense to address this in bug 447757?
Yes, I'll work on for other keys in bug447757 after this bug. Basically, the normal keys' keycode should be computed from unshifted char on all keyboard layout but we should not ignore NumLock state. Otherwise, some special keys which are around normal keys like IME related keys such as Henkan key, Eisu-toggle key (Shifted Eisu-toogle key is CapsLock key on Japanese keyboard layout) shouldn't be used unshifted keycode because they actually have different meaning. However, as you thought, we should use unshifted key code for modifier keys, I think. If you want me to put off the patch to the bug, it's not problem. The new class is needed for preparing the bug. And it should be tested by Nightly testers.
I reorder the patches. This was part.10 which is r+.
Attachment #587994 - Attachment is obsolete: true
Attachment #587997 - Attachment is obsolete: true
Attachment #588321 - Attachment is obsolete: true
Attachment #588322 - Attachment is obsolete: true
Attachment #587994 - Flags: review?(karlt)
This was part.8, but IsCtrlAltTab() in nsWindow.cpp directly uses KeymapWrapper::AreModifiersActive().
Attachment #599474 - Flags: review?(karlt)
Attachment #599474 - Attachment is patch: true
This was part.7. So, if you want this to be put off next bug, I agree with that.
Attachment #599475 - Flags: review?(karlt)
Comment on attachment 599474 [details] [diff] [review] part.8 IsCtrlAltTab() should use KeymapWrapper >+ * modifieres in aModifier are active. Please, change "modifieres" to "modifiers" while you are here.
Attachment #599474 - Flags: review?(karlt) → review+
Comment on attachment 599475 [details] [diff] [review] part.9 Should compute modifier key's DOM keycode from unshifted GDK keyval I mark this r+ because it improves behavior. When working on bug 447757 please consider reworking this to share code as much as practical. For example, I assume NumLock could be considered even for modifier keyvals.
Attachment #599475 - Flags: review?(karlt) → review+
> For example, I assume NumLock could be considered even for modifier keyvals. I meant, we shouldn't compute keycode with ignoring NumLock state. Not the NumLock key itself. E.g., '4' of NumPad shouldn't be computed as 'U' key on notebook and 'Right arrow' key on Desktop's full keyboard. Other platform's patches always honor the NumLock state. Thank you for your review for these big changes. I'm going to land these patches today. And I'll post patches in bug 447757 next week or later.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: