8123 - nsWindow::DispatchWindowEvent returns PR_TRUE even when the event wasn't handled

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[tague]

nsWindow::DispatchWindowEvent on Windows is returns PR_TRUE even when a keyboard
event wasn't handled.

I would expect that DispatchWindowEvent would return PR_FALSE when an event
wasn't handled.  Without this distinction, it is difficult to tell when to
propogate events to the DefWindowProc.

Comment 1

[karnaze (gone)]

Kevin, please reassign as appropriate.

Comment 2

[buster]

this was a bug in the editor event handler.  It is now fixed.  Grab the latest
nsEditorEventListeners.cpp.

Comment 3

[tague]

greg's changes reversed the problem.  now it's returning false, event when an
event is handled.  reopening and assigning to gre.

Comment 4

[buster]

All I did was make the editor event delegate return NS_OK if the event was not
handled, and NS_ERROR_BASE if it was, as per conversation with joki.  I didn't
touch nsWindow or any DOM event classes.  I do see this code, which hardcodes
the result for key down, is this what you mean?

BOOL nsWindow::OnKeyDown( UINT aVirtualKeyCode, UINT aScanCode)
{
  WORD asciiKey;

  asciiKey = 0;
  DispatchKeyEvent(NS_KEY_DOWN, asciiKey, aVirtualKeyCode); <--ret val ignored!

  // always let the def proc process a WM_KEYDOWN
  return FALSE;                                             <-- hardcoded ret
}

Comment 5

[joki (gone)]

Yeah, there are still a lot of spots in the widget code where return values
aren't propagated out.

Comment 6

[leger]

Moving all Widget Set bugs, past and present, to new HTML Form Controls
component per request from karnaze.  Widget Set component will be retired
shortly.

Comment 7

[buster]

Tom said he was looking into this, presumably with Tague.  CC'ing Rod.

Comment 8

[buster]

This bug has the effect of dispatching Control+X keystrokes twice (where
"X" is any character, not just 'x'.)  The first dispatch is as a KeyDown,
which we handle in the editor properly.  The second dispatch is as a
KeyPressed with some funky character, which we also handle "properly" as
if it were a real keystroke.  There's no way for us to know the
difference.
Marked as a blocker.  We won't be able to turn on GFX-rendered widgets with this
problem.  Since GFX-rendered widgets have no menu or toolbar UI, the only way
for the user to issue commands (copy, paste, undo, ...) to them is through
keyboard shortcuts.
Set target milestone to M8 per conversation last week with Tom.

Comment 9

[buster]

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

Comment 10

[Akkana Peck]

Changing platform to all.  Steve says this is also the reason that typing
spacebar in the editor causes a pagedown, which makes the editor virtually
unusable.

Comment 11

[buster]

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

Comment 12

[buster]

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

Comment 13

[joki (gone)]

Ok, well putting aside my current horrible way of marking events as consumed
(consumption by event handlers is currently indicated by returning something
*other than* NS_OK.  This is being fixed but not for M8.) I see three problems:

1.  The editor is not consuming events. There is a "return NS_ERROR_BASE" at the
bottom of the keypress handler, which is the correct (though hideous) way to the
do this.  But there are early returns which return ok so the event is not
consumed.  Some editor guy should look at this.

2. The widget code in nsWindow.cpp (at least on Windows) calls DispatchKeyEvent
to send the event out but then ignores the return value.  So the correct value
never makes it all the way back to the WindowProc.  But the correct value does
make it that far so it is propagating from the xp to the widget code.  Some
widget guy should look at this.

3. We're not all using the same events internally.  Tague has been making a big
push to get people using keypress instead of keydown to help i18n.  There are
still spots where we use the wrong one.  And I'm guilty of this one, too.  Most
notably the space bar scrolling is handled on a keydown but the editor handles
the later keypress.  These need to be the same.

