104371 - shiftKey not captured in KEYPRESS event

Status: RESOLVED FIXED

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

Description

[Chris Strolia-Davis]

The shift key is not being captured by the event object when the KEYPRESS event 
is fired.  Alt and Ctrl keys seem to be working fine.

Comment 1

[Chris Strolia-Davis]

Attachment: Test Case for "shiftKey" capture on KEYPRESS event

Comment 2

[Chris Strolia-Davis]

Just looked at explanations of Severities and changed severity to be more 
accurate.

Comment 3

[Boris Zbarsky [:bzbarsky]]

confirming on linux build 2002-01-18-08.

Comment 4

[Jess Thrysoee]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1a+) Gecko/20020704

Still not working. This breaks the Shift+Space scrollPageUp keybinding, which is
very annoying.

Comment 5

[Radha on family leave (not reading bugmail)]

Taking from joki

Comment 6

[David Hyatt]

Attachment: Remove this bogus clearing of the shiftkeydown bool.

Just before firing the key press, the SHIFT key down boolean is being cleared. 
This is incorrect.

Comment 7

[David Hyatt]

I assume a similar bug exists in the Linux widget code.

Comment 8

[David Hyatt]

Attachment: Fix Linux and Windows both.

Comment 9

[David Hyatt]

This fix seems to break SHIFT+space just being a space in editor.  That seems 
odd to me.  Will investigate further.

Comment 10

[David Hyatt]

Attachment: Fix Editor's listeners to not ignore charcode events with shift down

This should do it.  I need to figure out next if Mac has this same shift key
problem.  I assume all widget code implementations do, since editor wouldn't
have worked otherwise.

Comment 11

[David Hyatt]

Attachment: Patch Mac widget code as well (Cocoa and Carbon)

Comment 12

[Christopher Blizzard (:blizzard)]

I think you need to patch in widget/src/gtk/ too.

Comment 13

[David Hyatt]

Attachment: Patch that handles gtk, gtk2, cocoa, carbon, and win32

Comment 14

[Christopher Blizzard (:blizzard)]

Comment on attachment 103628 [details] [diff] [review]
Patch that handles gtk, gtk2, cocoa, carbon, and win32

r=blizzard on the gtk and gtk2 changes

Comment 15

[David Hyatt]

cc'ing brade to get editor review for the editor change.

Comment 16

[Kathleen :Brade]

Comment on attachment 103628 [details] [diff] [review]
Patch that handles gtk, gtk2, cocoa, carbon, and win32

r=brade

I think that this will be fine although Akkana and I have some concerns about
IME.  Please verify that IME is not broken.

Historically, what I recall about this code is that we needed a way to know
whether a key combination was intended to be typed into an editor or was some
type of shortcut key or whatever.  All widget code made a guess for that and
set all the modifier flags to false.  If there were no modifiers and there was
a keypress with a charCode, the editor should try to insert that.

Probably if someone (foolishly) set "shift" as their modifier key, they'd run
into some problems but I think those would pop up long before getting into this
code ;-)

Comment 17

[Kathleen :Brade]

oops! I also forgot to mention that you can remove these lines from the 2 editor
files:

521    if (NS_SUCCEEDED(aKeyEvent->GetKeyCode(&keyCode)) && 
522 -      NS_SUCCEEDED(aKeyEvent->GetShiftKey(&isShift)) &&
523        NS_SUCCEEDED(aKeyEvent->GetCtrlKey(&ctrlKey)) &&


no point getting "isShift" if it isn't used anymore, it's just slowing us down ;-)

Comment 18

[Blake Ross]

sr=me

Comment 19

[David Hyatt]

Fixed.

Comment 20

[Mike Kaply [:mkaply]]

Any idea what to look for in the code to see if OS/2 has this problem?

Comment 21

[:deanis74]

mkaply: A guess...

http://lxr.mozilla.org/mozilla/source/widget/src/os2/nsWindow.cpp#2138

  else if (usVKey == VK_SPACE)
  {
    event.isShift = PR_FALSE;
  }

Comment 22

[Boris Zbarsky [:bzbarsky]]

might want to ping drivers too.... ;)

Comment 23

[Aleksey Nogin]

Somehow the 
    NS_SUCCEEDED(aKeyEvent->GetShiftKey(&isShift))
line got erased from editor/libeditor/html/nsHTMLEditor.cpp in this commit
(although it is not removed in the attached patch).

Now brad TBox correctly states:

