Last Comment Bug 630811 - Implement DOM3 KeyboardEvent.getModifierState()
: Implement DOM3 KeyboardEvent.getModifierState()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
:
:
Mentors:
http://www.w3.org/TR/DOM-Level-3-Even...
Depends on: 749553 749560 1023067 630813 630817 731878 753256 768287 769190 951021 951023 1009388
Blocks: 1249872 628165 680829 747013
  Show dependency treegraph
 
Reported: 2011-02-02 01:19 PST by Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
Modified: 2016-02-20 02:24 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.55 KB, text/html)
2012-04-18 18:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
part.1 Move modifiers from nsMouseEvent_base to nsInputEvent (21.75 KB, patch)
2012-04-18 20:12 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
part.2 Replace nsInputEvent::isShift, nsInputEvent::isControl, nsInputEvent::isAlt and nsInputEvent::isMeta (78.68 KB, patch)
2012-04-18 20:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
part.3 Improve nsDOMWindowUtils::send*Event() for new modifiers (15.69 KB, patch)
2012-04-18 20:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
part.4 Support new modifiers on all events derived from nsInputEvent on Windows (6.10 KB, patch)
2012-04-18 20:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
jmathies: review+
Details | Diff | Splinter Review
part.5 Support new modifiers on all events derived from nsInputEvent on GTK (9.80 KB, patch)
2012-04-18 20:32 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa (33.13 KB, patch)
2012-04-18 20:35 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent (20.89 KB, patch)
2012-04-18 20:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
testcase (1.54 KB, text/html)
2012-04-18 21:23 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa (9.75 KB, patch)
2012-04-23 02:10 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent (20.93 KB, patch)
2012-04-23 18:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
jst: superreview+
Details | Diff | Splinter Review
testcase (1.81 KB, text/html)
2012-07-09 19:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
testcase (1.83 KB, text/html)
2014-07-01 00:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-02 01:19:10 PST
We cannot know whether some modifier keys are pressed or not:

1. Windows key on Windows
2. Windows key which is mapped to Super key on Linux
3. Hyper key on Linux.

For them, we should implement KeyboardEvent.getModifierState().
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 18:31:18 PDT
Created attachment 616410 [details]
testcase
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:12:10 PDT
Created attachment 616428 [details] [diff] [review]
part.1 Move modifiers from nsMouseEvent_base to nsInputEvent
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:22:36 PDT
Created attachment 616432 [details] [diff] [review]
part.2 Replace nsInputEvent::isShift, nsInputEvent::isControl, nsInputEvent::isAlt and nsInputEvent::isMeta

This replaces is* with Is*() or just removes them. Not changing any logic.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:25:13 PDT
Created attachment 616436 [details] [diff] [review]
part.3 Improve nsDOMWindowUtils::send*Event() for new modifiers
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:29:12 PDT
Created attachment 616437 [details] [diff] [review]
part.4 Support new modifiers on all events derived from nsInputEvent on Windows
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:32:11 PDT
Created attachment 616438 [details] [diff] [review]
part.5 Support new modifiers on all events derived from nsInputEvent on GTK
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:35:09 PDT
Created attachment 616440 [details] [diff] [review]
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa

> -  nsDragEvent geckoEvent(true, aMessage, nsnull);
> -  [self convertGenericCocoaEvent:[NSApp currentEvent] toGeckoEvent:&geckoEvent];
> +  nsDragEvent geckoEvent(true, aMessage, mGeckoChild);
> +  nsCocoaUtils::InitInputEvent(geckoEvent, [NSApp currentEvent]);

I realized that this always fails to initialize modifiers. I think that we should store latest modifiers by other events and use it. But I think we should do it in another bug.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 20:37:50 PDT
Created attachment 616441 [details] [diff] [review]
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-18 21:23:02 PDT
Created attachment 616448 [details]
testcase

This works with IE9 too.
Comment 10 Jim Mathies [:jimm] 2012-04-19 08:31:17 PDT
Comment on attachment 616437 [details] [diff] [review]
part.4 Support new modifiers on all events derived from nsInputEvent on Windows

Functionality looks fine. r+ assuming this fits with the new spec.
Comment 11 Steven Michaud [:smichaud] (Retired) 2012-04-20 14:29:56 PDT
Comment on attachment 616440 [details] [diff] [review]
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa

This looks fine to me.

I do have one question, though.

+    // Remove basic modifiers from keypress event because if they are included,
+    // nsPlaintextEditor ignores the event.
+    keypressEvent.modifiers &= ~(widget::MODIFIER_CONTROL |
+                                 widget::MODIFIER_ALT |
+                                 widget::MODIFIER_META);

Wouldn't it make better sense to move this to just before the
following lines?

  // TODO:
  // If mCurrentKeyEvent.mKeyEvent is null and when we implement textInput
  // event of DOM3 Events, we should dispatch it instead of keypress event.
  bool keyPressHandled = DispatchEvent(keypressEvent);

