11983 - [BLOCKER] editor shortcut keys get dispatched twice

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[buster]

in M9 candidate build, the editor has both a set of XUL keybindings and an
nsIDOMKeyListener.  Each dispatches editor commands, with the intent that the
XUL keybindings get first crack at a key event and if they trigger an action,
then the editor's own event listener never gets notification of the key event.
This is not happening.  XUL is firing the action, but not consuming the event.
Kathy and I tried a series of experiments that convinced us of this.  Kathy can
provide the details.  I did my experiments on Windows only.  Kathy can provide
details about other platforms.

Comment 1

[buster]

something has to be done for this for M9.  Worse case is to yank XUL keybindings
from the editor completely.

Comment 2

[Kathleen :Brade]

joki--I thought we had the events getting eaten but that doesn't seem to be the
case anymore.  Ideas?

Comment 3

[joki (gone)]

I believe that it does still work as far as DOM is concerned.  I would suspect
the problem is in one handling keydown while the other handles keypress.

Comment 4

[saari (gone)]

Wow, mid air collision, but I was about to say the same thing Tom. I thought akk
was going to remap the XUL to keydown?

Comment 5

[Kathleen :Brade]

The correct behavior we agreed to was to use keypress.
Steve and I tried moving the internal (non-XUL) keybindings to keypress but it
still seemed to be getting called (with both XUL and internal keybindings are on
keypress).

Comment 6

[saari (gone)]

Do you have an example binding that I can try?

Comment 7

[saari (gone)]

Ok, I just walked through the XUL keybinding code, and it definitely does return
the event status of consumeNoDefault, via that hack that joki has... should I be
setting the status in the event directly as well? (for some reason I thought
returning that BASE_ERROR thing was currently correct thing to do)

Comment 8

[saari (gone)]

I believe I am beginning to understand the problem. In nsRDFElement.cpp,
RDFElementImpl::HandleDOMEvent line 2347 there lies this comment :

    //Capturing stage
    // XXX Needs to be implemented.  Copy from nsGenericElement at some point.
    // Talk to [e-mail] for help.
    // For now at least handle document capture.

And after that, the event merrily goes on its way to be handled a second time.
There is the bug, CCing waterson. Joki, are we ready to do the right thing
instead of returning NS_BASE_ERROR?

Comment 9

[joki (gone)]

I think their may still be a more basic misunderstanding here.  Internally
we're using the consumeNoDefault flag.  Now ignore that for a second.  From the
DOM perspective, there are three things that can be done to an event during
processing.  One is cancelling the default action.  This is the equivalent of
setting the consume flag.  This does *not* prevent further processing.  It
simply prevents the default action of the system.  Now in most cases the system
would be the browser itself but since much of the browser is implemented inside
the content area this is a little confusing as the system is really now Gecko,
not the browser, and since Gecko doesn't generally do too much with the event
themselves, cancelling the default action won't buy you much.

Then there are two other methods, preventCapture and preventBubble.
These methods will, respectively, prevent further handling by lower Nodes in
the tree after capturing and prevent further handling by higher Nodes in the
tree during bubbling.  If you wish a lower or higher level Node to get an event
and then stop the event from going further you need to use these flags.  The
consumeNoDefault flag will only tell the system to stop processing the event
but it will still go give the event to all the other DOM handlers.

Comment 10

[joki (gone)]

By the way, if any of that was unclear please go read the DOM Level 2 Event
spec at:
http://www.w3.org/TR/WD-DOM-Level-2/events.html
Its real close to final so not much is gonna change.

Comment 11

[saari (gone)]

Thank you, much more clear now. So to properly "handle and consume" an event, I

need to preventCapture and preventBubble... I'll give that a shot.

Comment 12

[saari (gone)]

Mmmm, tasty, with no harmful side effects (at least in my tests). I'll toss
patches at a few people before checking in.

Comment 13

[saari (gone)]

Ok, I disabled keybinding on linux and win32 for M9, but I'm keeping the bug open

for the real fix in M10.



The real fix has a weird side effect on XPMenus that needs to be addressed before

I can check in the patch.

Comment 14

[Kathleen :Brade]

saari said he fixed this for M10 now so marking this as fixed.

Comment 15

[sujay]

Just adding verification steps for when I regress this one:

 to verify this one: check Control-B (for Bold) that it does make the text bold
but doesn't unbold it
too (applying it a second time).

Comment 16

[sujay]

seems to work fine now...verified in 9/2 build.