Closed Bug 1009388 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: masayuki, Assigned: masayuki)

References

()

Details

Attachments

(4 files, 2 obsolete files)

getModifierState("Accel") returns true when a modifier key for shortcut key is active. This is useful for our chrome implementation.
This must be useful for our internal code too!
Attachment #8423766 - Flags: review?(bugs)
Other modules which access "ui.key.accelKey" should share WidgetInputEvent::AccelModifier() for consistency behavior.

I'll request review to a11y peer later.
Attachment #8423768 - Flags: review?(bugs)
Same.

Does this need additional review, e.g., by bz?
Attachment #8423769 - Flags: review?(bugs)
Same.

Perhaps, Enn or (another) Neil's review is needed?
Attachment #8423772 - Flags: review?(bugs)
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
Attachment #8423766 - Flags: review?(bugs) → review+
Attachment #8423768 - Flags: review?(bugs) → review+
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.
Attachment #8423769 - Flags: review?(bugs) → review+
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.
Attachment #8423772 - Flags: review?(bugs) → review+
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()).
Attachment #8423768 - Flags: review?(surkov.alexander)
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.
Attachment #8423772 - Flags: review?(enndeakin)
Attachment #8423772 - Flags: review?(enndeakin) → review+
Attachment #8423768 - Flags: review?(surkov.alexander) → review+
You need to log in before you can comment on or make changes to this bug.