1009388 - Support DOM3 Events' virtual modifier "Accel" for getModifierState()

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

getModifierState("Accel") returns true when a modifier key for shortcut key is active. This is useful for our chrome implementation.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Support getModifierState("Accel")

This must be useful for our internal code too!

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Accessible should use WidgetEvent::AccelModifier() for consistency with other modules

Other modules which access "ui.key.accelKey" should share WidgetInputEvent::AccelModifier() for consistency behavior.

I'll request review to a11y peer later.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 nsXBLPrototypeHandler should use WidgetEvent::AccelModifier() for consistency with other modules

Same.

Does this need additional review, e.g., by bz?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 nsMenuFrame should use WidgetEvent::AccelModifier() for consistency with other modules

Same.

Perhaps, Enn or (another) Neil's review is needed?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8423766 [details] [diff] [review]
part.1 Support getModifierState("Accel")

>+mouseEvent = new MouseEvent("mousedown", { metaKey: true });
>+is(mouseEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "MouseEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
>+mouseEvent = new MouseEvent("mousedown", { ctrlKey: true, altKey: true, metaKey: true });
>+is(mouseEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_CONTROL ||
>+                                        kAccelKeyCode == KeyboardEvent.DOM_VK_ALT ||
>+                                        kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "MouseEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
perhaps better to change the message here, since it is the same as in the previous test

>+wheelEvent = new WheelEvent("wheel", { metaKey: true });
>+is(wheelEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "WheelEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
>+wheelEvent = new WheelEvent("wheel", { ctrlKey: true, altKey: true, metaKey: true });
>+is(wheelEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_CONTROL ||
>+                                        kAccelKeyCode == KeyboardEvent.DOM_VK_ALT ||
>+                                        kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "WheelEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
Ditto
>+keyboardEvent = new KeyboardEvent("keydown", { metaKey: true });
>+is(keyboardEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "KeyboardEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
>+keyboardEvent = new KeyboardEvent("keydown", { ctrlKey: true, altKey: true, metaKey: true });
>+is(keyboardEvent.getModifierState(kAccel), kAccelKeyCode == KeyboardEvent.DOM_VK_CONTROL ||
>+                                           kAccelKeyCode == KeyboardEvent.DOM_VK_ALT ||
>+                                           kAccelKeyCode == KeyboardEvent.DOM_VK_META,
>+   "KeyboardEvent.getModifierState(\"" + kAccel + "\") should be true if metaKey is an accel modifier");
ditto

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8423769 [details] [diff] [review]
part.3 nsXBLPrototypeHandler should use WidgetEvent::AccelModifier() for consistency with other modules

>+// static
>+int32_t
>+nsXBLPrototypeHandler::AccelKeyMask()
>+{
>+  switch (WidgetInputEvent::AccelModifier()) {
>+    case MODIFIER_ALT:
>+      return cAlt | cAltMask;
>+    case MODIFIER_CONTROL:
>+      return cControl | cControlMask;
>+    case MODIFIER_META:
>+      return cMeta | cMetaMask;
>+    case MODIFIER_OS:
>+      return cOS | cOSMask;
>+    default:
>+      MOZ_CRASH("Handle the new result of WidgetInputEvent::AccelModifier()");
>+      return 0;
>+  }

I would write this
  switch (WidgetInputEvent::AccelModifier()) {
    case MODIFIER_ALT:
      return KeyToMask(nsIDOMKeyEvent::DOM_VK_META);
    case ....

That way we don't need the foo | fooMask stuff in two places.

Doesn't need another review.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8423772 [details] [diff] [review]
part.4 nsMenuFrame should use WidgetEvent::AccelModifier() for consistency with other modules

The changes to nsMenuBarListener aren't really related to this bug, but
they look ok.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8423768 [details] [diff] [review]
part.2 Accessible should use WidgetEvent::AccelModifier() for consistency with other modules

I've not tested this yet because I'm not sure how to test this.

If you think I should confirm actual behavior since this is not covered by automated tests, let me know how to test this.

But I believe this is enough simple migration for the new API (WidgetInputEvent::AccelModifier()).

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8423772 [details] [diff] [review]
part.4 nsMenuFrame should use WidgetEvent::AccelModifier() for consistency with other modules

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8423772 [details] [diff] [review]
> part.4 nsMenuFrame should use WidgetEvent::AccelModifier() for consistency
> with other modules
> 
> The changes to nsMenuBarListener aren't really related to this bug, but
> they look ok.

The reason why I changed it is, nsMenuBarListener.cpp defines MODIFIER_* with macro which are same name with mozilla::MODIFIER_* but their values are different. Therefore, unified cpp file causes using the macro value in nsMenuFrame.cpp.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Support getModifierState("Accel") (r=smaug)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 nsXBLPrototypeHandler should use WidgetEvent::AccelModifier() for consistency with other modules (r=smaug)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c98d18c4e751
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b7b325362e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb1e6e4f9ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/f91fc1ddca79

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I wrote a document in MDN.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.getModifierState#.22Accel.22_virtual_modifier

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c98d18c4e751
https://hg.mozilla.org/mozilla-central/rev/c1b7b325362e
https://hg.mozilla.org/mozilla-central/rev/ccb1e6e4f9ee
https://hg.mozilla.org/mozilla-central/rev/f91fc1ddca79