PluginInstanceChild::PluginWindowProcInternal() should use switch-case statement at checking each message because adding new message handler may make damage to the performance and switch-case statement is easier to read for Windows message handler. Review commit: https://reviewboard.mozilla.org/r/42369/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42369/

Before sending key messages to plugin's windowproc, we should check if the key combination is reserved by chrome. If a key combination is consumed by reserved command in the parent process, we should not send it to the plugin. The sending message should not be raw WM_(SYS)KEY(DOWN|UP) because if we send it to the parent process, parent process will call ::TranslateMessage() and the key event may be handled by IME or may cause WM_(SYS)CHAR in the parent process. Therefore, this patch defines MOZ_WM_KEYDOWN and MOZ_WM_KEYUP. Note that sending message to the parent process may make damage to text input on plugins. Therefore, this is performed only when at least one modifier key except Shift key is pressed. For sharing our defining messages in all message handlers, this patch creates a new header file which just defines messages. Review commit: https://reviewboard.mozilla.org/r/42371/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42371/

Only on Windows, when at least one modifier key except Shift key is pressed, each key event is sent to the parent process from a plugin process before sending the key event to the focused plugin. At this time, parent process needs to check if the key combination is reserved by chrome and if it's reserved, perform the command. For performing this without changes in normal keyboard event path, this patch defines new keyboard events, ePluginKeyDown and ePluginKeyUp. nsXBLWindowKeyHandler should listen to them and handle them as eKeyDown or eKeyUp event. Finally, ePluginKeyDown should perform a keypress event handler if it's reserved because keypress event won't be fired. Review commit: https://reviewboard.mozilla.org/r/42373/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42373/

nsWindow for Windows, NativeKey and KeyboardLayout should handle MOZ_WM_KEYDOWN and MOZ_WM_KEYUP like WM_(SYS)KEYDOWN and WM_(SYS)KEYUP. Dispatching event for them should be ePluginKeyDown and ePluginKeyUp since this guarantees that neither keydown nor keyup event handler can listen to this. If the event is consumed, return true to the sender (a plugin process). Otherwise, return false. Note that MOZ_WM_KEYDOWN shouldn't cause eKeyPress because ePluginKeyDown should be handled as a keypress event if the key combination matches with a reserved command and we need to return to the plugin process as soon as possible. Review commit: https://reviewboard.mozilla.org/r/42375/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42375/

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #4) > Created attachment 8734589 [details] > MozReview Request: Bug 1257759 part.3 Add ePluginKeyDown and ePluginKeyUp > events which perform reserved shortcut keys r?smaug > > Only on Windows, when at least one modifier key except Shift key is pressed, > each key event is sent to the parent process from a plugin process before > sending the key event to the focused plugin. > > At this time, parent process needs to check if the key combination is > reserved by chrome and if it's reserved, perform the command. > > For performing this without changes in normal keyboard event path, this > patch defines new keyboard events, ePluginKeyDown and ePluginKeyUp. > nsXBLWindowKeyHandler should listen to them and handle them as eKeyDown or > eKeyUp event. Finally, ePluginKeyDown should perform a keypress event > handler if it's reserved because keypress event won't be fired. > > Review commit: https://reviewboard.mozilla.org/r/42373/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/42373/ FYI: Another possible approach is, just using eKeyDown and eKeyUp but WidgetKeyboardEvent should have a new member like "mNotFollowedByKeyPress". Then, if it's true, nsXBLWindowKeyHandler should perform matching keypress event handler with eKeyDown. This *might* be useful if we will stop dispatching keypress events completely for non-printable key events like "Enter", "Escape". Then, like Ctrl-Tab, nobody needs to listen to new pluginkeyevents. Even if you like this approach better, I'd like to do this after I fix all regressions of the series of related patches. I meant, I've already landed enough risky changes at once. So, putting off such risky implementation later. But if you think I should take this approach here, I'll rewrite the patch and part.4. Up to you, smaug.

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato https://reviewboard.mozilla.org/r/42369/#review39101

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm https://reviewboard.mozilla.org/r/42371/#review39169 Could you add additional review to jimm? (reviewboard cannot add additional reviewer by reviewer, bug 1161230) ::: widget/windows/WinMessages.h:56 (Diff revision 1) > + > +/***************************************************************************** > + * WM_* messages and related constants which may not be defined by > + * old Windows SDK > + ****************************************************************************/ > + We can remove most following defines such as WM_THEMECHANGED becasue these are for old Windows SDK and WINVER == 0x0500. But I will handle it as bug 953242.

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm https://reviewboard.mozilla.org/r/42371/#review39203 ::: dom/plugins/ipc/PluginInstanceChild.cpp:1692 (Diff revision 1) > + break; > + } > + > + // If the key combination is reserved by chrome, we shouldn't allow > + // plugins to handle the key. Let's check if it's reserved first. > + HWND parentWnd = ::GetParent(mPluginParentHWND); This will fail in 64-bit builds that have the sandbox enabled. ::: dom/plugins/ipc/PluginInstanceChild.cpp:1695 (Diff revision 1) > + // If the key combination is reserved by chrome, we shouldn't allow > + // plugins to handle the key. Let's check if it's reserved first. > + HWND parentWnd = ::GetParent(mPluginParentHWND); > + bool isKeydown = message == WM_KEYDOWN || message == WM_SYSKEYDOWN; > + if (parentWnd && > + ::SendMessage(parentWnd, isKeydown ? MOZ_WM_KEYDOWN : MOZ_WM_KEYUP, This doesn't look safe for a couple reasons - 1) If the browser is in an ipc rpc or sync send, this will end up getting processed by our deferred handling code in WindowsMessageLoop.cpp. 2) If this gets processed in the browser, what does the browser do with this message? Could this generate gecko events? 3) In 64-bit builds this call will fail due to a bad hwnd.

