358714 - implement accessible object for xforms minimal select in xhtml context

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Marking blockin from bug 358713. Some things needed for this bug will be implemented there. But minimal select should have some special code.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: inprocess

1) there are not yet accessible objects for popup and dropmarker. Waiting for the bug 349644.

Comment 2

[alexander :surkov (:asurkov)]

Also, select1 has wrong caching children and select1 doesn't generate some events needed for ally.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[Aaron Leventhal]

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

r= for mozilla/accessible piece.

Comment 5

[aaronr]

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

>Index: accessible/public/nsIAccessibleProvider.idl
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/public/nsIAccessibleProvider.idl,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 nsIAccessibleProvider.idl
>--- accessible/public/nsIAccessibleProvider.idl	28 Dec 2006 14:19:45 -0000	1.13
>+++ accessible/public/nsIAccessibleProvider.idl	3 Jan 2007 08:58:49 -0000
>@@ -103,56 +103,63 @@ interface nsIAccessibleProvider : nsISup

>+  /** Used for xforms select1 element that is represented by combobox */
>+  const long XFormsSelectCombobox = 0x00002014;
>+  /** Used for xforms item element that is used inside xforms select1
>+   * elements represented by combobox */
>+  const long XFormsItemCombobox = 0x00002015;

You are adding new contstants to the idl, so you should update the uuid, right?

>Index: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 nsXFormsFormControlsAccessible.cpp
>--- accessible/src/xforms/nsXFormsFormControlsAccessible.cpp	28 Dec 2006 14:19:47 -0000	1.7
>+++ accessible/src/xforms/nsXFormsFormControlsAccessible.cpp	3 Jan 2007 08:58:50 -0000

>+NS_IMETHODIMP
>+nsXFormsItemComboboxAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)
>+{
>+  if (aIndex != eAction_Click)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  nsAccessible::GetTranslatedString(NS_LITERAL_STRING("select"), aName);
>+  return NS_OK;
>+}
>+

I assume that you don't need to worry about a corresponding deselect?

>Index: accessible/src/xforms/nsXFormsWidgetsAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsWidgetsAccessible.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsWidgetsAccessible.cpp
>--- accessible/src/xforms/nsXFormsWidgetsAccessible.cpp	12 Dec 2006 16:19:16 -0000	1.1
>+++ accessible/src/xforms/nsXFormsWidgetsAccessible.cpp	3 Jan 2007 08:58:50 -0000