As far as an uncancelled ctrl-foo keydown allowing a ctrl-foo keypress, I
thought that one of the points of tague's code was that he needed the keydown to
go through to ensure the proper i18n translation and that we're supposed to be
using the keypress anyway.  And for a regular ascii character (which I belive
we're using for accelerators) I'm not sure why it would be translated to
something else.  Am I missing something on that part?  I don't know where or why
we'd get a funky character.

Comment 14

[rods (gone)]

Extra Info:
Mac & Win both toolkits both generate a KeyDown, KeyPress, KeyUp. Win does this
because it actually gets a system event WM_CHAR that is used to generate the
KeyPress. The Mac synthezises the event.

There isn't anyway to tell the Windows OS that you have consumed the KeyDown and
that you don't wnat the WM_CHAR.

My take on this is the DOM needs to track this, it needs to keep track of the
Key processing return codes and then throw those subsequent key events away
if any one of them was consumed.

Comment 15

[joki (gone)]

That is what we have done in the past.  This time we were tryng to do all of
the synthesis of KEYPRESS events in the xp layer but based on Windows and the
way its i18n stuff works we need to deal with both KEYDOWN and CHAR events
there.

In 4.0 we had a hashtable that had to keep track of which keydowns had been
sent and which were cancelled and such.  Yuck.  It was ugly.  But I guess we
can try it again.  But if the DOM code needs to do it then we have to establish
which events are sent to Gecko and make sure all platforms behave the same.
And I guess that means that if Windows' i18n support forces us to use a certain
set we have to make the others follow suit.  I'd rather see us try to fix this
in the widget layer and just let the events that flow out be the spec'd DOM
events.

Comment 16

[joki (gone)]

Okay well one of the open parts of this bug was the handling of keydown when we
should be handling keypress, notable example being the spacebar scrolling which
causes scrolling upon spacing in the editor.  I've checked in a fix for that
bit.  But there are a number of additional pieces we need to address here.

Comment 17

[buster]

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

Comment 18

[joki (gone)]

Okay, after a discussion with rods we decided this is what we'll do for the
time being.  Rod is going to check in some fixes to that will ensure that the
return value for events is propgated through the widget library.  Keep in mind
that with the Windows behavior this does *not* mean that cancelling a keydown
will prevent the keypress.  Windows will send the onchar event regardless and
we don't keep state on a per-key basis yet.  This means that editor is still
going to get keypresses after keydowns.  For the moment the easiest solution
we have is for them to handle keypresses instead of downs for hotkeys.  For
standard US input we should just be getting Unicode char values from the
keypresses so that should work.

For M9, we can then look at getting the right sequence of keydown and
keypresses including cancellation of following events if necessary.  However,
the amount of work involved in this piece of work effectively prohibits this
from going into M8.  So I suggest we move this bug to M9.

Comment 19

[buster]

I'm happy to switch the editor's handling of control-keys to keypressed rather
than keydown if that's the right thing to do.  My first experiment trying to do
this was a total failure.  If you set a breakpoint in
nsTextEditorKeyListener::KeyPress, fire up the editor (apprunner -edit, or
viewer with tools menu item "editor mode"), and type ctrl-b, you'll see that the
event's keycode is 0 and its charcode is 2.  Neither of these values makes any
sense, so the control keys can't be handled here (unless a bug is fixed
elsewhere.)
What I can do is put checks around keyPressed to see if the control or alt key
are on, and if so ignore the event.  This seems hacky, but it gives us the
effect of ignoring the control key redispatch of keypressed.

Comment 20

[joki (gone)]

Okay, I suggest you go with that fix for now to get through M8.  Once you check
that in why don't you move this bug.  For the next milestone that leaves:

1. Get the proper symantics for keydown/keypress/keyup on all platforms.
2. Get the proper event values for all of these.  I had thought this was okay
but if that were true then for the ctrl-b case you mentioned you should get a
keypress with either a zero or VK_B value (we're still deciding) for the keycode
but you should definitely have a Unicode 'b' char in the charCode field.  I
don't know why you don't.

Comment 21

[buster]

editor workaround checked in.  control keys still handled on key-down, but
control keys are now ignored on keypressed.

Comment 22

[joki (gone)]

Okay.  I believe then this is as much of this as will be fixed for M8.  I'm
moving the milestone to M9.

Comment 23

[joki (gone)]

So I believe pieces of this have been fixed.  Is this still a blocker and if so
for whom?  I know we still have some issues with use of keypress vs keydown in
different situations.  Can anyone comment on this?  Looking to move to M10 and
remove blocker status.

Comment 24

[tague]

i don't think it's a blocker anymore - i'm able to do the stuff that i need to
do now.  since we are going to redo some of the keyboard event stuff at a later
point, i wouldn't worry about this one too much.

Comment 25

[chris hofmann]

joki at w3c

Comment 26

[joki (gone)]

The return value part of this was fixed a while back.  I left this open so that
I could fix the cancellation issue where cancelled keydowns could stop
keypresses.  We have decided not to add that functionality.  I'm marking this
fixed based on current status.

Comment 27

[rubydoo123]

based on input from joki and tague -- marking verified