(In reply to Jim Mathies [:jimm] from comment #9) > Comment on attachment 8734588 [details] > MozReview Request: Bug 1257759 part.2 Send WM_KEYDOWN and WM_KEYUP messages > to PluginInstanceChild::PluginWindowProcInternal() for checking if the key > combination is reserved by chrome r?m_kato > > https://reviewboard.mozilla.org/r/42371/#review39203 > > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1692 > (Diff revision 1) > > + break; > > + } > > + > > + // If the key combination is reserved by chrome, we shouldn't allow > > + // plugins to handle the key. Let's check if it's reserved first. > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > This will fail in 64-bit builds that have the sandbox enabled. In 64-bit build with sandbox, windowed mode won't be used, isn't it? > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1695 > (Diff revision 1) > > + // If the key combination is reserved by chrome, we shouldn't allow > > + // plugins to handle the key. Let's check if it's reserved first. > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > + bool isKeydown = message == WM_KEYDOWN || message == WM_SYSKEYDOWN; > > + if (parentWnd && > > + ::SendMessage(parentWnd, isKeydown ? MOZ_WM_KEYDOWN : MOZ_WM_KEYUP, > > This doesn't look safe for a couple reasons - > > 1) If the browser is in an ipc rpc or sync send, this will end up getting > processed by our deferred handling code in WindowsMessageLoop.cpp. Oh, right. Can we check if it's in IPC RPC (we can check if it's in SendMessage(), though)? Or should I use other way to send key messages to the chrome process, e.g., IPC? > 2) If this gets processed in the browser, what does the browser do with this > message? Could this generate gecko events? Yes, it generates new Gecko events. The events are handled only by nsXBLWindowKeyHandler for now. However, it's possible to listen them in web content if web developers know the event name. So, yes, I should change the following patch which won't expose the new event to the web contents. > 3) In 64-bit builds this call will fail due to a bad hwnd. So, as I said above, this isn't problem.

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm The new events shouldn't be exposed to web contents because if web contents access plugins, it causes deadlock.

Looks like users cannot enable windowed mode if Flash is in sandbox: https://reviewboard.mozilla.org/r/21809/diff/2#index_header And I assume that sandbox is enabled only when the plugin is Flash. http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?rev=b153d20c301d#1182 However, looks like that user can enable sandbox for Silverlight...

Okay, I'm rewriting the patches with using IPC instead of SendMessage(). However, I still have a question. That is, checking |(InSendMessageEx(nullptr)&(ISMEX_REPLIED|ISMEX_SEND)) == ISMEX_SEND| before IPC is enough for safe.

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug I need to rewrite this too.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #10) > (In reply to Jim Mathies [:jimm] from comment #9) > > Comment on attachment 8734588 [details] > > MozReview Request: Bug 1257759 part.2 Send WM_KEYDOWN and WM_KEYUP messages > > to PluginInstanceChild::PluginWindowProcInternal() for checking if the key > > combination is reserved by chrome r?m_kato > > > > https://reviewboard.mozilla.org/r/42371/#review39203 > > > > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1692 > > (Diff revision 1) > > > + break; > > > + } > > > + > > > + // If the key combination is reserved by chrome, we shouldn't allow > > > + // plugins to handle the key. Let's check if it's reserved first. > > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > > > This will fail in 64-bit builds that have the sandbox enabled. > > In 64-bit build with sandbox, windowed mode won't be used, isn't it? You are correct. Sorry, I was thinking this was similar to bug 1257911, this that's related to code that runs in content. > > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1695 > > (Diff revision 1) > > > + // If the key combination is reserved by chrome, we shouldn't allow > > > + // plugins to handle the key. Let's check if it's reserved first. > > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > > + bool isKeydown = message == WM_KEYDOWN || message == WM_SYSKEYDOWN; > > > + if (parentWnd && > > > + ::SendMessage(parentWnd, isKeydown ? MOZ_WM_KEYDOWN : MOZ_WM_KEYUP, > > > > This doesn't look safe for a couple reasons - > > > > 1) If the browser is in an ipc rpc or sync send, this will end up getting > > processed by our deferred handling code in WindowsMessageLoop.cpp. > > Oh, right. Can we check if it's in IPC RPC (we can check if it's in > SendMessage(), though)? Or should I use other way to send key messages to > the chrome process, e.g., IPC? Ugh. :) Sync from plugin to chrome in general is really bad. Is there anyway we can forward the reserve key info down in advance and keep it up to date through lazy (async) updates? > > 2) If this gets processed in the browser, what does the browser do with this > > message? Could this generate gecko events? > > Yes, it generates new Gecko events. If the gecko event generates IPC traffic from content -> plugin you get a deadlock. We want to avoid this. > The events are handled only by > nsXBLWindowKeyHandler for now. However, it's possible to listen them in web > content if web developers know the event name. So, yes, I should change the > following patch which won't expose the new event to the web contents.

(In reply to Jim Mathies [:jimm] from comment #16) > > > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1695 > > > (Diff revision 1) > > > > + // If the key combination is reserved by chrome, we shouldn't allow > > > > + // plugins to handle the key. Let's check if it's reserved first. > > > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > > > + bool isKeydown = message == WM_KEYDOWN || message == WM_SYSKEYDOWN; > > > > + if (parentWnd && > > > > + ::SendMessage(parentWnd, isKeydown ? MOZ_WM_KEYDOWN : MOZ_WM_KEYUP, > > > > > > This doesn't look safe for a couple reasons - > > > > > > 1) If the browser is in an ipc rpc or sync send, this will end up getting > > > processed by our deferred handling code in WindowsMessageLoop.cpp. > > > > Oh, right. Can we check if it's in IPC RPC (we can check if it's in > > SendMessage(), though)? Or should I use other way to send key messages to > > the chrome process, e.g., IPC? > > > Ugh. :) Sync from plugin to chrome in general is really bad. Is there anyway > we can forward the reserve key info down in advance and keep it up to date > through lazy (async) updates? Hmm, it's difficult because of "key hell"... We need the complicated path for matching each key message with the keyboard layout. The main reason why I need to use synchronous communication between plugin process and chrome process is that key messages needs to be passed to ::TraslateMessage() API and plugin does it already. If we can steal key messages before that, we can post the key messages to chrome process and we can post back the key message again from EventStateManager like a default action. Is this possible? I'm not so familiar with Win32 message hook. > > > 2) If this gets processed in the browser, what does the browser do with this > > > message? Could this generate gecko events? > > > > Yes, it generates new Gecko events. > > If the gecko event generates IPC traffic from content -> plugin you get a > deadlock. We want to avoid this. Yeah, my new patch does this.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #17) > (In reply to Jim Mathies [:jimm] from comment #16) > > > > ::: dom/plugins/ipc/PluginInstanceChild.cpp:1695 > > > > (Diff revision 1) > > > > > + // If the key combination is reserved by chrome, we shouldn't allow > > > > > + // plugins to handle the key. Let's check if it's reserved first. > > > > > + HWND parentWnd = ::GetParent(mPluginParentHWND); > > > > > + bool isKeydown = message == WM_KEYDOWN || message == WM_SYSKEYDOWN; > > > > > + if (parentWnd && > > > > > + ::SendMessage(parentWnd, isKeydown ? MOZ_WM_KEYDOWN : MOZ_WM_KEYUP, > > > > > > > > This doesn't look safe for a couple reasons - > > > > > > > > 1) If the browser is in an ipc rpc or sync send, this will end up getting > > > > processed by our deferred handling code in WindowsMessageLoop.cpp. > > > > > > Oh, right. Can we check if it's in IPC RPC (we can check if it's in > > > SendMessage(), though)? Or should I use other way to send key messages to > > > the chrome process, e.g., IPC? > > > > > > Ugh. :) Sync from plugin to chrome in general is really bad. Is there anyway > > we can forward the reserve key info down in advance and keep it up to date > > through lazy (async) updates? > > Hmm, it's difficult because of "key hell"... We need the complicated path > for matching each key message with the keyboard layout. > > The main reason why I need to use synchronous communication between plugin > process and chrome process is that key messages needs to be passed to > ::TraslateMessage() API and plugin does it already. If we can steal key > messages before that, we can post the key messages to chrome process and we > can post back the key message again from EventStateManager like a default > action. Is this possible? I'm not so familiar with Win32 message hook. > I think you want to set this up so you can make a decision on the plugin instance side whether or not the browser wants to consume the key event and do that. If we can't, I would say don't try and fix this. We'll be switching to windowless for 32-bit flash this summer which works around this bug for most plugin use on the web.

Okay, I'm thinking another approach. That is, when PluginInstanceChild recevies WM_KEYDOWN/WM_KEYUP, it should send the messages to nsWindow with asynchronous IPC. Then, nsWindow dispatches eKeyDownOnPlugin/eKeyUpOnPlugin to execute reserved shortcut keys. After that, nsWindow should post MOZ_WM_KEYDOWN/MOZ_WM_KEYUP to the plugin window and PluginInstanceChild sends the key message to plugin. If PluginInstanceChild receives another WM_KEYDOWN/WM_KEYUP after posting MOZ_WM_KEYDOWN_ON_PLUGIN/MOZ_WM_KEYUP_ON_PLUGIN but not received MOZ_WM_KEYDOWN/MOZ_WM_KEYUP yet, whole key events should be handled in same path. Then, we can avoid to shuffle the order of key events on plugin. Of course, the performance may not good but I believe that it's rare case to type characters normally immediately after Ctrl+foo or Alt+foo.

https://reviewboard.mozilla.org/r/42371/#review39203 > This will fail in 64-bit builds that have the sandbox enabled. New patches won't use HWND to send/post key events to the chrome process. > This doesn't look safe for a couple reasons - > > 1) If the browser is in an ipc rpc or sync send, this will end up getting processed by our deferred handling code in WindowsMessageLoop.cpp. > 2) If this gets processed in the browser, what does the browser do with this message? Could this generate gecko events? > 3) In 64-bit builds this call will fail due to a bad hwnd. New patches use neither synchronous IPC nor SendMessage(), i.e., fully asynchronus handling about shortcut key handling and following messages if it's necessary.

When PluginInstanceChild receives native key events, it should post the events to the chrome process first for checking if the key combination is reserved. However, posting all key events to the chrome process may make damage to the performance of text input. Therefore, this patch starts to post a key event whose key combination may be a shortcut key. However, for avoiding to shuffle the event order, it posts following key events until all posted key events are handled by the chrome process. For receiving response from widget, this patch defines nsIKeyEventInPluginCallback. It's specified by nsIWidget::OnKeyEventInPluginProcess() for ensuring the caller will receive the reply. Basically, the caller of nsIWidget::OnKeyEventInPluginProcess() should reply to the child process. However, if the widget is a PuppetWidget, it cannot return the result synchronously. Therefore, PuppetWidget::OnKeyEventInPluginProcess() returns NS_SUCCESS_EVENT_HANDLED_ASYNCHRONOUSLY and stores the callback to mKeyEventInPluginCallbacks. Then, TabParent::HandledKeyEventBeforePlugin() will call PuppetWidget::HandledKeyEventBeforePlugin(). Review commit: https://reviewboard.mozilla.org/r/46325/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46325/

On Windows, applications cannot handle IME messages asynchronously. Therefore, we cannot put off to send IME messages to plugin even if there are some pending keyboard events which were posted to the parent process. This patch makes PluginInstanceChild consume all key events which are returned from the parent process during IME composition. And when an IME composition is committed, mark pending key events as outdated. Review commit: https://reviewboard.mozilla.org/r/46327/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46327/

If a plugin process posts native key events to the widget, it needs to check if the key combination is reserved by chrome because if it's reserved by chrome, the reserved shortcut key handler should be executed and the event shouldn't be handled by the focused plugin. This patches add eKeyDownOnPlugin and eKeyUpOnPlugin. nsXBLWindowKeyHandler will listen to them and handle them as normal keydown and keypress or keyup event. Note that these events won't be fired on content in the default event group and won't be sent to the remote process. Review commit: https://reviewboard.mozilla.org/r/46329/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46329/

eKeyDownOnPlugin (mozkeydownonplugin) and eKeyUpOnPlugin (mozkeyuponplugin) should execute if the key combination is reserved by the linked <command> element. Note that there is no eKeyPressOnPlugin. Therefore, eKeyDownOnPlugin may execute shortcut key handler which is registered as a keypress event handler. Review commit: https://reviewboard.mozilla.org/r/46331/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46331/

Implementing nsWindow::OnKeyEventInPluginProcess() on Windows. This patch makes NativeKey class dispatches eKeyDownOnPlugin and eKeyUpOnPlugin when the method is called. Review commit: https://reviewboard.mozilla.org/r/46333/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46333/

nsWindow for Windows cannot decide if a preceding WM_*KEYDOWN or WM_*KEYUP which is a preceding message of WM_*CHAR is consumed because nsWindow cannot know if WM_*CHAR message came from same window which received the preceding WM_*KEYDOWN or WM_*KEYUP. Therefore, PluginInstanceChild should do that. Review commit: https://reviewboard.mozilla.org/r/46335/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46335/

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42369/diff/1-2/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42371/diff/1-2/

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42373/diff/1-2/

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42375/diff/1-2/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm https://reviewboard.mozilla.org/r/42371/#review43687

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm https://reviewboard.mozilla.org/r/42373/#review43689

Comment on attachment 8742054 [details] MozReview Request: Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm https://reviewboard.mozilla.org/r/46325/#review43693 ::: dom/ipc/PBrowser.ipdl:295 (Diff revision 1) > + * > + * aKeyEventData The native key event data. The actual type copied into > + * NativeEventData depending on the caller. Please check > + * PluginInstanceChild. > + */ > + prio(urgent) async OnKeyEventInPluginProcess(NativeEventData aKeyEventData); I think we can shorten these api names up and make them more explicit, how about: OnWindowedPluginKeyEvent HandledWindowedPluginKeyEvent ::: dom/plugins/base/nsNPAPIPluginInstance.cpp:1188 (Diff revision 1) > + if (NS_WARN_IF(!library)) { > + return NS_ERROR_FAILURE; > + } > + > + // This method should be called when the plugin is running OOP. > + if (NS_WARN_IF(!library->IsOOP())) { I think this is a useless check here, oop isn't specific to e10s.

Comment on attachment 8742055 [details] MozReview Request: Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm https://reviewboard.mozilla.org/r/46327/#review43709

Comment on attachment 8742058 [details] MozReview Request: Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm https://reviewboard.mozilla.org/r/46333/#review43713

Comment on attachment 8742059 [details] MozReview Request: Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm https://reviewboard.mozilla.org/r/46335/#review43715

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42369/diff/2-3/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42371/diff/2-3/

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42373/diff/2-3/

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42375/diff/2-3/

Comment on attachment 8742054 [details] MozReview Request: Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46325/diff/1-2/

Comment on attachment 8742055 [details] MozReview Request: Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46327/diff/1-2/

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46329/diff/1-2/

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46331/diff/1-2/

Comment on attachment 8742058 [details] MozReview Request: Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46333/diff/1-2/

Comment on attachment 8742059 [details] MozReview Request: Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46335/diff/1-2/

smaug: Could you review this bug? I want to land them in this cycle for consistency with same fix on Mac.

Yup, sorry. Tiny bit long review queue. I'll prioritize this one.

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug https://reviewboard.mozilla.org/r/42375/#review44987

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug https://reviewboard.mozilla.org/r/46331/#review44999 Could you update the somewhat minor issue and ask review again. This is quite tricky stuff, so I feel I'll need to look at this patch again (real soon!) ::: dom/xbl/nsXBLWindowKeyHandler.cpp:420 (Diff revision 2) > +{ > + return GetEventTypeForHandler(aEvent->AsEvent()); > +} > + > +already_AddRefed<nsIAtom> > +nsXBLWindowKeyHandler::GetEventTypeForHandler(nsIDOMEvent* aEvent) const The method name isn't describing what it does. At least add some good explanation why we're doing this magic, where event types foo are converted to bar. and could we not use string comparison, but widget events mMessage. That would emphasize this is very magic low level stuff. ::: dom/xbl/nsXBLWindowKeyHandler.cpp:743 (Diff revision 2) > } > // Otherwise, we've not found a handler for the event yet. > continue; > } > > + // If it's not reserved and the event is a key event on a plugin, Is this the behavior we have currently on non-e10s? I assume it is.

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug https://reviewboard.mozilla.org/r/46329/#review44997 I'd like to see a new patch for this too, so that I'm forced to re-read the patch :) ::: layout/base/nsPresShell.cpp:7127 (Diff revision 2) > > - // Dispatch event directly if the event is synthesized from > - // nsITextInputProcessor, or there is no need to fire > + // Dispatch event directly if the event is a keypress event, a key event on > + // plugin, or there is no need to fire beforeKey* and afterKey* events. > - // beforeKey* and afterKey* events. > if (aEvent.mMessage == eKeyPress || > + aEvent.IsKeyEventOnPlugin() || Ok, so this is fine, since we don't need to care about the b2g stuff here. ::: layout/base/nsPresShell.cpp:7194 (Diff revision 2) > bool > PresShell::ForwardKeyToInputMethodApp(nsINode* aTarget, > WidgetKeyboardEvent& aEvent, > nsEventStatus* aStatus) > { > - if (!XRE_IsParentProcess() || aEvent.mIsSynthesizedByTIP) { > + if (!XRE_IsParentProcess() || aEvent.mIsSynthesizedByTIP || ....nor here. ::: widget/TextEventDispatcher.cpp:402 (Diff revision 2) > const WidgetKeyboardEvent& aKeyboardEvent, > nsEventStatus& aStatus, > void* aData, > uint32_t aIndexOfKeypress) > { > - MOZ_ASSERT(aMessage == eKeyDown || aMessage == eKeyUp || > + MOZ_ASSERT(WidgetKeyboardEvent::IsKeyDownEvent(aMessage) || In this method I'd like to see a comment why we're using these IsKey*Event methods. I'm not sure I like that those methods hide the plugin-ness of the events, since I assume people could easily start to use them elsewhere assuming they deal with normal events only. ::: widget/TextEvents.h:156 (Diff revision 2) > { > + // If this is a keyboard event on a plugin, it shouldn't fired on content. > + mFlags.mOnlySystemGroupDispatchInContent = > + mFlags.mNoCrossProcessBoundaryForwarding = IsKeyEventOnPlugin(); > + } > + So could we call the methods something like IsKeyOrPluginKeyDownEvent ? Same with other methods. Or do you explicitly want to hide the plugin-ness of the events. If so, please add a comment.

> and could we not use string comparison, but widget events mMessage. That would emphasize this is > very magic low level stuff. Sure. (And I'd like to rewrite nsXBLWindowKeyHandler with Widget*Event later because we fixed bug 1256589.) >> + // If it's not reserved and the event is a key event on a plugin, > > Is this the behavior we have currently on non-e10s? I assume it is. I'm not sure what did you mean here. This bug isn't related to e10s vs. non-e10s. But perhaps, my patches work with only OOPP (IIRC, in process plugin has already gone completely). > I'm not sure I like that those methods hide the plugin-ness of the events, since I assume people > could easily start to use them elsewhere assuming they deal with normal events only. Yes. But in most cases, key events on plugin should be ignored since they should be handled only by some handlers which need to intercept key events before plugin like the reserved shortcut keys. > So could we call the methods something like > IsKeyOrPluginKeyDownEvent ? Same with other methods. Sure. The purpose of the methods are just for easier to read since without the methods, there were a lot of additional conditions such as |aMessage == eKeyDown || aMessage == eKeyDownOnPlugin || aMessage == eKeyUp || aMessage == eKeyUpOnPlugin|.

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42369/diff/3-4/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42371/diff/3-4/

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42373/diff/3-4/

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42375/diff/3-4/

Comment on attachment 8742054 [details] MozReview Request: Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46325/diff/2-3/

Comment on attachment 8742055 [details] MozReview Request: Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46327/diff/2-3/

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46329/diff/2-3/

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46331/diff/2-3/

Comment on attachment 8742058 [details] MozReview Request: Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46333/diff/2-3/

Comment on attachment 8742059 [details] MozReview Request: Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46335/diff/2-3/

https://reviewboard.mozilla.org/r/46329/#review44997 > In this method I'd like to see a comment why we're using these IsKey*Event methods. > > I'm not sure I like that those methods hide the plugin-ness of the events, since I assume people could easily start to use them elsewhere assuming they deal with normal events only. Renamed to IsKeyDownOrKeyDownOnPlugin() and IsKeyUpOrKeyUpOnPlugin().

https://reviewboard.mozilla.org/r/46331/#review44999 > The method name isn't describing what it does. > At least add some good explanation why we're doing this magic, where event types foo are converted to bar. > > and could we not use string comparison, but widget events mMessage. That would emphasize this is very magic low level stuff. Alhtough, I don't rename the method, but I added the explanation into the definition in the header file.

https://reviewboard.mozilla.org/r/46331/#review44999 > Alhtough, I don't rename the method, but I added the explanation into the definition in the header file. If you have better name for the method, let me know. I'll rename it before landing.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #57) > Yes. But in most cases, key events on plugin should be ignored since they > should be handled only by some handlers which need to intercept key events > before plugin like the reserved shortcut keys. Which is exactly why I don't like the plugin-ness to be hidden. Otherwise we'll end up handling those events in cases we shouldn't.

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug https://reviewboard.mozilla.org/r/46329/#review45175 So currently, when plugin (non-windowed at least) is handling key event, capturing listeners in DOM in ancestors of the <object> can handle the event, right? Do we end up changing that behavior with all these patches? But r+ for this particular patch. ::: widget/TextEventDispatcher.cpp:421 (Diff revision 3) > if (aMessage == eKeyPress && !aKeyboardEvent.ShouldCauseKeypressEvents()) { > return false; > } > > // Basically, key events shouldn't be dispatched during composition. > - if (IsComposing()) { > + if (IsComposing() && !WidgetKeyboardEvent::IsKeyEventOnPlugin(aMessage)) { So this could still use a short comment why key event on plugin isn't handled here.

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug https://reviewboard.mozilla.org/r/46331/#review45189 ::: dom/xbl/nsXBLWindowKeyHandler.h:80 (Diff revision 3) > bool* aOutReservedForChrome = nullptr); > > + // Returns event type for matching between aWidgetKeyboardEvent and > + // shortcut key handlers. This is used for calling WalkHandlers(), > + // WalkHandlersInternal() and WalkHandlersAndExecute(). > + nsIAtom* GetEventTypeForHandler( I think the method name should be clearer. Maybe ConvertEventToDOMEventType or some such, hinting that is it actually possibly changing the type. ::: dom/xbl/nsXBLWindowKeyHandler.cpp:426 (Diff revision 3) > + return nsGkAtoms::keyup; > + } > + if (aWidgetKeyboardEvent.mMessage == eKeyPress) { > + return nsGkAtoms::keypress; > + } > + return nullptr; Shouldn't we MOZ_ASSERT here if we're about to return null. ::: dom/xbl/nsXBLWindowKeyHandler.cpp:473 (Diff revision 3) > + } > + } > > + nsCOMPtr<nsIAtom> eventTypeAtom(GetEventTypeForHandler(*widgetKeyboardEvent)); > + if (NS_WARN_IF(!eventTypeAtom)) { > + return NS_ERROR_OUT_OF_MEMORY; NS_ERROR_OUT_OF_MEMORY sure isn't right anymore. NS_ERROR_UNEXPECTED perhaps, but in practice the assertion I suggested should be enough, right?

> So currently, when plugin (non-windowed at least) is handling key event, capturing listeners in DOM > in ancestors of the <object> can handle the event, right? Yes. Although, I don't like the behavior (I think that key events should be handled in system group or as default action (i.e., after all propagations)). > Do we end up changing that behavior with all these patches? No, this bug tries to fix only a specific issue of windowed mode on Windows. That is, in windowed mode on Windows, native key events are fired only in the plugin process. Therefore, currently, no keyboard events are fired when a windowed plugin has focus on Windows. However, we should execute at least reserved shortcut keys even on windowed plugins. Therefore, these patches post key events whose modifier state may match shortcut key (i.e., only when Ctrl or Alt is pressed) to the chrome process asynchronously (and receiving its result asynchronously). The reason why we don't post all key events to the chrome process is that it may make damage to the text input performance because at least 4 times, asynchronous IPCs are necessary per native key event (one key press causes at least 3 native key events on Windows). Additionally, IME on Windows needs to be handled by plugin *synchronously*. So, we cannot post key events if there is composition in the plugin or key events which may cause starting composition since we shouldn't shuffle event order at sending plugin. Due to those limitations, I'm trying to fix only reserved shortcut key issue on windowed plugin on Windows for now. In other words, these patches don't change any behavior on web contents since the new key events on plugin are never fired in the default event group on web contents. So, just creating a chance to execute reserved shortcut key handler before sending native key events to focused plugin.

Btw, I thought we were going to get rid of windowed plugins. But perhaps that isn't going to happen real soon.

> So this could still use a short comment why key event on plugin isn't handled here. Both plugin process and our chrome process have independent IME contexts. So, handling key events on plugins don't need to check our chrome process's composition state. I'll add a comment before landing.

(In reply to Olli Pettay [:smaug] from comment #75) > Btw, I thought we were going to get rid of windowed plugins. But perhaps > that isn't going to happen real soon. Yes. But it's only for Flash Player. I.e., if we'd put off to ban the other plugins (especially, Silverlight), we'd need this fix even after getting rid of windowed Flash Player.

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42369/diff/4-5/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42371/diff/4-5/

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42373/diff/4-5/

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42375/diff/4-5/

Comment on attachment 8742054 [details] MozReview Request: Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46325/diff/3-4/

Comment on attachment 8742055 [details] MozReview Request: Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46327/diff/3-4/

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46329/diff/3-4/

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46331/diff/3-4/

Comment on attachment 8742058 [details] MozReview Request: Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46333/diff/3-4/

Comment on attachment 8742059 [details] MozReview Request: Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46335/diff/3-4/

Comment on attachment 8734587 [details] MozReview Request: Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42369/diff/5-6/

Comment on attachment 8734588 [details] MozReview Request: Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42371/diff/5-6/

Comment on attachment 8734589 [details] MozReview Request: Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42373/diff/5-6/

Comment on attachment 8734590 [details] MozReview Request: Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42375/diff/5-6/

Comment on attachment 8742054 [details] MozReview Request: Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46325/diff/4-5/

Comment on attachment 8742055 [details] MozReview Request: Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46327/diff/4-5/

Comment on attachment 8742056 [details] MozReview Request: Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46329/diff/4-5/

Comment on attachment 8742057 [details] MozReview Request: Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46331/diff/4-5/

Comment on attachment 8742058 [details] MozReview Request: Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46333/diff/4-5/

Comment on attachment 8742059 [details] MozReview Request: Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46335/diff/4-5/

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ca78699bae28da5a8ec8c0d13f556350f441b3 Bug 1257759 part.1 Use switch-case at the first message handling in PluginInstanceChild::PluginWindowProcInternal() r=m_kato https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a49478bfcb151d3adfa3b37e92abff29ac3d94 Bug 1257759 part.2 Separate Windows' message and related definitions from nsWindowDefs.h to mozilla/widget/WinMessages.h r=jimm https://hg.mozilla.org/integration/mozilla-inbound/rev/8ced614c9a1ff986eb1be374b75f96d545c5a8a1 Bug 1257759 part.3 ModifierKeyState should be available in plugin module r=jimm https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2f821d43475cc6c1f75844fae3b8f8a7308b11 Bug 1257759 part.4 Rename WidgetGUIEvent::PluginEvent to NativeEventData for using this class to send native event from plugin process to content and/or chrome process r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1952b7fec843cbb6e1b402c7a0e2a42ba9ba335f Bug 1257759 part.5 PluginInstanceChild should post received native key event to chrome process if the key combination may be a shortcut key r=jimm https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6daf0220a4d81cdb4825f7261b5627c80c8f00 Bug 1257759 part.6 Keep event order between keyboard events and IME events in a plugin process r=jimm https://hg.mozilla.org/integration/mozilla-inbound/rev/502b6b849493d82d320ff091a440ab144c79ac62 Bug 1257759 part.7 Add new internal events which represent key events on plugin r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/52cc5374ec1b31933061d385a01bf6b27dec7648 Bug 1257759 part.8 nsXBLWindowKeyHandler should handle eKeyDownOnPlugin and eKeyUpOnPlugin events only with reserved shortcut key handlers r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf2088320755b225f40a7b90ba0381460550421 Bug 1257759 part.9 Implement nsWindow::OnKeyEventInPluginProcess() on Windows r=jimm https://hg.mozilla.org/integration/mozilla-inbound/rev/beab3af262e39c6e77eba2c032794a89c4d95493 Bug 1257759 part.10 PluginInstanceChild should consume WM_*CHAR messages which follow consumed WM_*KEYDOWN or WM_*KEYUP message r=jimm

https://hg.mozilla.org/mozilla-central/rev/f9ca78699bae https://hg.mozilla.org/mozilla-central/rev/b7a49478bfcb https://hg.mozilla.org/mozilla-central/rev/8ced614c9a1f https://hg.mozilla.org/mozilla-central/rev/6d2f821d4347 https://hg.mozilla.org/mozilla-central/rev/1952b7fec843 https://hg.mozilla.org/mozilla-central/rev/2b6daf0220a4 https://hg.mozilla.org/mozilla-central/rev/502b6b849493 https://hg.mozilla.org/mozilla-central/rev/52cc5374ec1b https://hg.mozilla.org/mozilla-central/rev/1bf208832075 https://hg.mozilla.org/mozilla-central/rev/beab3af262e3

This patch has done nothing at all. Shortcuts still don't work in flash. Explanation in bug 78414 comment 678 Personally for Masayuki Nakano: (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London) from bug 78414 comment 679) > Hey, I said, "If you think that it's not enough to reserve shortcut keys for > some functions, please file a bug for each shortcut key". > > And Ctrl+N, Ctrl+W and Ctrl+T should work on Windows. They are reserved shortcut keys. 1) You haven't seen me saying that I want some _additional_ shortcut to be "reserved", so there was no need to repeat an irrelevant phrase from another comment (which I pretty much read). Are you making these logical failures (including discussions in other bugs) on purpose??? 2) I said that NO SHORTCUTS work in flash, so bug 78414 is clearly not "fixed" in any way. You completely ignored that fact, so OK, I'll reopen this bug. There's no need to create more bugs for the same issue as there're plenty of duplicates filed. Including bug 78414, which is not fixed. 3) You completely ignored (I'm getting used to this) the fact that it's not clear which shortcuts as there's no obvious list in any bug you mentioned in bug 78414 comment 676, bug 78414 comment 677 If there was an list, that'd allowed all users to distinguish "request for a new reserved shortcut" from "mozilla failed to provide announced functionality". Well, _I_ was quite confident that actual bug 78414 is the latter case, but you somehow responded as if it was the latter case (and it is). If you ever made clear list of reserved shortcuts existed, there'd be no such confusion (probably).

Hey, don't reopen existing fixed bugs. Please file a new bug for specific bugs. At least on my environment, this fix really work fine.

arni2033: First, you should test in safe mode. Finally, but if you still reproduce a bug for reserved shortcut keys on Windows or Mac, please file a bug for each problem since they must be specific bugs depending on some environment or conditions.

> 1) You haven't seen me saying that I want some _additional_ shortcut to be "reserved" I've not talked only to you. You should read XUL reference and search reserved attribute in mozilla-central.

What about a link? I have the exact same question: What shortcut keys are "reserved", i.e. which shortcuts can I expect to always work even when a plugin is active. Please drop the arrogant attitude, since it's not helpful... Only because arni2033 raised his concerns in a too aggressive manner does not mean they are invalid.