Windows key should not be mapped to Panorama search

VERIFIED FIXED in Firefox 4.0b12

Status

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: fryn, Assigned: iangilman)

Tracking

Trunk
Firefox 4.0b12
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
OS: Windows 7 → All
Hardware: x86 → All
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED

Updated

8 years ago
Blocks: 627096
Priority: -- → P2
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
Posted patch patch v1 (obsolete) — Splinter Review
Pushed to try.
Attachment #506498 - Flags: review?(ian)
(Assignee)

Comment 3

8 years ago
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
Attachment #506498 - Flags: review?(ian) → review+
A keypress event is generated but with no valuable information so that charCode and keyCode is 0.
Attachment #506498 - Flags: approval2.0?
Attachment #506498 - Flags: approval2.0? → approval2.0+
blocking2.0: ? → final+
Whiteboard: [hardblocker]
(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 :)
(Reporter)

Comment 11

8 years ago
I'll push this later today.
Thanks for the patch, Tim. :)
(Reporter)

Comment 12

8 years ago
Pushed.

http://hg.mozilla.org/mozilla-central/rev/6a883344c0f0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
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.
(Reporter)

Comment 15

8 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 4.0b11 → ---
(Reporter)

Comment 16

8 years ago
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.
(Reporter)

Comment 18

8 years ago
Key whitelists are much safer than key blacklists for this type of code.
(Reporter)

Comment 19

8 years ago
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 :)
Assignee: tim.taubert → sdwilsh
Status: REOPENED → ASSIGNED
(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.
(Reporter)

Comment 24

8 years ago
> (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.
Whiteboard: [hardblocker] → [softblocker]
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.
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
(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.
Posted patch Patch for widget v1.0 (obsolete) — Splinter Review
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.
Attachment #508651 - Flags: review-
(Reporter)

Comment 37

8 years ago
(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).
(Reporter)

Updated

8 years ago
Attachment #507209 - Attachment description: patch for checkin → patch for checkin (checked in but only fixed Linux)
(Reporter)

Comment 39

8 years ago
(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.
(Assignee)

Comment 41

8 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Assignee: nobody → ian
(Assignee)

Comment 42

8 years ago
Is it safe to check for 91 and 92 on all platforms, or do I need to #if it for windows?
(Assignee)

Comment 43

8 years ago
Posted patch Patch v1 for Windows (obsolete) — Splinter Review
Attachment #509320 - Flags: review?(fryn)
(Reporter)

Comment 45

8 years ago
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.
Attachment #509320 - Flags: review?(fryn) → review+
(Assignee)

Updated

8 years ago
Attachment #509320 - Flags: review?(dietrich)
(Reporter)

Updated

8 years ago
Attachment #509320 - Flags: review+ → feedback+
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.
Attachment #509320 - Flags: review?(dietrich) → review+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 50

8 years ago
Pushed by sdwilsh.

http://hg.mozilla.org/mozilla-central/rev/aebd2f355859
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
(Reporter)

Comment 51

8 years ago
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").
Attachment #508651 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Attachment #509512 - Attachment description: hg export of patch v1 for windows for checkin → hg export of patch v1 for windows (checked in)
(Reporter)

Updated

8 years ago
Attachment #509320 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Flags: in-testsuite+

Comment 52

8 years ago
I can verify that this bug is fixed on Firefox 4 Beta 11 on Linux x86_64.
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
Multimedia keys still map to Panorama search. Please fix this as well.
(Reporter)

Comment 54

8 years ago
(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.
(Reporter)

Comment 56

8 years ago
(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...
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.