The default bug view has changed. See this FAQ.

Implement DOM3 KeyboardEvent.getModifierState()

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug, {dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 5 obsolete attachments)

21.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
78.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.10 KB, patch
jimm
: review+
Details | Diff | Splinter Review
9.80 KB, patch
karlt
: review+
Details | Diff | Splinter Review
9.75 KB, patch
Details | Diff | Splinter Review
20.93 KB, patch
Details | Diff | Splinter Review
1.83 KB, text/html
Details
(Assignee)

Description

6 years ago
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().
(Assignee)

Updated

6 years ago
Depends on: 630817
(Assignee)

Updated

6 years ago
Depends on: 630813
(Assignee)

Updated

6 years ago
Blocks: 680829
(Assignee)

Comment 1

5 years ago
Created attachment 616410 [details]
testcase
(Assignee)

Updated

5 years ago
Depends on: 731878
(Assignee)

Comment 2

5 years ago
Created attachment 616428 [details] [diff] [review]
part.1 Move modifiers from nsMouseEvent_base to nsInputEvent
Attachment #616428 - Flags: review?(bugs)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #616432 - Flags: review?(bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 616436 [details] [diff] [review]
part.3 Improve nsDOMWindowUtils::send*Event() for new modifiers
Attachment #616436 - Flags: review?(bugs)
(Assignee)

Comment 5

5 years ago
Created attachment 616437 [details] [diff] [review]
part.4 Support new modifiers on all events derived from nsInputEvent on Windows
Attachment #616437 - Flags: review?(jmathies)
(Assignee)

Comment 6

5 years ago
Created attachment 616438 [details] [diff] [review]
part.5 Support new modifiers on all events derived from nsInputEvent on GTK
Attachment #616438 - Flags: review?(karlt)
(Assignee)

Comment 7

5 years ago
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.
Attachment #616440 - Flags: review?(smichaud)
(Assignee)

Comment 8

5 years ago
Created attachment 616441 [details] [diff] [review]
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent
Attachment #616441 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
Created attachment 616448 [details]
testcase

This works with IE9 too.
Attachment #616410 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #616448 - Attachment is patch: false
Attachment #616448 - Attachment mime type: text/plain → text/html

Comment 10

5 years ago
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.
Attachment #616437 - Flags: review?(jmathies) → review+

Updated

5 years ago
Blocks: 747013
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.
Attachment #616440 - Flags: review?(smichaud) → review+
(Assignee)

Comment 12

5 years ago
(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.
Attachment #616438 - Flags: review?(karlt) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 617422 [details] [diff] [review]
part.6 Support new modifiers on all events derived from nsInputEvent on Cocoa

updated for the review comment.
Attachment #616440 - Attachment is obsolete: true
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.
Attachment #616428 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #616432 - Flags: review?(bugs) → review+
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 =
Attachment #616436 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #616441 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 617731 [details] [diff] [review]
part.7 Add getModifierState() to DOM KeyboardEvent and DOM MouseEvent
Attachment #616441 - Attachment is obsolete: true
Attachment #617731 - Flags: superreview?(jst)

Updated

5 years ago
Attachment #617731 - Flags: superreview?(jst) → superreview+
(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.
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf567dc0cd5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3dfd5f66400
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4b48e5f7e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8112c0b9de
https://hg.mozilla.org/integration/mozilla-inbound/rev/575e562a8898
https://hg.mozilla.org/integration/mozilla-inbound/rev/f38b80d2743d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b032eb5936c1
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/bf567dc0cd5b
https://hg.mozilla.org/mozilla-central/rev/c3dfd5f66400
https://hg.mozilla.org/mozilla-central/rev/1d4b48e5f7e0
https://hg.mozilla.org/mozilla-central/rev/fe8112c0b9de
https://hg.mozilla.org/mozilla-central/rev/575e562a8898
https://hg.mozilla.org/mozilla-central/rev/f38b80d2743d
https://hg.mozilla.org/mozilla-central/rev/b032eb5936c1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
(Assignee)

Comment 21

5 years ago
https://developer.mozilla.org/en/DOM/KeyboardEvent#getModifierState%28%29
(Assignee)

Updated

5 years ago
Depends on: 749553
(Assignee)

Updated

5 years ago
Depends on: 749560
(Assignee)

Updated

5 years ago
Depends on: 753256
(Assignee)

Updated

5 years ago
Depends on: 768287
(Assignee)

Updated

5 years ago
Depends on: 769190
(Assignee)

Comment 22

5 years ago
Created attachment 640488 [details]
testcase
Attachment #616448 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 951021
(Assignee)

Updated

3 years ago
Depends on: 951023
(Assignee)

Updated

3 years ago
Depends on: 1009388
(Assignee)

Updated

3 years ago
Depends on: 1023067
(Assignee)

Comment 23

3 years ago
Created attachment 8448561 [details]
testcase
Attachment #640488 - Attachment is obsolete: true
Documentation:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.getModifierState
and
https://developer.mozilla.org/en-US/Firefox/Releases/15#DOM
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

a year ago
Blocks: 1249872
You need to log in before you can comment on or make changes to this bug.