71760 - Move JS spacebar handler code into C++

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Christopher Blizzard (:blizzard)]

When embedding mozilla I get warnings when pressing the spacebar:

JavaScript error:
 line 1: document.commandDispatcher has no properties 

This is from the htmlBindings.xul fil in the keypress handler for the spacebar key.

Info from kin:

"A quick look in lxr leads me to believe that your little code snippet assumes
that 'document' is a XUL Document. You guys aren't using XUL in the embedding
world right?

It looks like an nsXULCommandDispatcher is automatically created in the Init()
method of all nsXULDocuments, which I believe is what you get when you do a
document.commandDispatcher ...  and nsXULCommandDispatcher defines methods for
getting/setting the focused element (document.commandDispatcher.focusedElement)
which is really a forwarding mechanism to the focus controller that it gets from
the nsPIDOMWindow interface."

Dave says that he can fix this.  Assigning to him.

Comment 1

[David Hyatt]

fixing summary.

Comment 2

[David Hyatt]

Ok, Arik, here's another fun one for you. :)

Comment 3

[Christopher Blizzard (:blizzard)]

What is going on with this bug?  I get a lot of complaints about this not
working for embedded folks.

Comment 4

[Christopher Blizzard (:blizzard)]

[23:11:03] <hyatt> blizzard, arik is working on the spacebar problem
[23:11:05] <hyatt> he's nearly done with it

Comment 5

[arik devens]

Attachment: first crack at a fix

Comment 6

[Christopher Blizzard (:blizzard)]

Attachment: remerged patch

Comment 7

[Christopher Blizzard (:blizzard)]

Is this not going into 0.9?  Is it not done?

Comment 8

[Brendan Eich [:brendan]]

I don't think interface-like methods should take nsAutoString:

+class nsAutoString;
 
 class nsXBLPrototypeHandler : public nsIXBLPrototypeHandler
 {
@@ -84,6 +85,7 @@
   
   inline PRInt32 GetMatchingKeyCode(const nsString& aKeyName);
   void ConstructMask();
+  void GetEventType(nsAutoString &type);

How about nsAWritableString or nsAString?  Cc'ing scc for advice.

/be

Comment 9

[arik devens]

attaching a new patch for the cpp file, i also changed the header file but don't
have the patch cause anon cvs is down

Comment 10

[arik devens]

Attachment: new patch that uses nsAWritableString instead of nsAutoString

Comment 11

[Christopher Blizzard (:blizzard)]

Attachment: complete patch that compiles

Comment 12

[Christopher Blizzard (:blizzard)]

The patch that I uploaded includes the changes to the header file, the xbl file
and uses controller->DoCommand(command) instead of
controller->DoCommand(command.Unicode()) since the latter didn't compile for me.

Comment 13

[Christopher Blizzard (:blizzard)]

Oh, what's so special about the "A" element in this patch?

Comment 14

[Brendan Eich [:brendan]]

Blizzard, your &-as-reference style is non-conforming! Red alert!

/be

Comment 15

[Brendan Eich [:brendan]]

Is there no VK_SPACE code to use instead of magic (but well known to old ASCII
dogs) 32?

Is the "A" focused element special case because you want space on an anchor to
load the link?  A more detailed comment would help.

+void
+nsXBLPrototypeHandler::GetEventType(nsAWritableString &type)
+{
+  mHandlerElement->GetAttribute(kNameSpaceID_None, kTypeAtom, type);
+  
+  if (type.IsEmpty()) {
+    // If we're a XUL key element, let's assume that we're "keypress".
+    nsCOMPtr<nsIAtom> tag;
+    mHandlerElement->GetTag(*getter_AddRefs(tag));
+    if (tag.get() == kKeyAtom)
+      type = NS_LITERAL_STRING("keypress");
+    else return;
+  }
 }
 
looks funky -- why bother with 'else return;' given the implicit return at the
end, one or two lines later?

/be

Comment 16

[arik devens]

Attachment: new patch with brendan's suggestions

Comment 17

[Brendan Eich [:brendan]]

r=[e-mail] -- hyatt should sr.  Important for 0.9?

/be

Comment 18

[Christopher Blizzard (:blizzard)]

In my mind it's important for 0.9 for some of my "customers."

Comment 19

[David Hyatt]

[s]r=hyatt

Comment 20

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9

Comment 21

[Christopher Blizzard (:blizzard)]

Can I check this into the 0.9 branch?

Comment 22

[arik devens]

landed for 0.9, will add to the trunk when the tree opens again.

Comment 23

[arik devens]

landed on trunk.

Comment 24

[Brendan Eich [:brendan]]

My review asked why 32 was used instead of the manifest constant, and the patch
responding to my review didn't fix that.  Arik, how about using the same SPACE
constant (nsIDOMKeyEvent::DOM_VK_SPACE, IIRC) that's used elsewhere in the very
same nsXBLPrototypeHandler.cpp?

Also, thinking more, could the C++ code's test for tag != "A" be less than the
same as the old JS keypress handler's tests for every other kind of focusable
element?  What about AREA tags?

/be

Comment 25

[Boris Zbarsky [:bzbarsky]]

reopening based on the comments in bug 53896.  This was not checked into trunk
correctly...

Comment 26

[arik devens]

ok, i've checking in the code now. should work, sorry to all.

i'm gonna leave the bug reopened until i get someone besides me to test it
though cause it worked for me last time ;-)

Comment 27

[Brendan Eich [:brendan]]

dammit, why not nsIDOMKeyEvent::DOM_VK_SPACE instead of 32, already?

/be

Comment 28

[arik devens]

brendan,

sorry, not trying to piss anyone off, meant to mention this in my last bug, i
wasn't going to add that till someone told me the code worked cause i didn't
trust my testing. i'll add it now.

Comment 29

[arik devens]

well, i'll add it once i can compile mozilla succesfully and test it ;-)

Comment 30

[arik devens]

commited nsIDOMKeyEvent::DOM_VK_SPACE change.

as far as i can tell though spacebar never made the buttons or checkbox's do
anything so i don't think i am actually a regression on that.

Comment 31

[Blake Ross]

Yes, spacebar used to submit buttons and toggle checkboxes.  See 53896. 
Anyways, closing this now.

Comment 32

[Boris Zbarsky [:bzbarsky]]

bug 79483 covers the regression