Rewrite NSFlagsChanged handler

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla19
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 disabled)

Details

Attachments

(1 attachment, 1 obsolete attachment)

When I research bug 784783, I realized our handling is wrong approach.

Now, we're dispatching key events only when modifierFlags is changed. This causes not dispatching some modifier key events. E.g., while you're pressing left Shift key, pressing right Shift key doesn't cause DOM key events.

And also, we're dispatching one or *more* key events for every NSFlagsChanged event with a native event. It means that we're dispatching two or more key events with same keycode for a NSFlagsChanged event.
Comment on attachment 668871 [details] [diff] [review]
Patch

This patch make the handler support:

1. dispatching shift, control, option and meta key event while the other same modifier key is pressed (e.g., right shift key event is dispatched even while left shift key is pressed)
2. dispatching help key event and fn key event.
3. computes DOM keyCode from the changed flags if keyCode of native key event isn't available.

First of all, we're dispatching modifier key events from the difference of modifierFlags of native NSFlagsChanged event. Therefore, if the other key which activates same device-independent flag is already pressed, we don't dispatch the additional modifier key event.

For solving this bug, we should use keyCode value of the native events. Then, we can dispatch key events easily in most cases.

However, if an event is fired after our window is deactivated by command + (shift +) tab, the kyCode is always 0 and but the new modifierFlags is noticed (I'm not sure if it's the only case of the weird keyCode value). This patch handles this case in the "default" label. In this case, we need to decide the DOM keyCode from changed modifierFlags. The loop scans device-dependent flags first since it's the only hint of the released key location (i.e., left or right). For this handling, when modifier key is actually pressed, we need to record the relation of keyCode and device-dependent flag. After that, scans device-independent flags for other modifier keys which don't have device-dependent flags.

Additionally, this patch adds DOM Function keyCode. It's a modifier key which is defined in D3E. Therefore, I think that it's worthwhile to add new DOM keyCode even though it's deprecated. If we don't add a new keyCode for it, I need additional work for preventing spam warnings.
Attachment #668871 - Flags: superreview?(bugs)
Attachment #668871 - Flags: review?(smichaud)
Comment on attachment 668871 [details] [diff] [review]
Patch