+editor/libeditor/html/nsHTMLEditor.cpp:1235
+ `PRBool isShift' might be used uninitialized in this function

since isShift is still used in several places, but is never set to anything!

Comment 24

[Christopher Blizzard (:blizzard)]

Uhh.  You checked this in without drivers approval?  Plus, it looks like there's
a warning there that needs to be fixed and os/2 might need updating?

Comment 25

[Christopher Blizzard (:blizzard)]

I backed this out.

Comment 26

[[e-mail]]

*** This bug should have been marked as a duplicate of bug 122017 ***

Comment 27

[David Hyatt]

Reassigning to default component owner of events.

Comment 28

[David Hyatt]

If someone wants to pick this up and run with it, I'd appreciate it.  I'm gonna
be too busy in the next few days to deal with this.

Comment 29

[Brian Ryner (not reading)]

I'll take it.

Comment 30

[Christopher Blizzard (:blizzard)]

The patch, as checked in, broke one hot key that I noticed - '*' in mail/news
which expands all threads.  At least, I'm pretty sure this was what broke it.

Comment 31

[Brian Ryner (not reading)]

Attachment: fix bugs with previous patch

Chagnes from the last patch:

- only remove isShift from nsPlaintextEditor; it's needed by nsHTMLEditor for
other purposes
- To fix key bindings such as "*", I made XBL not check for a matching shift
key state if the key was matched using the charcode, and shift was not
specified in the modifier mask.
- patched the rest of the widget toolkits

Comment 32

[David Hyatt]

In the XBL code, rather than passing in cAlt | cShift | cMeta, etc. I'd rather
you had a #define or a constant for  the full mask, e.g., cKeyMask and just
passed that in.

I think the code that ignores shift would be better served by setting a local
var to cKeyMask and then nulling SHIFT out rather than being set to a|b|c and
then adding in SHIFT conditionally.

With my suggested changes we could add new modifier keys in the future and not
have to change this code.

Comment 33

[Brian Ryner (not reading)]

Attachment: address hyatt's comments

Comment 34

[David Hyatt]

Comment on attachment 104070 [details] [diff] [review]
address hyatt's comments

sr=hyatt

Comment 35

[Kathleen :Brade]

Comment on attachment 104070 [details] [diff] [review]
address hyatt's comments

r=brade for the editor and Macintosh changes

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104070 [details] [diff] [review]
address hyatt's comments

r=bzbarsky on the xbl changes and widget changes

Comment 37

[[e-mail]]

Comment on attachment 104070 [details] [diff] [review]
address hyatt's comments

Wouldn't it be easier to reverse the mask?
>PRInt32 modKeys = cAllModifiers;
PRInt32 modIgnore = 0;
>modKeys &= ~cShift;
modIgnore = cShift;
>PRBool result = ModifiersMatchMask(aKeyEvent, modKeys);
PRBool result = ModifiersMatchMask(aKeyEvent, modIgnore);
>PRBool result = ModifiersMatchMask(aMouseEvent, cAllModifiers);
PRBool result = ModifiersMatchMask(aMouseEvent, 0);
>PRInt32 aModifiersMask)
PRInt32 aIgnoreMask)
>if (aModifiersMask & cMeta) {
if (!(aIgnoreMask & cMeta)) {
>if (aModifiersMask & cShift) {
if (!(aIgnoreMask & cShift)) {
>if (aModifiersMask & cAlt) {
if (!(aIgnoreMask & cAlt)) {
>if (aModifiersMask & cControl) {
if (!(aIgnoreMask & cControl)) {

Comment 38

[[e-mail]]

Comment on attachment 104070 [details] [diff] [review]
address hyatt's comments

> nsXBLPrototypeHandler::ModifiersMatchMask(nsIDOMUIEvent* aEvent, PRInt32 aModifiersMask)
> {
>   nsCOMPtr<nsIDOMKeyEvent> key(do_QueryInterface(aEvent));
>   nsCOMPtr<nsIDOMMouseEvent> mouse(do_QueryInterface(aEvent));
To me this just cries out for an extra interface nsIDOMUIEventModifiers (or
whatever) that both nsIDOMKeyEvent and nsIDOMMouseEvent implement:
nsXBLPrototypeHandler::ModifiersMatchMask(nsIDOMUIEventModifiers*
aEventModifiers, PRInt32 aModifiersMask)

Comment 39

[Asa Dotzler [:asa]]

drivers are concerned about the possibilities of regressions here. Please hold
off on landing this till we open for 1.3a (real soon now). Thanks

Comment 40

[Brian Ryner (not reading)]

checked in on the trunk.

Comment 41

[[e-mail]]

*** Bug 79047 has been marked as a duplicate of this bug. ***