122017 - Shouldn't always unset shift when dispatching keypress event

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Blake Ross]

Rod, can you take a look at this patch?  Currently shift+space doesn't work
because of this; the shift gets ignored.  I think space is a special case
because shift+space doesn't produce a different character.  I couldn't think of
any similar cases to exclude.

Comment 1

[Blake Ross]

Attachment: patch

Comment 2

[:deanis74]

What about function keys?

Comment 3

[:deanis74]

OK, I ran through a bunch of keys using my testcase in bug 50255.  Shift doesn't
get unset for function keys.  The only other keys I could finder that it did get
unset for were /*-+ on the keypad.  Is fixing this just a hack for the problems
in 50255?

Comment 4

[Blake Ross]

No, it gets unset for almost every key in the keypress event, which is what
we're dealing with here -- the reason being, I think, that shift+char wasn't
intended as some kind of accelerator, but just to uppercase (or otherwise shift)
char.  So it'd be weird to overload shift by both shifting the character and
passing shift as a modifier.

As you said, shift doesn't get unset for function keys.  That's because OnChar
is never even called for the function keys, and the system doesn't even seem to
post WM_CHAR or WM_SYSCHAR when you press a function key. So I'm not sure where
those get handled, but they do.

I don't really think this specific fix is much of a hack.  It makes sense not to
unset shift for space so we can handle shift+space in the app (which is what
this patch is for).  Now in the context of this function in general, from what I
read in 50255, this whole thing is a hack ;)

Comment 5

[Blake Ross]

Got r=ben and sr=hewitt on this and checked it in.

Comment 6

[Blake Ross]

Hmm, I think I'm going to back this out.  With this fix, shift+space deosn't do
a space anymore sinec it's passed along as shift+space.  But without it, the app
can't handle shift+space. What is the answer here?

Comment 7

[:deanis74]

Did this get backed out?

Comment 8

[[e-mail]]

If only event="keypress" keycode="VK_SPACE" worked... :-(

Comment 9

[[e-mail]]

Forget that last comment, that conflicts with trapping space in checkboxes :-(

Hmmm... does that mean that even with a fix like this that checkboxes will have
to be fixed to work with the new shift+space as well as just space?

Comment 10

[[e-mail]]

*** Bug 104371 should have been marked as a duplicate of this bug ***