Closed
Bug 1009388
Opened 9 years ago
Closed 9 years ago
Support DOM3 Events' virtual modifier "Accel" for getModifierState()
Categories
(Core :: DOM: Events, enhancement)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: masayuki, Assigned: masayuki)
References
()
Details
Attachments
(4 files, 2 obsolete files)
3.96 KB,
patch
|
smaug
:
review+
surkov
:
review+
|
Details | Diff | Splinter Review |
8.38 KB,
patch
|
smaug
:
review+
enndeakin
:
review+
|
Details | Diff | Splinter Review |
11.18 KB,
patch
|
Details | Diff | Splinter Review | |
4.75 KB,
patch
|
Details | Diff | Splinter Review |
getModifierState("Accel") returns true when a modifier key for shortcut key is active. This is useful for our chrome implementation.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
This must be useful for our internal code too!
Attachment #8423766 -
Flags: review?(bugs)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Same. Does this need additional review, e.g., by bz?
Attachment #8423769 -
Flags: review?(bugs)
Assignee | ||
Comment 4•9 years ago
|
||
Same. Perhaps, Enn or (another) Neil's review is needed?
Attachment #8423772 -
Flags: review?(bugs)
Comment 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8423768 -
Flags: review?(bugs) → review+
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8423766 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8423769 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8423772 -
Flags: review?(enndeakin) → review+
Updated•9 years ago
|
Attachment #8423768 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
I wrote a document in MDN. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.getModifierState#.22Accel.22_virtual_modifier
Comment 14•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•