Open Bug 1257617 Opened 9 years ago Updated 1 year ago

keypress event with ctrl modifier fires twice

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

47 Branch
x86_64
Windows 10
defect

Tracking

()

Tracking Status
e10s + ---

People

(Reporter: antonglv, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: btpp-fixlater triaged)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20160303134406 Steps to reproduce: 1. Open browser (with the "Enable multi-process Firefox Developer Edition" option enabled). "Firefox Developer Edition Start Page - e10s" was opened. 2. Open Scratchpad and run in the "Browser" environment code below: window. addEventListener ("keypress", e => console. log ("keypress"), true); 3. Switch to the browser window and press any ctrl-key combination which has binded command (where key is any alphabetic key except 'q'), for example, ctrl+a. Actual results: Two messages "keypress" will appear in the Browser console. Expected results: Only one message "keypress" should appear in the Browser console as it happens in the case if on the step 3 was pressed a key without ctrl modifier (or with other modifier).
Component: Untriaged → Event Handling
Product: Firefox → Core
Masayuki has been hacking the relevant code very recently. (IIRC one needs to enable chrome debugging in devtools to get the Browser environment in scratchpad.) We do need to do something unusual here when dispatching events to child process.
I don't reproduce this bug with both Nightly and Aurora... # And my changes are landed only for Nightly for now. Reporter, do you reproduce this bug with safe mode?
Flags: needinfo?(antonglv)
I can reproduce the bug only in Firefox 47.0a2, not in Firefox 45, not in Nightly. UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0 The bug is reproduced in safe mode. Two events fire, first with rangeParent=xul:stack and rangeOffset=1 and second with rangeParent=null and rangeOffset=0 > And could you check it with https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html ? The bug is not reproduced at https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html, only one keypress event is catched there. It seems it affected only chrome event handlers.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Clear the needinfo with previous post.
Flags: needinfo?(antonglv)
I hastened to say that the bug is not reproduced in Nightly (probably because it's easy to forget to set the "Browser" environment in the scratchpad). Now I see the bug is reproduced also in Nightly, both in normal and safe mode. UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0 (Firefox Nightly 48.0a1 (2016-03-17)
First, nsXBLWindowKeyHandler listens to keyboard events on XUL document, not window. https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/dom/xbl/nsXBLService.cpp#531,536-542,559 Therefore, window in capture phase receives keyboard events before nsXBLWindowKeyHandler. Next, if key combination is not reserved, nsXBLWindowKeyHandler stops propagation only in the system event group only when the key combination matches a shortcut key. https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/dom/xbl/nsXBLWindowKeyHandler.cpp#430,436-438,443-445,447-449,453,459 Therefore, if mWantReplyFromContentProcess is set to true, it seems that same keyboard events fired twice with following conditions: * matches with a shortcut key * but not reserved shortcut key * event target is in a remote process * in the default event group or window in capture phase
It seems in comment 7 you're saying this is a legitimate bug in e10s mode, Masayuki? Are you going to be able to work on this? Is this just affecting chrome pages?
Flags: needinfo?(masayuki)
Whiteboard: btpp-followup-2016-04-21
Yeah, after I finish to work on bug 1257759, I can do this. # See comment 2, I couldn't reproduce this bug... I'll make automated test first.
Flags: needinfo?(masayuki)
Whiteboard: btpp-followup-2016-04-21 → btpp-fixlater
Starting to work on this... After I confirmed the bug, I'll mark the status as ASSIGNED.
Assignee: nobody → masayuki
Hmm, key events in chrome is really broken...
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I should document what happens now: 1. If a keyboard event matches a non-reserved shortcut key registered to nsXBLWindowKeyHandler, its propagation is stopped in system group at capturing phase at the document node. 2. The event is sent to the content process as a default action of it. 3. Then, the event is back to the chrome process again. This event isn't distinguishable from Javascript whether a real keyboard event or a reply event and this reply event is fully propagated as a real event. I've been trying to fire a test keyboard event before dispatching real key events into the chrome DOM tree and if the event wants a reply event, stop dispatching into the chrome DOM tree. However, this causes shuffling event order in chrome, so, very bad... So, now, I give up to fix this bug with this approach. I'm thinking that we should not dispatch reply keyboard events as real keyboard event. So, I think that we should not stop propagation of real keyboard event in the system event group. Instead, nsXBLWindowKeyHandler should execute shortcut key handlers with reply keyboard events. Smaug, do you have better idea, if not, I should add reply keyboard events such as eReplyKeyDown, eResultKeyDown or something like that. Do you have better name for such events?
Flags: needinfo?(bugs)
I think that this bug is serious for some add-ons which listen to keypress events in chrome process. They receive two same keypress events (may be other events between the two keypress events). That may cause some add-ons not working in e10s mode.
tracking-e10s: --- → ?
clearing the NI to get this back into triage for Masayuki's request
Flags: needinfo?(bugs)
A bug in chrome that doesn't appear to break anything. However it may impact addons. Lets wait for smaug to get back to masayuki here.
Blocks: e10s-addons
Flags: needinfo?(bugs)
(In reply to Jim Mathies [:jimm] from comment #15) > A bug in chrome that doesn't appear to break anything. Exactly. I'm working on fixing long standing TSF bugs on Facebook and Google Docs (because the symptom becomes worse in e10s mode). I'll be back after that.
Hi Masayuki, do you know when you can start working on this issue? Do you have an idea how long it will take?
Flags: needinfo?(masayuki)
Priority: -- → P2
I've already restarted yesterday, but now I need to work on bug 1260651 today for unblocking other developers. Anyway, this is now top of my queue of writing patches.
Flags: needinfo?(masayuki)
Whiteboard: btpp-fixlater → btpp-fixlater triaged
Sorry for the delay, I'll be back here soon after finishing bug 1286464.
There are a lot of paths to dispatch keyboard events currently: 1. Dispatch keydown/keyup/keypress event in chrome first, then, dispatch it into child process from ESM::PostHandleEvent() 2. Dispatch keydown/keyup/keypress event in chrome first, then, nsXBLWindowKeyHandler prevents ESM to dispatch it into child process due to reserved shortcut. 3. Dispatch keydown/keyup/keypress event in chrome first, then, nsXBLWindowKeyHandler stops its propagation for waiting reply from child process. Then, ESM::PostHandleEvent() dispatches the event into the child process. Finally, TabParent receives the event from child process and dispatch it again (for start of the propagation) in chrome. (This is this bug) 4. Dispatch keypress event from widget, then, ESM::PreHandleEvent() posts the event into a child process if chrome has focus but there is an active content process (tab). Then, the keypress event is dispatched in chrome normally. After that, TabParent receives only "not handled" event from the child process, then, dispatch eAccessKeyNotFound event in chrome. #1 and #2 doesn't have problem. #3 is the simple case of this bug. My current approach is, we should dispatch test event before dispatching every keydown/keyup/keypress event from PresShell. And nsXBLWindowKeyHandler handles the test events and set mIsReserved or mWantsReplyFromContentProcess to true if it's necessary. Then, PresShell dispatches the keyboard events in the remote process first if mWantsReplyFromContentProcess is true. After receiving the event, TabParent will dispatch the keyboard event and call ESM::PostHandleEvent() manually. (The last part is ugly, though) This approach may cause odd keyboard event race only in chrome process. For example, if #3 case and #1 case occur in very short time or when the remote process is busy, the latter #1's keyboard event is handled by chrome first. Then, #3's keyboard is handled chrome, next. However, I don't think that we should use #3's new approach always because it may causes bad UI response. (Of course, we should do same thing that if we will have a lot of bug report caused by this issue, but it's really not in the scope of this bug.) Additionally, if we have a lot of work for #4, we should put off to fix it in another bug because it's rare case. Usually, shortcut keys shouldn't use same same modifier key combination with content access key's modifier key combination and using it when chrome has focus. But I'll try to fix this too.
Unfortunately, perhaps, I cannot fix this bug in 51 because I have a lot of urgent fix around native key event handling regressions and will take a vacation. Sorry for the delay to fix (Although, I'll try to fix in 51 as far as possible).
I'm back this bug again, I shouldn't go away except if we'd get bug reports which should be uplifted to release and/or beta branch.
Flags: needinfo?(bugs)
No longer blocks: 1273472
With Firefox 57 only WebExtensions are permitted and are, by default, e10s compatible.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
This is a bug for chrome of Firefox too.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
In recent weeks on windows I encounter examples of this with ctrl+W - multiple tabs sometimes are closed. I mostly use nightly
Component: Event Handling → User events and focus handling
Assignee: masayuki → nobody
Severity: normal → S3
Status: REOPENED → NEW
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.