Closed Bug 1333459 Opened 8 years ago Closed 8 years ago

[e10s] preventDefault doesn't prevent standard browser accesskeys anymore

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: carsten.kroeger, Assigned: masayuki)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(6 files)

Bug 1333459 - part1: Move methods of EventStateManager which check modifiers of access key to WidgetKeyboardEvent
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1333459 - part3: Add automated tests into browser_accesskeys.js
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1333459 - part2-1: EventStateManager should have a way to check if there is accesskey which is executed by a specific keyboard event
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1333459 - part2-3: Make nsMenuBarListener::KeyPress() wait reply from remote process if the eKeyPress event will be sent to a remote process later
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1333459 - part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process
59 bytes, text/x-review-board-request
smaug
: review+
Details
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36 Steps to reproduce: Use the following sample --- <!DOCTYPE html> <html> <body> <button type="button" onclick="myFunction()">Click Me!</button> <script> function eventHandle(e) { if (e.altKey && e.keyCode > 18){ e.preventDefault(); } } function myFunction() { document.body.addEventListener('keydown',eventHandle,true); }; </script> </body> </html> --- Step 1: Press ALT+D (in german browser for the file menu shortcut; I think ALT+F in english localisation) Step 2: Press the button on the page to active the on keyDown handler. Step 3: Press ALT+D (in german browser for the file menu shortcut; I think ALT+F in english localisation) Actual results: After Step 1: The file menu of Firefox opens After Step 3: The file menu of Firefox opens Expected results: After Step 1: The file menu opens After Step 3: Nothing should happen because of the preventDefault. Up to version 47 it was possible to suppress the standard implementation of this shortcuts. We build a large desktop application like web site and we provide many shortcuts to our customers. Since version 49 we are in trouble because some of our important shortcuts not working on some machines. Our website is localisable, the shortcuts are related to the selected language also the firefox shortcuts are connected to the browser language so actually many of the formally working shortcuts not available anymore because preventDefault not working as before. Thanks for your help in advance
Masayuki, do know if this was intentional? Maybe e10s-related?
Flags: needinfo?(masayuki)
yep, it's actually a bug. I'm not sure if this is dup of older report.
Flags: needinfo?(masayuki)
After further investigations I found the reason of the different behaviors on different machines. For me it looks like it has something to do with the Electrolysis/Accessibility (https://wiki.mozilla.org/Electrolysis/Accessibility) On machines where the error did not occur older profiles with the following entries in prefs.js exists: user_pref("accessibility.lastLoadDate", 1485337595); user_pref("accessibility.loadedInLastSession", true); user_pref("accessibility.typeaheadfind.flashBar", 0); When I add this rows in the profiles where the bug occur the bug is solved. So I think the new (?) accessibility functionality is the reason for this bug.
Summary: preventDefault dosen't prevent standard browser shortcuts anymore → preventDefault dosen't prevent standard browser accesskeys anymore
When you have accessibility on or off does it change the state of electrolysis? Look in about:support for "Multiprocess Windows" and see if it's different. I'm asking because accessibility being on can sometimes trigger e10s being on or off.
Flags: needinfo?(carsten.kroeger)
Yes your right. about:support shows on "working" machines: Fenster mit mehreren Prozessen 0/1 (deaktiviert durch Werkzeuge fĂĽr Barrierefreiheit) What means Multiprocess Windows 0/1 (deactivated through tools for accessibility) or something like that.
Flags: needinfo?(carsten.kroeger)
It looks like nsMenuBarListener::KeyDown doesn't see the defaultPrevented flag as false.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 380637
Neil, are you able (and/or the best person) to take this?
Flags: needinfo?(enndeakin)
Keywords: regression
Priority: -- → P2
Yes I have a working on a fix.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Neil, Thank you very much for you speedy response and all your efforts with regard to key-handling. This is a very important aspect of our current project.
Currently, we have the bizarre setup in multi-process mode where a key event is fully dispatched to a parent process including the system phase and code which handles default behaviour, but we don't know at this point if content is going to handle it. Then we send events to the content process. Then we can optionally send the event entirely again to the parent process, where we can check if content cancelled the event and do default handling. But we have to know this the first time we sent the event and not do the default handling then. If there is no child process each consumer has to find this out and do something different. This is the case here with menus, as was the case with bug 1168042. So actually i don't think I am going to fix this.
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
Thank you for your investigation. This bug must be a bug of current KeyboardEvent propagation model in e10s mode. I'm going to add "test" key* events for checking if each key event should be fired in remote process first. I think that we can fix this after that.
Depends on: 1257617
See Also: → 1335412
Dear Mozillans, Is anyone working on this bug? It broke my website (www.typeit.org) - me and my users would appreciate a quick resolution. Thanks!
(In reply to Tomasz P. Szynalski from comment #12) > Dear Mozillans, > Is anyone working on this bug? It broke my website (www.typeit.org) - me and > my users would appreciate a quick resolution. Thanks! I'm sorry for the delay. I'd like to fix it as soon as possible, but current our highest priority is improving performance. So, it's difficult to fix a little bit later. (Perhaps, I should fix the preparation patches separately for avoiding to conflict with other patches, though.)
> it's difficult to fix a little bit later. I meant, it's difficult to fix it soon, therefore, it's a little bit later.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13) Thanks for the answer, Masayuki. I hope you'll find the time to fix this. Right now I have no other choice but to tell my users to use Chrome or Safari...
(In reply to Tomasz P. Szynalski from comment #15) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13) > > Thanks for the answer, Masayuki. I hope you'll find the time to fix this. > Right now I have no other choice but to tell my users to use Chrome or > Safari... Or, turn off the multi process mode from Options.
Ah, only this case might be able to be fixed easier than the root cause. I'll check this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Blocks: 1335412
Depends on: 1377653
No longer depends on: 1257617
OS: Unspecified → All
Hardware: Unspecified → All
See Also: 1335412 → 1257617
Summary: preventDefault dosen't prevent standard browser accesskeys anymore → [e10s] preventDefault dosen't prevent standard browser accesskeys anymore
huh, these patches are for some reason hard to review.
Comment on attachment 8884491 [details] Bug 1333459 - part3: Add automated tests into browser_accesskeys.js https://reviewboard.mozilla.org/r/155232/#review160870
Attachment #8884491 - Flags: review?(bugs) → review+
Comment on attachment 8884489 [details] Bug 1333459 - part1: Move methods of EventStateManager which check modifiers of access key to WidgetKeyboardEvent https://reviewboard.mozilla.org/r/155228/#review160878 Hopefully I didn't miss anything. Hard-to-review code move patch.
Attachment #8884489 - Flags: review?(bugs) → review+
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review160892 Could you explain the setup a bit more. I don't yet understand at all what is supposed to happen and what happens. I'm especially surprised with all the "dispatch to multiple tabs" behavior. (I guess partially because I wasn't involved with bug 1168042) ::: dom/events/EventStateManager.cpp:773 (Diff revision 1) > + // result in the remote process. > + if (IsRemoteTarget(GetFocusedContent())) { > + // Shouldn't stop propagation in this process here because we need to > + // check if the key combination is reserved by nsXBLWindowKeyHandler. > + // (In such case, we shouldn't fire the event on remote process.) > + keyEvent->MarkAsWaitingReplyFromRemoteProcess(); Why do we call MarkAsWaitingReplyFromRemoteProcess here? ::: dom/events/EventStateManager.cpp:1209 (Diff revision 1) > - // A remote child process is focused. The key event should get sent to > - // the child process. > + // processes. > + else if (!aEvent->IsHandledInRemoteProcess()) { > - aEvent->mAccessKeyForwardedToChild = true; > - } else { > AccessKeyInfo accessKeyInfo(aEvent, aAccessCharCodes); > nsContentUtils::CallOnAllRemoteChildren(mDocument->GetWindow(), Why do we have this kind of code. Looks bizarre. Why we send event to all the tabs?
Attachment #8884490 - Flags: review?(bugs)
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review160892 Even when a remote process has focus, currently, accesskeys registered to ESM are handled in PreHandleEvent() in the main process. Similarly, keyboard events are dispatched in the main process's DOM tree first and then, nsMenuBarListenr handles accesskeys in the menubar without checking the result of the remote process. So, like shortcut key handling in nsXBLWindowKeyHandler, when modifier keys of eKeyPress event matches either content or chrome accesskey and a remote process has focus, the main process should wait the reply from the remote process. Sending keyboard event to all remote children is really confused to me too when I writing these patches. Although I was involved. Even when chrome has focus, e.g., searchbar has focus, content accesskey should work (although I don't know if this is reasonable behavior), e.g., when there is <input accesskey="f"> in foreground tab, Alt+Shift+F should cause moving focus to the <input> element. Perhaps, there is only one remote process actually since background tabs are ignored and Firefox doesn't put put remote contents like tiles. > Why do we call MarkAsWaitingReplyFromRemoteProcess here? So, if a remote process has focus, eKeyPress event should be handled by the remote process first. Then, if preceding eKeyDown event is consumed, TabChild won't return reply eKeyPress event. Therefore, if preceding eKeyDown event is consumed, accesskey won't work in the main process if it's marked as waiting reply. I'll update the commit message and comment here. > Why do we have this kind of code. Looks bizarre. Why we send event to all the tabs? See above comment. I think that this is actually working with only one remote process. (Anyway, this is out scope of this bug and if we need to discuss more, we should wait Enn to back.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27) > > Why do we call MarkAsWaitingReplyFromRemoteProcess here? > > So, if a remote process has focus, eKeyPress event should be handled by the > remote process first. But MarkAsWaitingReplyFromRemoteProcess doesn't mean that. And since we call MarkAsWaitingReplyFromRemoteProcess in ESM::PreHandleEvent and forwarding to child process happens in ::PostHandleEvent, we end up handling the event first in parent process. > > Then, if preceding eKeyDown event is consumed, TabChild won't return reply > eKeyPress event. Therefore, if preceding eKeyDown event is consumed, > accesskey won't work in the main process if it's marked as waiting reply. Where is this keydown handling processed? I still don't understand the setup.
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review161202 Should EventStateManager call StopPropagation or something when MarkAsWaitingReplyFromRemoteProcess() is called and then not add the hack to nsMenuBarListener::KeyPress. ::: layout/xul/nsMenuBarListener.cpp:264 (Diff revision 2) > - if (aKeyEvent) { > + return NS_OK; > - aKeyEvent->GetIsTrusted(&trustedEvent); > } > > - if (!trustedEvent) { > + // If it needs to wait reply from remote process, event should do nothing. > + if (nativeKeyEvent->IsWaitingReplyFromRemoteProcess()) { Ahaa, here. This is really misusing IsWaitingReplyFromRemoteProcess(). This one particular event listener checks whether we want to send the event to child process, but otherwise we handle the event normally in chrome process
Attachment #8884490 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #31) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27) > > > > Why do we call MarkAsWaitingReplyFromRemoteProcess here? > > > > So, if a remote process has focus, eKeyPress event should be handled by the > > remote process first. > But MarkAsWaitingReplyFromRemoteProcess doesn't mean that. Yes, but that means that reply event will be dispatched in the main process if preceding eKeyDown event wasn't consumed. The purpose of this patch is, putting off to handle access key until receiving reply eKeyPress event when a remote process has focus. > And since we call MarkAsWaitingReplyFromRemoteProcess in ESM::PreHandleEvent > and forwarding to child process happens in ::PostHandleEvent, we end up > handling the event first in parent process. Yes. But it's necessary because the key combination may be reserved. If it's reserved, we shouldn't fire the keyboard event in content. However, if we stop dispatching the event in the main process in PresHandleEvent, nsXBLWindowKeyHandler won't/cannot mark the event as reserved. # If we can make nsXBLWindowKeyHandler not use event listeners, we can make this simpler, but I have no idea how to do that. Should it register to root ESM instead of using keyboard event handlers? > > Then, if preceding eKeyDown event is consumed, TabChild won't return reply > > eKeyPress event. Therefore, if preceding eKeyDown event is consumed, > > accesskey won't work in the main process if it's marked as waiting reply. > Where is this keydown handling processed? If preceding eKeyDown event is consumed, TabChild::mIgnoreKeyPressEvent is set to true. Then, TabChild will ignore following eKeyPress event (neither to dispatch into the DOM tree in the remote process nor to reply to the main process). I.e., only when eKeyPress reply event is back, preceding eKeyDown event wasn't consumed.
(In reply to Olli Pettay [:smaug] from comment #32) > Comment on attachment 8884490 [details] > Bug 1333459 - part2: EventStateManager should check if it needs to wait > reply from remote content before handling access keys > > https://reviewboard.mozilla.org/r/155230/#review161202 > > Should EventStateManager call StopPropagation or something when > MarkAsWaitingReplyFromRemoteProcess() is called and then not add the hack to > nsMenuBarListener::KeyPress. > > ::: layout/xul/nsMenuBarListener.cpp:264 > (Diff revision 2) > > - if (aKeyEvent) { > > + return NS_OK; > > - aKeyEvent->GetIsTrusted(&trustedEvent); > > } > > > > - if (!trustedEvent) { > > + // If it needs to wait reply from remote process, event should do nothing. > > + if (nativeKeyEvent->IsWaitingReplyFromRemoteProcess()) { > > Ahaa, here. This is really misusing IsWaitingReplyFromRemoteProcess(). This > one particular event listener checks whether we want to send the event to > child process, but otherwise we handle the event normally in chrome process Yes, but how do you think about the reserved key combination issue? I think that it's never problem with Firefox. But I'm not sure with addons.
Flags: needinfo?(bugs)
So you don't want to call StopPropagation so that XBL's reserved key handlers get called, but you do want to prevent MenuBarListener to handle the event?
Flags: needinfo?(bugs)
Could nsMenuBarListener check the target of the event and do similar thing what XBL listeneners do and call MarkAsWaitingReplyFromRemoteProcess() if needed?
(In reply to Olli Pettay [:smaug] from comment #35) > So you don't want to call StopPropagation so that XBL's reserved key > handlers get called, but you do want to prevent MenuBarListener to handle > the event? Ah, do you mean that if access key hits first (than reserved shortcut key), the access key should always win? And also should the keyboard event be fired on content normally? However, this patch does NOT check if the key combination actually matches with an access key. Should I do it? (In reply to Olli Pettay [:smaug] from comment #36) > Could nsMenuBarListener check the target of the event and do similar thing > what XBL listeneners do and call MarkAsWaitingReplyFromRemoteProcess() if > needed? Yeah, it's almost done by ESM::PrehandleEvent() instead since the access key modifiers are same (except Shift key, I forgot this though). So, let's sort out the things. If we should stop propagation in the main process, ESM should check whether coming key combination will actually hit an access key. Then, it will always win to any shortcut keys including reserved ones (but perhaps, this is okay) and fired in content. Finally, nsMenuBarListener should do same thing in KeyPress()?
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #37) > (In reply to Olli Pettay [:smaug] from comment #35) > > So you don't want to call StopPropagation so that XBL's reserved key > > handlers get called, but you do want to prevent MenuBarListener to handle > > the event? > > Ah, do you mean that if access key hits first (than reserved shortcut key), > the access key should always win? And also should the keyboard event be > fired on content normally? I don't mean really anything. Just trying to understand the patch. > So, let's sort out the things. If we should stop propagation in the main > process, ESM should check whether coming key combination will actually hit > an access key. Then, it will always win to any shortcut keys including > reserved ones (but perhaps, this is okay) and fired in content. Finally, > nsMenuBarListener should do same thing in KeyPress()? Maybe something like that yes.
Flags: needinfo?(bugs)
Summary: [e10s] preventDefault dosen't prevent standard browser accesskeys anymore → [e10s] preventDefault doesn't prevent standard browser accesskeys anymore
Okay, thanks. I'll rewrite the patch with new approach.
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review163748 ::: commit-message-6e43a:5 (Diff revision 3) > +Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys r?smaug > + > +Currently, access key is handled in EventStateManager::PreHandleEvent() with eKeyPress event, i.e., before dispatching it into the DOM tree, if the access key is registered in EventStateManager. So, the main process does not check if the preceding eKeyDown event is consumed in focused remote process. > + > +When preceding eKeyDown event is consumed in the main process, eKeyPress event won't be dispatched by widget. However, if remote process has focus, it's impossible. Therefore, main process needs to post eKeyPress event to check if preceding eKeyDown event was consumed. When eKeyPress event is marked as "waiting reply from remote process", TabChild sends it back to the main process only when preceding eKeyDown event wasn't consumed. So, only when eKeyPress event is back to the main process, main process should handle accesskey with it. What is impossible? ::: dom/events/EventStateManager.cpp:1251 (Diff revision 3) > - // A remote child process is focused. The key event should get sent to > - // the child process. > + // processes. > + else if (!aEvent->IsHandledInRemoteProcess()) { > - aEvent->mAccessKeyForwardedToChild = true; > - } else { > AccessKeyInfo accessKeyInfo(aEvent, aAccessCharCodes); > nsContentUtils::CallOnAllRemoteChildren(mDocument->GetWindow(), Could you file a bug about sorting out this CallOnAllRemoteChildren oddity. This looks like a possible major bug to me (but not about this bug). ::: layout/base/PresShell.cpp:8158 (Diff revision 3) > nsContentUtils::SetIsHandlingKeyBoardEvent(true); > } > - if (aEvent->IsAllowedToDispatchDOMEvent()) { > + // If EventStateManager or something already stopped the propagation, > + // we shouldn't dispatch event into the DOM tree. > + if (aEvent->IsAllowedToDispatchDOMEvent() && > + !aEvent->PropagationStopped()) { Why this is needed? Looks odd to call PropagationStopped here. Because of this, r-.
Attachment #8884490 - Flags: review?(bugs) → review-
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review163790
Comment on attachment 8886631 [details] Bug 1333459 - part2-1: EventStateManager should have a way to check if there is accesskey which is executed by a specific keyboard event https://reviewboard.mozilla.org/r/157390/#review163800 ::: dom/events/EventStateManager.h:481 (Diff revision 1) > - nsIDocShellTreeItem* aBubbledFrom, > + nsIDocShellTreeItem* aBubbledFrom, > - ProcessingAccessKeyState aAccessKeyState); > + ProcessingAccessKeyState aAccessKeyState, > + bool aExecute); > > - bool ExecuteAccessKey(nsTArray<uint32_t>& aAccessCharCodes, > - bool aIsTrustedEvent); > + /** > + * Look for access key and execute found access key if aExecute is true in Please document what return value means ::: dom/events/EventStateManager.h:486 (Diff revision 1) > - bool aIsTrustedEvent); > + * Look for access key and execute found access key if aExecute is true in > + * the instance. > + */ > + bool LookForAccessKeyAndExecute(nsTArray<uint32_t>& aAccessCharCodes, > + bool aIsTrustedEvent, > + bool aExecuted); aExecute
Attachment #8886631 - Flags: review?(bugs) → review+
Comment on attachment 8886632 [details] Bug 1333459 - part2-3: Make nsMenuBarListener::KeyPress() wait reply from remote process if the eKeyPress event will be sent to a remote process later https://reviewboard.mozilla.org/r/157392/#review163808 ::: dom/xbl/nsXBLWindowKeyHandler.cpp:537 (Diff revision 1) > { > WidgetKeyboardEvent* widgetEvent = > aEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent(); > > - if (widgetEvent->IsCrossProcessForwardingStopped() || > - widgetEvent->mFlags.mOnlySystemGroupDispatchInContent) { > + if (widgetEvent->mFlags.mOnlySystemGroupDispatchInContent || > + !widgetEvent->MaybePostedToRemoteProcessLater()) { I don't understand !widgetEvent->MaybePostedToRemoteProcessLater() ::: widget/WidgetEventImpl.cpp:355 (Diff revision 1) > return false; > } > } > > bool > +WidgetEvent::MaybePostedToRemoteProcessLater() const I don't understand this name
Attachment #8886632 - Flags: review?(bugs) → review+
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review163748 > What is impossible? It's impossible widget (TextEventDispatcher) cannot stop dispatching following eKeyPress event because eKeyDown event hasn't been consumed yet when it tries to dispatch eKeyPress event. > Could you file a bug about sorting out this CallOnAllRemoteChildren oddity. This looks like a possible major bug to me (but not about this bug). Filed bug 1382146. > Why this is needed? Looks odd to call PropagationStopped here. > Because of this, r-. If we don't stop dispatching event here, EventDispatcher will dispatch the event in the system group. https://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/events/EventDispatcher.cpp#497-498,501-502 How do you think about this? Do you think that such events should be dispatched only in the system group first in the main process? I don't think so, because different from nsXBLWindowKeyHandler, ESM doesn't have a chance to stop propagation in the system group.
Comment on attachment 8886631 [details] Bug 1333459 - part2-1: EventStateManager should have a way to check if there is accesskey which is executed by a specific keyboard event https://reviewboard.mozilla.org/r/157390/#review163800 > Please document what return value means Okay, I added documents here and other methods which are renamed by this patch. > aExecute Oops...
Comment on attachment 8886632 [details] Bug 1333459 - part2-3: Make nsMenuBarListener::KeyPress() wait reply from remote process if the eKeyPress event will be sent to a remote process later https://reviewboard.mozilla.org/r/157392/#review163808 > I don't understand !widgetEvent->MaybePostedToRemoteProcessLater() Okay, I'll add comment here. > I don't understand this name What name do you like? The method checks if the event will be sent to a remote process. Therefore, when the target is in a remote process and it's not stopped forwarding to remote process, this returns true. WillBeSentToRemoteProcess()?
Ahaa, that is the reason for PropagationStopped() check. I think we elsewhere use StopPropagation() before event dispatch to explicitly prevent default group listeners to get the event, but still ensure system group gets them. I feel like here, in these patches, we are misusing StopPropagation()/PropagationStopped() a bit. Could we check whether we're waiting for reply from child process, and not PropagationStopped();
Trying with IsWaitingReplyFromRemoteProcess()...
There are two bugs: #1, TabChild dispatches events without resetting mFlags.mWantReplyFromContentProcess. Therefore, even in child process, IsWaitingReplyFromRemoteProcess() returns true before handling the event in PresShell. So, I think that APZCCallbackHelper::DispatchWidgetEvent() should reset it because it's used when TabChild dispatches events. #2, EventStateManager::(Pre|Post)HandleEvent() consumes events with setting nsEventStatus to nsEventStatus_ConsumeNoDefault but without calling WidgetEvent::PreventDefault(). Additionally, TabChild doesn't respect the status value before sending reply event to the main process. Therefore, TabChild should call PreventDefault() in such case. (I think this should be fixed for any events by bug 331630.) Fixes of those bugs will be included into part2-2 (will be reviewed by smaug).
Hmm, oddly, new patches cause orange with some tests which use synthesizeNativeKey only on macOS. I'll investigate it tomorrow.
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review165302 ::: gfx/layers/apz/util/APZCCallbackHelper.cpp:492 (Diff revision 4) > nsEventStatus > APZCCallbackHelper::DispatchWidgetEvent(WidgetGUIEvent& aEvent) > { > nsEventStatus status = nsEventStatus_eConsumeNoDefault; > if (aEvent.mWidget) { > + aEvent.ResetWaitingReplyFromRemoteProcessState(); This looks like a wrong place to reset this kind of flag. Do we even need to forward the flag to child process
Attachment #8884490 - Flags: review?(bugs) → review-
Comment on attachment 8888732 [details] Bug 1333459 - part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process https://reviewboard.mozilla.org/r/159766/#review165312 ::: widget/cocoa/TextInputHandler.mm:2818 (Diff revision 1) > nsAString* insertString = currentKeyEvent->mInsertString; > if (aKeyboardEvent.mMessage == eKeyPress && aIndexOfKeypress == 0 && > (!insertString || insertString->IsEmpty())) { > // Inform the child process that this is an event that we want a reply > // from. > - aKeyboardEvent.mFlags.mWantReplyFromContentProcess = true; > + // XXX This should be called only when the target is a remote process. This is indeed quite annoying.
Attachment #8888732 - Flags: review?(bugs) → review+
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review165302 > This looks like a wrong place to reset this kind of flag. > Do we even need to forward the flag to child process Hmm, then, you might not like though, I think that each TabChild should reset the flag manually because the flag cannot be reset in ParamTraits since TabChild needs to check the flag when it sends reply event...
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review165302 > Hmm, then, you might not like though, I think that each TabChild should reset the flag manually because the flag cannot be reset in ParamTraits since TabChild needs to check the flag when it sends reply event... Okay, I wrap APZCCallbackHelper::DispatchWidgetEvent() with TabChild::DispatchWidgetEvent() and reset the state in it before calling APZCCallbackHelper::DispatchWidgetEvent().
Comment on attachment 8884490 [details] Bug 1333459 - part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys https://reviewboard.mozilla.org/r/155230/#review165514 ::: dom/ipc/TabChild.cpp:1639 (Diff revisions 4 - 5) > } > return false; > } > > +nsEventStatus > +TabChild::DispatchWidgetEvent(WidgetGUIEvent& aEvent) Could we call this DispatchWidgetEventViaAPZ
Attachment #8884490 - Flags: review?(bugs) → review+
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/2d219d3f6dc4 part1: Move methods of EventStateManager which check modifiers of access key to WidgetKeyboardEvent r=smaug https://hg.mozilla.org/integration/autoland/rev/69b23bbdb956 part2-1: EventStateManager should have a way to check if there is accesskey which is executed by a specific keyboard event r=smaug https://hg.mozilla.org/integration/autoland/rev/ce1fe9986ce9 part2-2: EventStateManager should check if it needs to wait reply from remote content before handling access keys r=smaug https://hg.mozilla.org/integration/autoland/rev/a1d8dca63454 part2-3: Make nsMenuBarListener::KeyPress() wait reply from remote process if the eKeyPress event will be sent to a remote process later r=smaug https://hg.mozilla.org/integration/autoland/rev/73e6945ecfa7 part3: Add automated tests into browser_accesskeys.js r=smaug https://hg.mozilla.org/integration/autoland/rev/57e0c8a166d2 part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process r=smaug
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17) Thank you for fixing this bug!
Depends on: 1435530
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: