15307 - [PP] Mac: Key binding modifiers aren't respected on Mac

Status: VERIFIED FIXED

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

Description

[Jean-Francois Ducarroz]

Today with have some xul keyset to defined menu shortcut. if I have the following definition:

<key id="key_newMessage" command="true" key="M;" observes="cmd_newMessage"/>

If I type Command-m, newMessage is fired, Good. But it's also fired when I just type "m"

You can see the problem with the message compose window. Every time to type "m" in the subject or in any of the
recipient, a new compose window is created.

Comment 1

[Jean-Francois Ducarroz]

It's so bad that every time you type 'n' or 'm' into any text field in the browser (url field or forms field) window,
you get either a new browser ('n') or a new message compose ('m') window. For me it's a M11 blocker!

Comment 2

[saari (gone)]

The key event has garbage fields, specifically GetMetaKey returns garbage,
causing keybinds to inappropriately fire.

This is a bug in the Gfx text widget repropogation of the key event.

I'm reassigning to buster

Comment 3

[lchiang]

cc: paulmac.

Comment 4

[buster]

The fix in my code is trivial, but there's a larger issue:

I noticed this code in nsGUIEvent.h:

109 joki     3.13 struct nsInputEvent : public nsGUIEvent {
110 rods     3.8                  /// PR_TRUE indicates the shift key in down
111                   PRBool          isShift;
112                               /// PR_TRUE indicates the control key in down
113                   PRBool          isControl;
114                               /// PR_TRUE indicates the alt key in down
115                   PRBool          isAlt;
116 pierre   3.24 #ifdef XP_MAC
117                               /// PR_TRUE indicates the command key in down
118                               /// For now, it's only used in Widget: not for
export
119                               /// in nsIDOMEvent.h or nsJSEvent.cpp (later
maybe)
120                   PRBool          isCommand;
121               #endif
122 joki     3.13 };

The part that bothers me is the XP_MAC ifdef, since this is clearly XP code (and
this is why I missed setting the isCommand variable,
since I work on windows and didn't see it as an uninitialized variable in my
purify runs.  I don't see any reason why this should be #ifdef
platform code.  While the command is only settable on Mac, the extra few bytes
are irrelevant because events are transient objects.

In fact, I would make the 4 bools a single bit field,  make the bit field
protected, and add accessors and setters.

Who owns this code?

Comment 5

[buster]

reassigning to Kathy based on this information...

from Kipp:


Fix it dude :-)

Just make sure that if you init the bitfields, that you do something like this:

 class nsInputEvent : ... {
public:

    struct ModeBits {
      PRUint32 isShift : 1;
      ...;
    };

protected:
     union {
        ModeBits mode;
        PRUint32 all;
     };

That way in your ctor you can init all the bits at once and keep purify
happy...It happens to be more efficient too, which doesn't hurt.


from Kathy:

In fact, this code has been changed on the branch.  The ifdef has been removed.
Also, the field has been renamed to "isMeta" since that is
more consistent with across platforms and across components.

Comment 6

[leger]

this is a go for M10...chofmann just approved...can we get this into the branch
ASAP.  Please contact leaf.

Comment 7

[Jean-Francois Ducarroz]

Patch has been checked in the M10 branch last night. Now is just a M11 issue.

Comment 8

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 9

[Loco]

Updating QA Contact.

Comment 10

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 11

[Madhur Bhatia]

QA contact updated

Comment 12

[luis rincon]

verified on build 2001-08-07-08-trunk