>+NS_IMETHODIMP
>+nsXFormsComboboxPopupWidgetAccessible::GetState(PRUint32 *aState)
>+{
>+  NS_ENSURE_ARG_POINTER(aState);
>+
>+  nsXFormsAccessible::GetState(aState);
>+
>+  PRBool isOpen = PR_FALSE;
>+  nsresult rv = sXFormsService->IsDropmarkerOpen(mDOMNode, &isOpen);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (isOpen)
>+    *aState = STATE_FLOATING | STATE_FOCUSABLE;
>+  else
>+    *aState = STATE_INVISIBLE | STATE_FOCUSABLE;

nit: I'd suggest assigning default states (like STATE_FOCUSABLE) to aState outside of this 'if (isOpen)' test like you do in other places above.  Then default states will be assigned before any tweaking is done.  Otherwise some time in the future someone might add another default state and not notice that STATE_FOCUSABLE is also a default state and then you'd have default states in two places in the same function.

>Index: accessible/src/xforms/nsXFormsWidgetsAccessible.h
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsWidgetsAccessible.h,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsWidgetsAccessible.h
>--- accessible/src/xforms/nsXFormsWidgetsAccessible.h	12 Dec 2006 16:19:16 -0000	1.1
>+++ accessible/src/xforms/nsXFormsWidgetsAccessible.h	3 Jan 2007 08:58:50 -0000
>@@ -69,9 +69,29 @@ public:
> class nsXFormsCalendarWidgetAccessible : public nsAccessibleWrap
> {
> public:
>   nsXFormsCalendarWidgetAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell);
> 
>   NS_IMETHOD GetRole(PRUint32 *aRole);
> };
> 
>+
>+/**
>+ * Accessible object for popup menu of minimal xforms select1 element that is
>+ * represented by combobox..

nit: get rid of a '.'

>Index: extensions/xforms/nsXFormsItemElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 nsXFormsItemElement.cpp
>--- extensions/xforms/nsXFormsItemElement.cpp	4 Dec 2006 14:55:25 -0000	1.18
>+++ extensions/xforms/nsXFormsItemElement.cpp	3 Jan 2007 08:58:50 -0000
>@@ -37,16 +37,19 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsIXFormsSelectChild.h"
> #include "nsXFormsStubElement.h"
> #include "nsIDOMHTMLOptionElement.h"
> #include "nsXFormsAtoms.h"
> #include "nsIDOMNodeList.h"
> #include "nsIDOMDocument.h"
>+#include "nsIDOMDocumentEvent.h"
>+#include "nsIDOMEvent.h"
>+#include "nsIPrivateDOMEvent.h"
> #include "nsString.h"
> #include "nsXFormsUtils.h"
> #include "nsIXFormsValueElement.h"
> #include "nsVoidArray.h"
> #include "nsIDOMText.h"
> #include "nsIXTFElementWrapper.h"
> #include "nsIXFormsContextControl.h"
> #include "nsIModelElementPrivate.h"
>@@ -392,16 +395,41 @@ nsXFormsItemElement::Refresh()
>     } while(current);
>   }
> }
> NS_IMETHODIMP
> nsXFormsItemElement::SetActive(PRBool aActive)
> {
>   /// @see comment in nsIXFormsItemElement.idl
> 
>+  if (aActive) {
>+    // Fire 'DOMMenuItemActive' event. This event is used by accessible module.
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDOMDocumentEvent> doc = do_QueryInterface(domDoc);
>+    NS_ENSURE_STATE(doc);
>+
>+    nsCOMPtr<nsIDOMEvent> event;
>+    doc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
>+
>+    event->InitEvent(NS_LITERAL_STRING("DOMMenuItemActive"),
>+                     PR_TRUE, PR_TRUE);
>+
>+    nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>+    privateEvent->SetTrusted(PR_TRUE);
>+

Maybe should use nsXFormsUtils::SetEventTrusted since that is what we do everywhere else in our code.

>Index: extensions/xforms/nsXFormsUtilityService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 nsXFormsUtilityService.cpp
>--- extensions/xforms/nsXFormsUtilityService.cpp	28 Dec 2006 14:19:47 -0000	1.16
>+++ extensions/xforms/nsXFormsUtilityService.cpp	3 Jan 2007 08:58:50 -0000

> nsresult
> nsXFormsUtilityService::GetSelectChildrenForNode(nsIDOMNode *aElement,
>                                                  nsNodeList *aNodeList)
> {
>   nsCOMPtr<nsIContent> contentElm(do_QueryInterface(aElement));
>   NS_ENSURE_TRUE(contentElm, NS_ERROR_FAILURE);
>-
>   nsCOMPtr<nsINodeInfo> nodeInfo(contentElm->NodeInfo());
>-  if (!nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS)))
>-    return NS_ERROR_FAILURE;
>-
>-  if (nodeInfo->Equals(nsXFormsAtoms::select) ||
>-      nodeInfo->Equals(nsXFormsAtoms::select1) ||
>-      nodeInfo->Equals(nsXFormsAtoms::choices)) {
>-    return GetSelectChildrenForNodeInternal(aElement, aNodeList);
>-  }
>-
>-  if (!nodeInfo->Equals(nsXFormsAtoms::itemset))
>-    return NS_ERROR_FAILURE;
> 
>-  nsCOMPtr<nsIXFormsItemSetUIElement> itemset(do_QueryInterface(aElement));
>-  NS_ENSURE_TRUE(itemset, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIDOMNode> container(aElement);
> 
>-  nsCOMPtr<nsIDOMElement> itemsetContent;
>-  itemset->GetAnonymousItemSetContent(getter_AddRefs(itemsetContent));
>-  NS_ENSURE_TRUE(itemsetContent, NS_ERROR_FAILURE);
>+  if (nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS)) &&
>+      nodeInfo->Equals(nsXFormsAtoms::itemset)) {
>+    nsCOMPtr<nsIXFormsItemSetUIElement> itemset(do_QueryInterface(aElement));
>+    NS_ENSURE_TRUE(itemset, NS_ERROR_FAILURE);
>+
>+    nsCOMPtr<nsIDOMElement> itemsetContent;
>+    itemset->GetAnonymousItemSetContent(getter_AddRefs(itemsetContent));
>+    container = do_QueryInterface(itemsetContent);
>+  }
> 
>-  return GetSelectChildrenForNodeInternal(itemsetContent, aNodeList);
>+  return GetSelectChildrenForNodeInternal(container, aNodeList);
> }

Well, it looks like you no longer test to ensure the xforms namespace and you used to check for the possibilities and return NS_ERROR_FAILURE if it wasn't one of the possibilities (select, select1, choices or itemset).  Aren't these safeguards still necessary?

And if the original safeguards are still necessary and you put them back in, I guess I don't understand why "choices" was an acceptable possibility.  I can see that select and select1 would be possibilities and that itemset would be a possibility due to the call back into GetSelectChildrenForNode from inside GetSelectChildrenForNodeInternal for itemsets.  But I don't see why xf:choices would ever be in aElement.

>Index: extensions/xforms/resources/content/select1.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select1.xml,v
>retrieving revision 1.30
>diff -u -8 -p -r1.30 select1.xml
>--- extensions/xforms/resources/content/select1.xml	12 Dec 2006 16:19:17 -0000	1.30
>+++ extensions/xforms/resources/content/select1.xml	3 Jan 2007 08:58:50 -0000
>@@ -47,23 +47,16 @@
> 
>   <!-- select1 -->
>   <binding id="xformswidget-select1"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> 
>     <!-- The strange indentation is because of the whitespace nodes.-->
>     <content>
>       <children includes="label|hint"/>
>-      <html:div class="-moz-xforms-select1-popup" anonid="popup"
>-                onmouseover="this.parentNode.shouldHandleBlur = false;
>-                             this.parentNode.mouseOver(event);"
>-                onmouseup="this.parentNode.mouseUp(event);"
>-                onmouseout="this.parentNode.shouldHandleBlur = true;">
>-        <children/>
>-      </html:div>
>       <html:span class="-moz-select1-container"
>                  anonid="container"><html:input
>                  class="-moz-xforms-select1-input xf-value"
>                  anonid="control"
>                  xbl:inherits="accesskey"
>                  onfocus="this.parentNode.parentNode.dispatchDOMUIEvent('DOMFocusIn')"
>                  onblur="this.parentNode.parentNode.handleBlur(); this.parentNode.parentNode.dispatchDOMUIEvent('DOMFocusOut');"
>                  onclick="this.parentNode.parentNode.handleControlClick();"
>@@ -72,19 +65,27 @@
>       /><html:input mozType:dropmarker="true"
>                     type="button"
>                     anonid="dropmarker"
>                     tabindex="-1"
>                     onmousedown="this.parentNode.parentNode.shouldHandleBlur = false;"
>                     onmouseup="this.parentNode.parentNode.shouldHandleBlur = true;"
>                     onclick="this.parentNode.parentNode.togglePopup();
>                              this.previousSibling.focus();"
>-        /></html:span></content>
>+        /></html:span>
>+      <html:div mozType:comboboxpopup="true" anonid="popup"
>+                onmouseover="this.parentNode.shouldHandleBlur = false;
>+                             this.parentNode.mouseOver(event);"
>+                onmouseup="this.parentNode.mouseUp(event);"
>+                onmouseout="this.parentNode.shouldHandleBlur = true;">
>+        <children/>
>+      </html:div>
>+    </content>

Any particular reason this div was moved?  I can't say that I really care that it was moved.  Just curious if it fixes/works around a problem or something like that.

Nothing really major.  r-'ing to get it out of my request queue.  Only semi-important question is whether you are going to leave in the tests inside GetSelectChildrenForNode and if xf:choices will still be one of the tests.  Rest are just nits.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> You are adding new contstants to the idl, so you should update the uuid, right?

Adding new constants doesn't require to change uuid.

> I assume that you don't need to worry about a corresponding deselect?

Yes, at least html analogue accessible object doesn't care about it. Also MSAA
defines only "select" action.

> Maybe should use nsXFormsUtils::SetEventTrusted since that is what we do
> everywhere else in our code.

Fine with me. Actually ally code doesn't care about if event is trusted or
not. Fixed.

> Well, it looks like you no longer test to ensure the xforms namespace and you
> used to check for the possibilities and return NS_ERROR_FAILURE if it wasn't
> one of the possibilities (select, select1, choices or itemset).  Aren't these
> safeguards still necessary?
> 
> And if the original safeguards are still necessary and you put them back in, I
> guess I don't understand why "choices" was an acceptable possibility.  I can
> see that select and select1 would be possibilities and that itemset would be a
> possibility due to the call back into GetSelectChildrenForNode from inside
> GetSelectChildrenForNodeInternal for itemsets.  But I don't see why xf:choices
> would ever be in aElement.

My bad. Safeguards are usefull still. Choices is represented in safeguards list
since I'd like to get accessible children for given node. For example, if given
node is select1 then the method doesn't return child nodes of choices.

> Any particular reason this div was moved?  I can't say that I really care that
> it was moved.  Just curious if it fixes/works around a problem or something
> like that.

I moved it there just because third accessible of html analogue accessible object
is popup listbox. I'm trying to be "compatible".

Comment 9

[aaronr]

(In reply to comment #8)

> My bad. Safeguards are usefull still. Choices is represented in safeguards list
> since I'd like to get accessible children for given node. For example, if given
> node is select1 then the method doesn't return child nodes of choices.
> 

From what I see, the only possible values for aElement coming into nsXFormsUtilityService::GetSelectChildrenFor are select and select1.  Is this correct?  If so, then the first time into GetSelectChildrenForNode, aElement will have to be select or select1.  Then it calls GetSelectChildrenForNodeInternal which will look for xf:items and xf:choices and add them to aNodeList.  If it finds a xf:itemset, it will call GetSelectChildrenForNode again, passing in the itemset as aElement.  So now the only possible aElements are select, select1 or itemset.

So I don't see how a xf:choices could possibly be passed into GetSelectChildrenForNode which means you don't need to test for it inside that function (though you'll probably still want to check for it in GetSelectChildrenForNodeInternal).  Or am I missing something?  Is it possible (or will it be possible in the future) to have aElement be a xf:choices for GetSelectChildrenForNode?

That is my only nit.  Rest of patch looks good.  If you agree with me that xf:choices can't possibly be passed into GetSelectChildrenForNode, then please remove that test (even though it really isn't part of this patch).  Otherwise leave the test in.  Either way, r=me.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> So I don't see how a xf:choices could possibly be passed into
> GetSelectChildrenForNode which means you don't need to test for it inside that
> function (though you'll probably still want to check for it in
> GetSelectChildrenForNodeInternal).  Or am I missing something?  Is it possible
> (or will it be possible in the future) to have aElement be a xf:choices for
> GetSelectChildrenForNode?

xf:choices can be passed there by accessible object for xf:choices element (http://lxr.mozilla.org/mozilla/source/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp#452)

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch4

I forgot to implement nsIAccessibleProvider for xforms:item and xforms:choices. Fixed.

Comment 12

[[e-mail]]

Comment on attachment 250966 [details] [diff] [review]
patch4

>+  nsCOMPtr<nsIContent> parent(content->GetBindingParent());\
>+  widget = do_QueryInterface(parent);\
This can actually be simplified to
  widget = do_QueryInterface(content->GetBindingParent());\

Comment 13

[alexander :surkov (:asurkov)]

Attachment: for checkin

needs to be checked in

Comment 14

[alexander :surkov (:asurkov)]

Attachment: fix for ? operator

Comment 15

[alexander :surkov (:asurkov)]

checked in by smaug

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

This caused the following error.
CSS Error (chrome://xforms/content/xforms.css :503.0): Found unclosed string '"minimal] choices,'.  Expected
                  identifier or string for value in attribute selector but found '"minimal] choices,'.  Ruleset ignored due to bad
                  selector.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: css fix

(In reply to comment #16)
> This caused the following error.
> CSS Error (chrome://xforms/content/xforms.css :503.0): Found unclosed string
> '"minimal] choices,'.  Expected
>                   identifier or string for value in attribute selector but
> found '"minimal] choices,'.  Ruleset ignored due to bad
>                   selector.
> 

Sorry, my bad.

Comment 18

[Boris Zbarsky [:bzbarsky]]

So I assume that all these descendant combinators are necessary?  That is, that you don't know where inside the <select1> the <choices> will be?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> So I assume that all these descendant combinators are necessary?  That is, that
> you don't know where inside the <select1> the <choices> will be?
> 

Choices can be child of select1, choices and IIRC itemset elements. For example, the structure can be:
<select1>
  <choices>
    <item/>
    <choices>
      <item/
    </choices>
  </choices>
</select1>

Comment 20

[Boris Zbarsky [:bzbarsky]]

So it really can be arbitrarily deeply nested?  Ok.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> So it really can be arbitrarily deeply nested?  Ok.
> 

Right. Formally Mozilla XForms doesn't apply restriction on depth of nested choices elements.

Comment 22

[alexander :surkov (:asurkov)]

'css fix' patch has been checked in by smaug.