197152 - AccDoAction failed with menuitem that has command (not oncommand) attribute

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Kyle Yuan]

In the chrome, you can see many menuitems like this:
<menuitem label="&openCmd.label;" accesskey="&openCmd.accesskey;" 
key="openLocationKb" command="Browser:Open"/>

In this case, both DoCommand and Click will fail.

Comment 1

[Kyle Yuan]

Attachment: patch

also doing some replacement of |_retval|

Comment 2

[Kyle Yuan]

the essential part of this patch is here (most of them come from 
nsMenuFrame::Execute) :

-NS_IMETHODIMP nsXULMenuitemAccessible::AccDoAction(PRUint8 index)
+NS_IMETHODIMP nsXULMenuitemAccessible::AccDoAction(PRUint8 aIndex)
 {
-  if (index == eAction_Select) {   // default action
+  if (aIndex == eAction_Select) {   // default action
+    // See if we have a command elt.  If so, we execute on the command instead
+    // of on our content element.
+    nsAutoString command;
+    nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
+    NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);
+
+    element->GetAttribute(NS_LITERAL_STRING("command"), command);
+    if (!command.IsEmpty()) {
+      nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
+      nsCOMPtr<nsIDocument> doc;
+      content->GetDocument(*getter_AddRefs(doc));
+      nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(doc));
+      nsCOMPtr<nsIDOMElement> commandElt;
+      domDoc->GetElementById(command, getter_AddRefs(commandElt));
+      nsCOMPtr<nsIContent> commandContent(do_QueryInterface(commandElt));
+      if (commandContent) {
+        nsEventStatus status = nsEventStatus_eIgnore;
+        nsMouseEvent event;
+        event.eventStructType = NS_EVENT;
+        event.message = NS_XUL_COMMAND;
+
+        nsCOMPtr<nsIPresShell> presShell;
+        doc->GetShellAt(0, getter_AddRefs(presShell));
+        presShell->HandleDOMEventWithTarget(commandContent, &event, &status);
+      }
+      else {
+        NS_ASSERTION(PR_FALSE, "A XUL <menuitem> is attached to a command that 
doesn't exist! Unable to execute menu item!\n");
+      }
+    }
+    else {

Comment 3

[Aaron Leventhal]

Kyle, should this be done in javascript/xbl?
Perhaps DoCommand() should be fixed to do it's job correctly, no?

    if (selectItem)
      selectItem->DoCommand();

Comment 4

[Kyle Yuan]

But by now, each element is dealing with the command tag by itself, you can see 
that at:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsButtonBoxFrame.cpp
#187 and 
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#1720

If we put this piece of code into nsXULElement, it certainly will add extra 
load to the general xul elements.

hyatt, could you give us some advice?

Comment 5

[Aaron Leventhal]

I doubt we'll hear from hyatt - maybe bz, alecf or boris can give advice.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So what problem are we trying to solve, exactly?

Comment 7

[Aaron Leventhal]

Boris, we have a menu item that we want to execute. I was saying that
DoCommand() or something similar should work. Kyle wasn't sure whether the extra
bloat should be avoided in Gecko, and keep it in the accessibility libraries
where it won't normally get loaded.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, I see....  Let me dig through that code a bit....

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  So I don't really know this code well, but....

1)  It would make sense for doCommand to work for both oncommand and command
    attrs.
2)  We should not be duplicating identical command-dispatch code in all those
    frames -- move it to content.
3)  Mailing hyatt at [e-mail] may well get a response, esp. if someone he
    knows (alecf?  aaronl?  I can do it if you'd rather) mails him.

Does that help any?  ;)

Comment 10

[Kyle Yuan]

thanks, bz, that does make sense. I agree with you on the point 1) and 2), and
it will be greatly appreciated that you send a mail to get a response from hyatt.

Comment 11

[Aaron Leventhal]

Comment on attachment 117043 [details] [diff] [review]
patch

r=aaronl

If Hyatt agrees on a better way, then we will do it.

Comment 12

[Kyle Yuan]

Comment on attachment 117043 [details] [diff] [review]
patch

bz, if there is no objection from you and hyatt, could sr this patch? 
you only need to review the code I mentioned in comment 2, other part of the
patch is doing some arguments renaming.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Um.. I thought we were going to make doCommand handle this stuff?  And to
refactor the code in the frames to not have to do this by hand? (no response
from hyatt, btw, so I'm just winging it here...)

Comment 14

[Aaron Leventhal]

Let's do as bz suggests. We're not going to hear from hyatt on this.

Comment 15

[Kyle Yuan]

Comment on attachment 117043 [details] [diff] [review]
patch

ok, I can have a try this week.

Comment 16

[Kyle Yuan]

Attachment: move command tag support from individual frames to nsXULElement

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 118273 [details] [diff] [review]
move command tag support from individual frames to nsXULElement

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v
>retrieving revision 1.452
>diff -u -r1.452 nsXULElement.cpp
>--- content/xul/content/src/nsXULElement.cpp	6 Mar 2003 01:01:28 -0000	1.452
>+++ content/xul/content/src/nsXULElement.cpp	24 Mar 2003 08:19:13 -0000
>@@ -3145,6 +3145,25 @@
> 
>     nsIDOMEvent* domEvent = nsnull;
>     if (NS_EVENT_FLAG_INIT & aFlags) {
>+        if (aEvent->message == NS_XUL_COMMAND) {
>+            // See if we have a command elt.  If so, we execute on the command instead
>+            // of on our content element.
>+            nsAutoString command;
>+            GetAttribute(NS_LITERAL_STRING("command"), command);

You should use GetAttr(kNameSpaceID_None, nsXULAtoms::command, command)
instead.

>+                else {
>+                    NS_ASSERTION(PR_FALSE, "A XUL element is attached to a command that doesn't exist!\n");

I'd recommend changing that to NS_WARNING().

r=bryner with those changes.  Also, sr=jag since we both went over this.

Comment 18

[Kyle Yuan]

checked in!
changes made as per bryner's comment.