Actually, do we really need the new const? What is the use case?
Do other browsers report that value?
Or could we make it [noscript] ? (I'm not sure if const + [noscript] is supported)
Attachment #668871 - Flags: superreview?(bugs) → superreview-
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 668871 [details] [diff] [review]
> Patch
> 
> Actually, do we really need the new const? What is the use case?

If we can add it, I can make the widget part change smaller. I'm not sure the use case. Fn key is rarely used.

> Do other browsers report that value?

No, other browsers, all Safari, Chrome and Opera don't dispatch key events for Fn key.

> Or could we make it [noscript] ? (I'm not sure if const + [noscript] is
> supported)

Not supported.

If we don't add new keyCode for Fn key, I think there are following options:

1. Dispatch Fn key event with keyCode 0.
2. Never dispatch Fn key event.
3. Not dispatch Fn key event for now, but would support when we add .key/.char support. Then, keyCode 0 doesn't have problem.

I recommend #3. How about you, smaug?
3# sounds ok. It means we should not add the const.
Posted patch PatchSplinter Review
This patch removes the Fn key handling.
Attachment #668871 - Attachment is obsolete: true
Attachment #671400 - Flags: review?(smichaud)
Comment on attachment 671400 [details] [diff] [review]
Patch

This looks fine to me (though I haven't tested it, and am not entirely sure I understand all of it).

But some of the comments need to be rewritten.  (Please let me know if any of my suggested replacement comments are incorrect.)

+  // Stores device dependent modifier flags with a modifier keyCode which
+  // causes changing the flag.
+  struct ModifierKey

"Stores the association of device dependent modifier flags with a modifier keyCode.  Being device dependent, this association may differ from one kind of hardware to the next."

+    case kVK_Help: {
+      // We don't assume that two or modifiers are not changed once when the
+      // keycode has a specific modifier keycode.
+      bool isKeyDown = ([aNativeEvent modifierFlags] & diff) != 0;

"We assume that at most one modifier is changed per event if the event is caused by pressing or releasing a modifier key."

+    // If the event is caused by other reasons, e.g., deactivated by
+    // command-tab, dispatch keyup events for the deactivated modifiers
+    default: {

"If the event is caused by something else than pressing or releasing a single modifier key (for example by the app having been activated/deactivated using command-tab), use the modifiers themselves to determine which key's event to dispatch, and whether it's a keyup or keydown event."

+            default:
+              // Other modifiers may be activated by two keys such as left key
+              // and right key.  Such keys must activates device dependent flags
+              // which were already handled.  So, nothing to do for them here.
+              continue;

I'm not sure I understand this comment, but I think it means something like:

"Some events are caused by two keys (such as the left shift key and the right shift key) being down at the same time.  But we've already processed events for each of these keys as it was pressed individually.  So there's nothing more to do here."
Attachment #671400 - Flags: review?(smichaud) → review+
Comment on attachment 671400 [details] [diff] [review]
Patch

One more thing:

+  // kCGEventFlagMaskNonCoalesced (= NX_NONCOALSESCEDMASK): See the document for
+  // Quarts Event Services.

It's "Quartz Event Services".
Thank you for your review!

(In reply to Steven Michaud from comment #7)
> +    // If the event is caused by other reasons, e.g., deactivated by
> +    // command-tab, dispatch keyup events for the deactivated modifiers
> +    default: {
> 
> "If the event is caused by something else than pressing or releasing a
> single modifier key (for example by the app having been
> activated/deactivated using command-tab), use the modifiers themselves to
> determine which key's event to dispatch, and whether it's a keyup or keydown
> event."

I don't see any key events when activating window by Cmd-Tab, so, I'll remove "activated/" from your text.

> +            default:
> +              // Other modifiers may be activated by two keys such as left
> key
> +              // and right key.  Such keys must activates device dependent
> flags
> +              // which were already handled.  So, nothing to do for them
> here.
> +              continue;
> 
> I'm not sure I understand this comment, but I think it means something like:
> 
> "Some events are caused by two keys (such as the left shift key and the
> right shift key) being down at the same time.  But we've already processed
> events for each of these keys as it was pressed individually.  So there's
> nothing more to do here."

Hmm, it's not. I tried meaning that modifier keys which can be distinguished left key or right key must have device dependent flag for the way to distinguish the location. So, if the event is NOT a generated event by another application like a11y tools, NSFlagsChanged event's modifierFlags must have device dependent flags with NSShiftKeyMask, NSCommandKeyMask, NSControlKeyMask or NSAlternateKeyMask. So, these modifiers should be handled by the "else" path before it comes into the "default" case. In other words, this case may be possible case. But we cannot decide the DOM key location for the modifier key event. I think that the key events in such illegal case shouldn't be important because of too rare case. Therefore, this patch ignores the case.
Thanks for your response.  After reading it and your patch several
more times, I understand better what it's all about.  But now I have a
question about the patch itself:

+        // Remove flags
+        modifiers &= ~flag;

Your patch assumes that all NSFlagsChanged events not triggered by
pressing or releasing a modifier key "turn off" one or more modifier
flags.

I imagine your tests showed this.  And if (say) an external app (like
an a11y app) generated an NSFlagsChanged event to turn on a flag, it
would presumably set its keyCode to the appropriate modifier key.

But are you sure this is right?

At the very least we need another comment explaining why :-)
(Following up comment #10)

Oops, I'm still misreading your patch.

Disregard comment #10.
(Following up comment #11)

No, it's comment #11 that should be disregarded.

Sigh :-(
Yeah, it's very complicated, don't mind.

As you pointed, this patch doesn't assume that a11y like external app generates NSFlagsChanged events without individual keyCode and activating one (or more) flags. If we do support such case, we need to do:

1. |bool dispatchKeyDown = false;| should be |bool dispatchKeyDown = (modifiers & flag) != 0;|

2. Needs to implement the "default" case which we need to decide the new comment.

3. Don't remove the flag if dispatchKeyDown is true.

However, I don't think that such strange event is handled properly on other applications, so that this patch doesn't think about such case. But even if we ignore such case, we need to skip first if |dispatchKeyDown| becomes true except the lockable modifiers, though. Do you think that we should support the case?
No, I don't think you need to change your patch.  We don't need	to
bend over backwards to handle weird/malformed NSFlagsChanged events.

I *think* I now	understand your	patch well enough to revise some of my
replacement comments,	as follows:

+    // If the event is caused by other reasons, e.g., deactivated by
+    // command-tab, dispatch keyup events for the deactivated modifiers
+    default: {

"If the event is caused by something else than pressing or releasing a
single modifier key (for example by the app having been activated
using command-tab), use the modifiers themselves to determine which
key's event to dispatch, and whether it's a keyup or keydown event.
In all cases we assume one or more modifiers are being deactivated
(never activated) -- otherwise we'd have received one or more events
corresponding to a single modifier key being pressed."

+          // Must not be the flag being activated in this situation.

"Any modifier change here must be a deactivation."

+            default:
+              // Other modifiers may be activated by two keys such as left key
+              // and right key.  Such keys must activates device dependent flags
+              // which were already handled.  So, nothing to do for them here.
+              continue;

"The other cases (NSShiftKeyMask, NSControlKeyMask, NSAlternateKeyMask
and NSCommandKeyMask) are handled by the other branch of the if
statement, below (which handles device dependent flags)."
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73ea50909dc

Thank you. According to the new comments, I think that you understand the change well.
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/e73ea50909dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla20 → mozilla19
You need to log in before you can comment on or make changes to this bug.