Steps to reproduce: I just tried to maximize my window while in tab view to test a feature, and it entered search mode upon pressing down the windows key. The Windows key should never be remapped, and it is both an OS-level shortcut itself and a modifier key. This occurred on Windows 7, but I assume that it happens on all Windows versions that we support.

Confirming on Linux.

Pushed to try.

Comment on attachment 506498 [details] [diff] [review] patch v1 >+ let sendWindowsKey = function () { >+ let utils = cw.QueryInterface(Components.interfaces.nsIInterfaceRequestor) >+ .getInterface(Components.interfaces.nsIDOMWindowUtils); >+ utils.sendKeyEvent('keydown', 0, 0, 0); Is 0 actually the key code for the windows key? r+ if so; otherwise looks good

A keypress event is generated but with no valuable information so that charCode and keyCode is 0.

Passed try.

(In reply to comment #4) > A keypress event is generated but with no valuable information so that charCode > and keyCode is 0. Oh... That is a bug of widget... We don't map VK_LWIN and VK_RWIN to NS_VK_META. I'll file it and I'll fix it when I refactor the keycode for DOM3 events. But I have no idea why that is confirmed on Linux. And if so, can the patch also fix this bug on Linux? http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkKeyUtils.cpp#76 Looks gdk's meta keys are mapped correctly. Our mapping isn't enough?

(In reply to comment #7) > Oh... That is a bug of widget... We don't map VK_LWIN and VK_RWIN to > NS_VK_META. I thought about something like that :) > I'll file it and I'll fix it when I refactor the keycode for DOM3 events. This probably won't happen for FF4, will it? I'm asking to know if this patch is still valid or we should hold it back. > But I have no idea why that is confirmed on Linux. And if so, can the patch > also fix this bug on Linux? Feel free to CC me on that new bug. I'll try the patch when it's ready. > http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkKeyUtils.cpp#76 > > Looks gdk's meta keys are mapped correctly. Our mapping isn't enough? I'm not familiar with that code but the buttons seem mapped. I'm using Ubuntu 10.10 if that might be relevant.

(In reply to comment #8) > (In reply to comment #7) > > I'll file it and I'll fix it when I refactor the keycode for DOM3 events. > > This probably won't happen for FF4, will it? I'm asking to know if this patch > is still valid or we should hold it back. Yes, I'll work it for next cycle. And such key event shouldn't be dispatched both keyCode and charCode are zero. So, the code should be inoffensive even if I fix the bug. > > But I have no idea why that is confirmed on Linux. And if so, can the patch > > also fix this bug on Linux? > > Feel free to CC me on that new bug. I'll try the patch when it's ready. I meant "the patch" is this bug's (i.e., attachment 507209 [details] [diff] [review]).

(In reply to comment #9) > Yes, I'll work it for next cycle. And such key event shouldn't be dispatched > both keyCode and charCode are zero. So, the code should be inoffensive even if > I fix the bug. Perfect :) > > > But I have no idea why that is confirmed on Linux. And if so, can the patch > > > also fix this bug on Linux? > > > > Feel free to CC me on that new bug. I'll try the patch when it's ready. > > I meant "the patch" is this bug's (i.e., attachment 507209 [details] [diff] [review]). Yeah, it's my patch, I developed it on Linux. Works for me :)

I'll push this later today. Thanks for the patch, Tim. :)

Pushed. http://hg.mozilla.org/mozilla-central/rev/6a883344c0f0

What is the behavior suppose to be after this lands?

When pressing the Windows key while in panorama the search mode should _not_ be activated.

I'm still hitting this. I inserted a |console.log| statement at the beginning of |beforeSearchKeyHandler|, and it logged the following: event.keyCode = 91 event.charCode = 0 in case either of these are useful: user agent string = Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre hardware = Lenovo ThinkPad

Alright; I just read through some documentation at the link below, and it looks like the left windows key is keyCode 91 and the right windows key (when one exists) is keyCode 92. http://msdn.microsoft.com/en-us/library/ms645540%28v=vs.85%29.aspx I think that the beforeSearchKeyHandler code is way too aggressive in snatching key events. If charCode is 0, we should just return. It's much safer to do that. Also, it may be better to capture keypress instead of keydown, since, AFAIK, keypress is only triggered when pressing keys that would produce immediate text input (accent marks, etc. are tricky, but let's not worry about those).

Looks like it opens both search mode and windows start menu search. Before it landed I see the search function open, putting the cursor in the search box, and then open Windows Start Menu with the cursor in the search box. After it landed I hit the Windows key I see the search function open, then my Windows Start Menu also opens and shows the cursor in the search box.

Key whitelists are much safer than key blacklists for this type of code.

Oops, here's the documentation link: http://msdn.microsoft.com/en-us/library/ms645540%28v=vs.85%29.aspx In short, I'd recommend replacing "keydown" with "keypress" and always returning when charCode is zero.

Going to steal this. If you object Tim, say so :)

(In reply to comment #18) > Key whitelists are much safer than key blacklists for this type of code. No, we should use blacklist because there are many keyboard layouts in the world. (In reply to comment #19) > Oops, here's the documentation link: > http://msdn.microsoft.com/en-us/library/ms645540%28v=vs.85%29.aspx > > In short, I'd recommend replacing "keydown" with "keypress" and always > returning when charCode is zero. Don't use keypress event. See bug 610821. We need to move focus at keydown event.

Note that the most correct fix way is nsWindow::MapFromNativeToDOM() should map VK_LWIN and VK_RWIN to NS_VK_META. http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6745

And keyevent.metaKey should be true while Win key is pressed. But I'm not sure why we're not doing so.

> (In reply to comment #19) > > In short, I'd recommend replacing "keydown" with "keypress" and always > > returning when charCode is zero. > > Don't use keypress event. See bug 610821. We need to move focus at keydown > event. Okay. If we're going to use keydown, |event.charCode| is always 0 for keydown events, AFAIK, so I retract my previous statement.

Since it only happens when in Panorama switching to softblocking.

Really, what we want here is the same behavior as find as you type (FAYT) in the browser, right? At least, that appears to be the goal of this feature after looking at the code and playing with it. FAYT does not have this problem, however, and that's because the code for it (https://hg.mozilla.org/mozilla-central/annotate/0a47be5cdd94/toolkit/content/widgets/findbar.xml#l1404) only checks to see if charCode is non-zero. In fact, the FAYT code is substantially simpler than the panorama code. It simple checks for these things (in this order) and will not handle the keypress if any are true: 1) event.ctrlKey evaluates to true 2) event.altKey evaluates to true 3) event.metaKey evaluates to true 4) event.getPreventDefault() evaluates to true 5) event.charCode evaluates to false The panorama code, as checked in, currently does checks (1), (2), and (3), plus a bunch of other checks that would be satisfied by just checking (5). I'm going to attach a patch that brings panorama's behavior in line with FAYT (as soon as I get a buildable tree so I can test it).

Oof, but FAYT doesn't work with IME either. Bug 615879 appears to be trying to deal with that as well, however.

On that note, I don't think I'm the right person to fix this bug.

(In reply to comment #26) > In fact, the FAYT code is substantially simpler than the panorama code. It > simple checks for these things (in this order) and will not handle the keypress > if any are true: > 1) event.ctrlKey evaluates to true > 2) event.altKey evaluates to true > 3) event.metaKey evaluates to true > 4) event.getPreventDefault() evaluates to true > 5) event.charCode evaluates to false > > The panorama code, as checked in, currently does checks (1), (2), and (3), plus > a bunch of other checks that would be satisfied by just checking (5). You misunderstand about (3). On windows, metaKey has never been true. I think that this is the root cause. If you don't mind I'll fix this bug on Fx4.next, I can fix this bug in widget level. I'm not sure such fix can be landed now.

I don't think that this is safe in semantic of code. *However*, on second thought, Win-key usually aren't pressed. So, we can also say this is safe. But then, is there the worth we fix this bug by this patch?

Comment on attachment 508651 [details] [diff] [review] Patch for widget v1.0 Will this patch work for Linux, too? Looks like it's Windows only :/

(In reply to comment #31) > Comment on attachment 508651 [details] [diff] [review] > Patch for widget v1.0 > > Will this patch work for Linux, too? Looks like it's Windows only :/ Ah, I'll check it, wait a couple of hours.

(In reply to comment #30) > I don't think that this is safe in semantic of code. *However*, on second > thought, Win-key usually aren't pressed. So, we can also say this is safe. But > then, is there the worth we fix this bug by this patch? I suspect that we'll want to take this after Firefox 4.

Hmm, there is an platform specific issue on gtk2. I need to research it tomorrow.

Comment on attachment 508651 [details] [diff] [review] Patch for widget v1.0 Hmm, we shouldn't set isMeta true due to compatibility with WebKit. And also we shouldn't do so on Linux too. (On Linux, Win-keys are mapped to super keys on Ubuntu and Fedora.) I think that we should define NS_VK_WIN, NS_VK_SUPER and NS_VK_HYPER. And also we should implement getModifierState() of DOM3 Events for nsIDOMKeyboardEvent. And it can check these *new* modifier keys' state.

FYI: I posted the super and hyper key issue to W3C. http://lists.w3.org/Archives/Public/www-dom/2011JanMar/0020.html

(In reply to comment #35) > I think that we should define NS_VK_WIN, NS_VK_SUPER and NS_VK_HYPER. And also > we should implement getModifierState() of DOM3 Events for nsIDOMKeyboardEvent. > And it can check these *new* modifier keys' state. Sometimes, there are distinct LWIN and RWIN keys. Both should be covered by code checking for the windows key(s).

It must be distinguished by location property. http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-location

(In reply to comment #38) > It must be distinguished by location property. > http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-location Ok cool. That works for me. If we can't fix the underlying widget code problem easily, I think we should still check in a workaround (e.g. check for keyCode 91 or 92) for Firefox 4.

(In reply to comment #39) > (In reply to comment #38) > If we can't fix the underlying widget code problem easily, I think we should > still check in a workaround (e.g. check for keyCode 91 or 92) for Firefox 4. Yes, it's reasonable for now.

(In reply to comment #39) > If we can't fix the underlying widget code problem easily, I think we should > still check in a workaround (e.g. check for keyCode 91 or 92) for Firefox 4. So basically we need another patch like attachment 507209 [details] [diff] [review], except with 91 and 92? I'll take this.

Is it safe to check for 91 and 92 on all platforms, or do I need to #if it for windows?

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d44ec274fca0

Comment on attachment 509320 [details] [diff] [review] Patch v1 for Windows (In reply to comment #42) > Is it safe to check for 91 and 92 on all platforms, or do I need to #if it for > windows? It's at least safe in that 91 and 92 are not known to be mapped to any alphanumeric key on any of the three major platforms. I think this is fine as a workaround to unbreak Windows users for Firefox 4. We can fix the underlying platform code later. Since I don't think I can give official reviews for this module, a rubberstamp from another reviewer may be a good idea.

Yes, it's safe. Our DOM keycode values are based on virtual keycodes of Windows. Therefore, on non-Windows platform, we need to map all native keycodes to our keycodes. On the other hand, on Windows, we just set the native keycode to our keycode. Therefore, the DOM key event have the unexpected keycode which is 91 or 92.

> Therefore, the DOM key event have the unexpected keycode which is 91 or 92. I meant that the DOM key event can have the unexpected keycode *only on Windows*.

Comment on attachment 509320 [details] [diff] [review] Patch v1 for Windows r=me, thanks everyone for digging into this.

Pushed by sdwilsh. http://hg.mozilla.org/mozilla-central/rev/aebd2f355859

Comment on attachment 508651 [details] [diff] [review] Patch for widget v1.0 Let's attach any widget code fixes to a separate bug (which seems to be already filed from the list of bugs on which this "depends").

I can verify that this bug is fixed on Firefox 4 Beta 11 on Linux x86_64.

Multimedia keys still map to Panorama search. Please fix this as well.

(In reply to comment #21) > (In reply to comment #18) > > Key whitelists are much safer than key blacklists for this type of code. > > No, we should use blacklist because there are many keyboard layouts in the > world. (In reply to comment #53) > Multimedia keys still map to Panorama search. Please fix this as well. Exactly why I recommended a whitelist.

(In reply to comment #54) > (In reply to comment #53) > > Multimedia keys still map to Panorama search. Please fix this as well. > > Exactly why I recommended a whitelist. I guess that this is on Windows, right? If so, we can fix it completely on Fx4.next because such unexpected key will not be coming (always zero if we don't define the keycode in nsIDOMKeyEvent). So, I recommend that we shouldn't touch this problem now because the problem that user cannot start search by some printable key should be more important than the current problem that user starts search by unexpected key.

(In reply to comment #55) > (In reply to comment #54) > > (In reply to comment #53) > > > Multimedia keys still map to Panorama search. Please fix this as well. > > > > Exactly why I recommended a whitelist. > > I guess that this is on Windows, right? If so, we can fix it completely on > Fx4.next because such unexpected key will not be coming (always zero if we > don't define the keycode in nsIDOMKeyEvent). So, I recommend that we shouldn't > touch this problem now because the problem that user cannot start search by > some printable key should be more important than the current problem that user > starts search by unexpected key. I disagree. The functionality of starting a search is already accessible from the search button. It is more important to avoid false positives. We should not break OS-level functionality.

(In reply to comment #56) > (In reply to comment #55) > > (In reply to comment #54) > > > (In reply to comment #53) > > > > Multimedia keys still map to Panorama search. Please fix this as well. > > > > > > Exactly why I recommended a whitelist. > > > > I guess that this is on Windows, right? If so, we can fix it completely on > > Fx4.next because such unexpected key will not be coming (always zero if we > > don't define the keycode in nsIDOMKeyEvent). So, I recommend that we shouldn't > > touch this problem now because the problem that user cannot start search by > > some printable key should be more important than the current problem that user > > starts search by unexpected key. > > I disagree. The functionality of starting a search is already accessible from > the search button. It is more important to avoid false positives. We should not > break OS-level functionality. Wait, on my environment, I'm using Microsoft's keyboard with IntelliType, I cannot see any native key events for such special keys. I guess that the IntelliType eats the key event before we get the message. According to Windows' event model, it's too hard checking whether the application which was handling the message previously has consumed the message or not. Therefore, I think the IntelliType's approach is popular in keyboard utilities. On the other hand, Windows key is handled by explorer, therefore, I think the latest patch is valuable for now. sdrocking, I have a question, do we break the keyboard function by this bug? Or, the function still works but we also start the search? If the former, it might be better to be fixed on Fx4. Frank, if you want to fix this bug with whitelist, how to implement you that? And I might have realized an issue. Attachment 507209 [details] [diff] banned the events whose keycode are zero. I guess this causes non-ASCII character keyboard layout users of Linux cannot start search by character input. On Linux, Hebrew, Greek and Cyrillic keyboard layout generate DOM key events with 0 keycode if my memory is correct...