50252 - some control key sequences don't generate the correct event

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Jean-Francois Ducarroz]

On linux, Ctrl+Return is seen as a Ctrl+M, Ctrl+= doesn't work correctly either. 
This is needed for Message compose to map Ctrl+Return or Ctrl+Enter to Send 
Message (see bug 20336).

I wrote a sample XUL file to test CTRL+Return/Enter, I'll attach it to this bug.

Windows has a similary problem which I will log another bug for.

Comment 1

[Jean-Francois Ducarroz]

Attachment: Sample test file

Comment 2

[Stuart Parmenter]

this may be the same bug as 46992.  I know when the event comes in in
nsGtkEventHandler.cpp, that the info is all seperate...  the mask for the
modifer keys is set to control and the key is backspace/enter/whatever.  Are we
doing something at this point that would cause us to send the ascii code which
might be the same for a few different keys instead of them as an individual key?

Comment 3

[Jean-Francois Ducarroz]

Windows problem has been logged as bug 50255

Comment 4

[Akkana Peck]

Right, I suspect these are the same issue.  I don't know why we'd be remapping
the key: the keycode table in nsGtkEventHandler.cpp maps GDK_Return to
NS_VK_Return, so if it's coming in as GDK_return I'm not sure where it would be
getting changed to M.

Comment 5

[rubydoo123]

this is a hard call, but am marking this future. There is a workaround for this
issue as ugly as it may seem.

Setting to future and adding helpwanted

Comment 6

[Jean-Francois Ducarroz]

What's the work around?

Comment 7

[:deanis74]

What is the Windows bug #?  If it's the same problem, maybe I can track it down 
on Win.

Comment 8

[Jean-Francois Ducarroz]

it's bug 50255 and I have attached a patch.

Comment 9

[Daniel Veditz [:dveditz]]

Appealing the nsbeta3- decision. This is a near-dogfood issue for me, the mouse 
is killing my wrists. This is the main reason I still primarily use 4.x (I 
know, shame on me).

Comment 10

[Daniel Veditz [:dveditz]]

Putting back the minus I appealed

Comment 11

[Akkana Peck]

Jag and I talked on IRC and this turned out to be fairly straightforward.  gtk
is doing funky stuff with control characters, but it's easy to work around that
in the code.  We should really fix this, since other bugs depend on it.

Comment 12

[Peter "jag" Annema]

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

Comment 13

[Akkana Peck]

Fix in hand.  Should we consider this for beta3 after all?  It's very safe.

Comment 14

[Akkana Peck]

Attachment: Fix

Comment 15

[Dan Rosen]

This patch also fixes bug 52217. I &heart; akkana :)

Comment 16

[Dan Rosen]

btw, r=dr if i'm actually qualified to do that sort of thing...

Comment 17

[Akkana Peck]

Thanks for the review!  Removing nsbeta3- for reconsideration: this would solve
some problems in checked-in code (e.g. the incorrect bindings for enlarge/reduce
text size) and since the fix is safe, we should consider taking it for beta3.

Comment 18

[Jean-Francois Ducarroz]

PDT, if you are accepting to nominate this bug, should also considere accepting it's windows version, bug 50255

Comment 19

[rubydoo123]

Akkana, check it in when the tree is open

this is a correctness and functionality issue -- p2

Comment 20

[Akkana Peck]

Checked in.  Enjoy!

Comment 21

[Dan Rosen]

i'm not sure what happened, since it looks like you checked in exactly the fix
you posted, but it doesn't work for me anymore... ctrl+1, 0, 9, =, -, and some
others work, but ctrl+2 through 8, \, and others don't.

(output from nsGtkEventHandler lines 406-418 confirm this).

the patch /did/ work for me yesterday, so i don't know what's up.

Comment 22

[Dan Rosen]

s/yesterday/friday (2000-09-15)

Comment 23

[Dan Rosen]

sorry for the spam -- here's the relevant debug output, compared with expected
results:

Key Press event: gtk string = '1', keyCode = 0x0, char code = '1' [ctrl]
  dr: correct
Key Press event: gtk string = '', keyCode = 0x32, char code = '' [ctrl]
  dr: should be 2
Key Press event: gtk string = ', keyCode = 0x0, char code = '{' [ctrl]
  dr: should be 3
Key Press event: gtk string = '', keyCode = 0x0, char code = '|' [ctrl]
  dr: should be 4
Key Press event: gtk string = '', keyCode = 0x0, char code = '}' [ctrl]
  dr: should be 5
Key Press event: gtk string = '', keyCode = 0x0, char code = '~' [ctrl]
  dr: should be 6
Key Press event: gtk string = '', keyCode = 0x0, char code = '' [ctrl]
  dr: should be 7
Key Press event: gtk string = '', keyCode = 0x0, char code = 'ß' [ctrl]
  dr: should be 8
Key Press event: gtk string = '9', keyCode = 0x0, char code = '9' [ctrl]
  dr: correct
Key Press event: gtk string = '0', keyCode = 0x0, char code = '0' [ctrl]
  dr: correct
Key Press event: gtk string = '-', keyCode = 0x0, char code = '-' [ctrl]
  dr: correct
Key Press event: gtk string = '=', keyCode = 0x0, char code = '=' [ctrl]
  dr: correct
Key Press event: gtk string = ', keyCode = 0x0, char code = '{' [ctrl]
  dr: should be [
Key Press event: gtk string = '', keyCode = 0x0, char code = '}' [ctrl]
  dr: should be ]
Key Press event: gtk string = '', keyCode = 0x0, char code = '|' [ctrl]
  dr: should be \

removing "REVIEWED" indicator for "fix in hand" -- i'm sure we're really close,
though... grr...

Comment 24

[Akkana Peck]

Turns out that gtk sends weird and wonky things for the string (and length is
sometimes 0) on number keys when control is pressed, and for some other keys as
well.  But it also has another value in the struct, keyval, which is reliable,
and using keyval simplifies the code greatly.  Attaching another patch, which
Pavlov has reviewed; just need to get another reviewer.

Comment 25

[Akkana Peck]

Attachment: better fix, using keyval

Comment 26

[Akkana Peck]

Attachment: Patch that includes the fix for 43433

Comment 27

[Akkana Peck]

I just attached another version of the patch, which includes the changes Ann
Adamcik contributed for the meta key in bug 43433.  They look fine to me, though
I can't test them because gtk doesn't obey the meta key on linux (it's a
solaris-only fix, for now).  Can I get a review for that as well?

Comment 28

[Akkana Peck]

Never mind about that last patch (the one that includes 43433) -- turns out that
doesn't work on solaris either.  So the "better fix, using keyval" patch is the
one I'm hoping to get reviewed.

Comment 29

[Akkana Peck]

Fixed.  Now we should handle alphabetics, all the numerics, and even
nonprintables such as ctrl-return and ctrl-backspace (for which we return the
keycode, not the charcode).

Comment 30

[Dan Rosen]

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

Comment 31

[Akkana Peck]

Sorry for the spam!  But apparently all these closed bugs need to have their
target milestones changed since M19 and M20 are going away.  Since they're
allready closed, I'm choosing M18.

Comment 32

[blanca]

Ctrl-M, Ctrl-+=, and space work with 20001101 build
Ctrl-Enter & Ctrl+shift+M does not do anything

Comment 33

[timeless]

hrm, could someone add ctrl-alt-f and ctrl-alt-t to the list of testcases. I 
need them for bugs, and last i checked they didn't work.

Comment 34

[Jean-Francois Ducarroz]

it's probably similar to Windows which doesn't generate any WM_CHAR or WM_SYSCHAR event for CTRL+ALT+nn 
sequence! 

Comment 35

[timeless]

yes, i'm (experiencing my) developing pains on windows.