'keypressEvent' will get initialized with modifiers (by
nsCocoaUtils::InitInputEvent()) whether or not 'currentKeyEvent' is
nil.  Currently you only remove them if if 'currentKeyEvent' is
non-nil.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-20 19:06:55 PDT
(In reply to Steven Michaud from comment #11)
> Comment on attachment 616440 [details] [diff] [review]
> part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa
> 
> This looks fine to me.
> 
> I do have one question, though.
> 
> +    // Remove basic modifiers from keypress event because if they are
> included,
> +    // nsPlaintextEditor ignores the event.
> +    keypressEvent.modifiers &= ~(widget::MODIFIER_CONTROL |
> +                                 widget::MODIFIER_ALT |
> +                                 widget::MODIFIER_META);
> 
> Wouldn't it make better sense to move this to just before the
> following lines?
> 
>   // TODO:
>   // If mCurrentKeyEvent.mKeyEvent is null and when we implement textInput
>   // event of DOM3 Events, we should dispatch it instead of keypress event.
>   bool keyPressHandled = DispatchEvent(keypressEvent);
> 
> 'keypressEvent' will get initialized with modifiers (by
> nsCocoaUtils::InitInputEvent()) whether or not 'currentKeyEvent' is
> nil.  Currently you only remove them if if 'currentKeyEvent' is
> non-nil.

Oh, you're right. Thank you, I'll update it.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-23 02:10:16 PDT
Created attachment 617422 [details] [diff] [review]
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa

updated for the review comment.
Comment 14 Olli Pettay [:smaug] (reviewing overload) 2012-04-23 12:33:08 PDT
Comment on attachment 616428 [details] [diff] [review]
part.1 Move modifiers from nsMouseEvent_base to nsInputEvent


> NS_IMETHODIMP
> nsDOMKeyboardEvent::GetAltKey(bool* aIsDown)
> {
>   NS_ENSURE_ARG_POINTER(aIsDown);
>-  *aIsDown = ((nsInputEvent*)mEvent)->isAlt;
>+  *aIsDown = ((nsInputEvent*)mEvent)->IsAlt();
While you're here, could you change the C style casting to
C++ static_cast



>+  widget::Modifiers modifiers = static_cast<nsInputEvent*>(mEvent)->modifiers;
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_SHIFT)) {
>-    return static_cast<nsInputEvent*>(mEvent)->isShift;
>+    return (modifiers & widget::MODIFIER_SHIFT) != 0;
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_CONTROL)) {
>-    return static_cast<nsInputEvent*>(mEvent)->isControl;
>+    return (modifiers & widget::MODIFIER_CONTROL) != 0;
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_META)) {
>-    return static_cast<nsInputEvent*>(mEvent)->isMeta;
>+    return (modifiers & widget::MODIFIER_META) != 0;
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_ALT)) {
>-    return static_cast<nsInputEvent*>(mEvent)->isAlt;
>+    return (modifiers & widget::MODIFIER_ALT) != 0;
>   }
> 
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_ALTGRAPH)) {
>     return (modifiers & widget::MODIFIER_ALTGRAPH) != 0;
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_WIN)) {
>     return (modifiers & widget::MODIFIER_WIN) != 0;
>   }
So why use modifiers & foo here, and not the helper methods, isAlt() etc. ?
I'd prefer using the methods. Perhaps there should be helpers for altgr and win modifiers too.
Comment 15 Olli Pettay [:smaug] (reviewing overload) 2012-04-23 12:58:18 PDT
Comment on attachment 616436 [details] [diff] [review]
part.3 Improve nsDOMWindowUtils::send*Event() for new modifiers

>   nsMouseEvent event(true, msg, widget, nsMouseEvent::eReal,
>                      contextMenuKey ?
>                        nsMouseEvent::eContextMenuKey : nsMouseEvent::eNormal);
>-  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
>-  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
>-  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
>-  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;
>+  event.modifiers =GetWidgetModifiers(aModifiers);
space after =
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-23 18:27:37 PDT
Thank you for the quick review, smaug!

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 616428 [details] [diff] [review]
> part.1 Move modifiers from nsMouseEvent_base to nsInputEvent

> So why use modifiers & foo here, and not the helper methods, isAlt() etc. ?
> I'd prefer using the methods. Perhaps there should be helpers for altgr and
> win modifiers too.

Did you mean only for AltGraph and Win? How about the others, i.e., CapsLock, NumLock, Scroll, Fn, and SymbolLock? Currently, we will support only CapsLock, NumLock and Scroll at dispatching native event. Fn and SymbolLock are supported only for untrusted events. I think that the former 3 might be used in the future.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-23 18:39:43 PDT
Created attachment 617731 [details] [diff] [review]
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent
Comment 18 Olli Pettay [:smaug] (reviewing overload) 2012-04-24 02:24:36 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 4/29 - 5/6) from comment #16)
> (In reply to Olli Pettay [:smaug] from comment #14)
> 
> Did you mean only for AltGraph and Win? How about the others, i.e.,
> CapsLock, NumLock, Scroll, Fn, and SymbolLock?
I mean all. Adding helper methods to get the value would be nice.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-25 17:42:00 PDT
https://developer.mozilla.org/en/DOM/KeyboardEvent#getModifierState%28%29
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-07-09 19:56:10 PDT
Created attachment 640488 [details]
testcase
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2014-07-01 00:24:27 PDT
Created attachment 8448561 [details]
testcase

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