Move JS spacebar handler code into C++

RESOLVED FIXED in mozilla0.9.1

Status

()

Core
XBL
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: blizzard, Assigned: arik devens)

Tracking

Trunk
mozilla0.9.1
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
fixing summary.
Status: NEW → ASSIGNED
Summary: warning from xbl js when embedding → Move JS spacebar handler code into C++
Target Milestone: --- → mozilla0.9

Comment 2

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

Assignee: hyatt → arik
Status: ASSIGNED → NEW
(Reporter)

Comment 3

17 years ago
What is going on with this bug?  I get a lot of complaints about this not
working for embedded folks.
(Reporter)

Comment 4

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

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

17 years ago
Created attachment 31039 [details] [diff] [review]
first crack at a fix
(Reporter)

Comment 6

17 years ago
Created attachment 31059 [details] [diff] [review]
remerged patch
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla0.9.1
(Reporter)

Comment 7

17 years ago
Is this not going into 0.9?  Is it not done?
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
(Assignee)

Comment 9

17 years ago
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
(Assignee)

Comment 10

17 years ago
Created attachment 31364 [details] [diff] [review]
new patch that uses nsAWritableString instead of nsAutoString
(Reporter)

Comment 11

17 years ago
Created attachment 31380 [details] [diff] [review]
complete patch that compiles
(Reporter)

Comment 12

17 years ago
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.
(Reporter)

Comment 13

17 years ago
Oh, what's so special about the "A" element in this patch?
Blizzard, your &-as-reference style is non-conforming! Red alert!

/be
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
(Assignee)

Comment 16

17 years ago
Created attachment 31655 [details] [diff] [review]
new patch with brendan's suggestions
r=brendan@mozilla.org -- hyatt should sr.  Important for 0.9?

/be
(Reporter)

Comment 18

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

Comment 19

17 years ago
[s]r=hyatt
(Reporter)

Comment 20

17 years ago
a=blizzard for 0.9
(Reporter)

Comment 21

17 years ago
Can I check this into the 0.9 branch?
(Assignee)

Comment 22

17 years ago
landed for 0.9, will add to the trunk when the tree opens again.
(Assignee)

Comment 23

17 years ago
landed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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
reopening based on the comments in bug 53896.  This was not checked into trunk
correctly...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 53896
(Assignee)

Comment 26

17 years ago
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 ;-)
dammit, why not nsIDOMKeyEvent::DOM_VK_SPACE instead of 32, already?

/be
(Assignee)

Comment 28

17 years ago
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.
(Assignee)

Comment 29

17 years ago
well, i'll add it once i can compile mozilla succesfully and test it ;-)
(Assignee)

Comment 30

17 years ago
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

17 years ago
Yes, spacebar used to submit buttons and toggle checkboxes.  See 53896. 
Anyways, closing this now.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
bug 79483 covers the regression
You need to log in before you can comment on or make changes to this bug.