349644 - implement accessible objects for xforms date input controls

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

xforms input control that is bound to instance node of xsd:date type is represented by certain widget. It has input field and dropmarker for calendar showing. What special should accessible object have to provide some info about dropmarker and popped up calendar?

Comment 1

[Aaron Leventhal]

Do any Microsoft apps such as outlook have such a control? It's often good to check those out with MSAA SDK tools.

May we see a testcase?

Comment 2

[alexander :surkov (:asurkov)]

Attachment: testcase

(In reply to comment #1)
> Do any Microsoft apps such as outlook have such a control? It's often good to
> check those out with MSAA SDK tools.

Microsoft has ActiveX object for calendar. I can see datepicker in "Adress Book" aplication ("properties" dialog of contact, tab "personal").

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[Aaron Leventhal]

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

Hello, if you run this with Window-Eyes (and turn off browse mode using Ctrl+Shift+A) it just says "cell" "cell" as you arrow around. It needs to say the day of the month, and the weekday, to be useful.

For the problem of saying the numeric day, you just need to make the accessible name of each cell something like "1".

To fix it so the day of the week is spoken, you need to make the "Mon", "Tue", "Wed" etc. into ROLE_COLUMNHEADER objects, and have their name be the appropriate string. You also need to make sure each cell exposes it's current coordinates via GetBounds(), otherwise the screen reader heuristic can't associate the column header with the cell.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 246258 [details] [diff] [review] [edit])
> Hello, if you run this with Window-Eyes (and turn off browse mode using
> Ctrl+Shift+A) it just says "cell" "cell" as you arrow around. It needs to say
> the day of the month, and the weekday, to be useful.
> 
> For the problem of saying the numeric day, you just need to make the accessible
> name of each cell something like "1".

Every cell (html:td) contains accessible text node. Why that text node is not spoken? Does MSAA removes that textnode? Or?

> To fix it so the day of the week is spoken, you need to make the "Mon", "Tue",
> "Wed" etc. into ROLE_COLUMNHEADER objects, and have their name be the
> appropriate string.
> 

html:th is used for days of the week. html:th has role ROLE_CELL. Who should html:th or "my days of week" cell has role ROLE_COLUMNHEADER?

Also, html:th has accessible text node too.

> You also need to make sure each cell exposes it's current
> coordinates via GetBounds(), otherwise the screen reader heuristic can't
> associate the column header with the cell.

I guess html:table and its children should have this magic, right?

Comment 6

[Aaron Leventhal]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 246258 [details] [diff] [review] [edit] [edit])
> > Hello, if you run this with Window-Eyes (and turn off browse mode using
> > Ctrl+Shift+A) it just says "cell" "cell" as you arrow around. It needs to say
> > the day of the month, and the weekday, to be useful.
> > 
> > For the problem of saying the numeric day, you just need to make the accessible
> > name of each cell something like "1".
The contents of something aren't automatically spoken, because it could be too verbose. The screen reader relies on the accessible name for an object. Luckily, there is an easy way to make the accessible name equal to the contents of the cell.
Use xhtml:role="wairole:grid" for each in your grid, so that the name will automatically be calculated for you. If you use that, you should also use 
"wairole:grid" and "wairole:columnheader" in the right places.


> 
> Every cell (html:td) contains accessible text node. Why that text node is not
> spoken? Does MSAA removes that textnode? Or?
> 
> > To fix it so the day of the week is spoken, you need to make the "Mon", "Tue",
> > "Wed" etc. into ROLE_COLUMNHEADER objects, and have their name be the
> > appropriate string.
> > 
> 
> html:th is used for days of the week. html:th has role ROLE_CELL. Who should
> html:th or "my days of week" cell has role ROLE_COLUMNHEADER?
> 
> Also, html:th has accessible text node too.
> 
> > You also need to make sure each cell exposes it's current
> > coordinates via GetBounds(), otherwise the screen reader heuristic can't
> > associate the column header with the cell.
> 
> I guess html:table and its children should have this magic, right?
> 

Comment 7

[Aaron Leventhal]

If you use the roles I mentioned above, it should fix the accessible name for the column headers as well.

As far as bounds, yes -- bounds should work right now in a normal HTML table.

We can deal with the bounds issues in a separate bug, I'm not sure what the problem is. It could be a more general regression on trunk that I'm not aware of.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[Aaron Leventhal]

I don't see an r= requested, not sure if that's intentional.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 247076 [details] [diff] [review]
patch2

(In reply to comment #9)
> I don't see an r= requested, not sure if that's intentional.
> 

Right, sorry, I forgot.

Comment 11

[Aaron Leventhal]

Working better -- it's reading the cell text as I arrow around.

Unfortunately, it's still not reading the column headers. I'm having a hard time debugging to see if the ROLE_TABLE and ROLE_COLUMNHEADER roles are being used. Can you tell me a line of code I can temporarily modify to prevent the calendar popup from being dismissed when I switch apps?

Comment 12

[alexander :surkov (:asurkov)]

Attachment: calendar

(In reply to comment #11)
> Working better -- it's reading the cell text as I arrow around.
> 
> Unfortunately, it's still not reading the column headers. I'm having a hard
> time debugging to see if the ROLE_TABLE and ROLE_COLUMNHEADER roles are being
> used. Can you tell me a line of code I can temporarily modify to prevent the
> calendar popup from being dismissed when I switch apps?
> 

I hope you can just use this testcase instead. If no please let me know.

Comment 13

[Aaron Leventhal]

Comment on attachment 247076 [details] [diff] [review]
patch2

Everything looks good when I traverse the hierarchy from the cell children that fire focus events. I see the role_columnheader objects, the role_table, etc.

However, when I try to traverse from the top of the document I can't find the table descendant at all. That's why GetChildAtPoint() isn't working. In other words, there is no problem with GetBounds(), but because the children aren't "attached" (traversable from the parent), several things are breaking.

We need to fix that. I should be able to use MSAA Inspect and find a table cell by hovering the mouse over it.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch3

(In reply to comment #13)

> However, when I try to traverse from the top of the document I can't find the
> table descendant at all. That's why GetChildAtPoint() isn't working.

Now calendar widget has right number of child accessible. The problem was treewalker doesn't traverse anonymous nodes if bound node is html element.

Comment 15

[Aaron Leventhal]

Great! That works now, and as I arrow left and right, it says "Wednesday 13" instead of just "13". Awesome.

Now to review the code itself :)

Comment 16

[Aaron Leventhal]

Comment on attachment 247183 [details] [diff] [review]
patch3

Looks very good. Please take a look at these comments and decide how to address them.

nsAccessibilityService:
-  if (!content->IsNodeOfType(nsINode::eHTML)) {
I don't think we need to try the QI for most HTML nodes. Since 90% of the nodes we check are still HTML, this line is still good for performance. How about either adding an nsINode::eXForms or using 
 if (content->GetNameSpaceID()  != kNameSpaceID_HTML) {
I think it's mostly the QI's that are potentially slow. But, we could cache the namespace id and use that several times instead of refetching to test with SVG/MathML. That's probably my mistake, but we should fix it.

Same thing here:
-    if (mBindingManager && !parentContent->IsNodeOfType(nsINode::eHTML)) {
Is there any way we can keep the HTML optimization and use a nsamespace check instead? It's only an issue for XForms, right? HTML doesn't use XBL for anything, I don't think. But, I suppose if HTML namespaced elements are allowed to use XBL, then we should be safe and keep your change.

The calendar widget doesn't look like it ever has direct text children, so it could inherit from nsAccessible instead of nsHyperTextAccessible, right? It would never implement nsIAccessibleText, would it?

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> nsAccessibilityService:
> -  if (!content->IsNodeOfType(nsINode::eHTML)) {
> I don't think we need to try the QI for most HTML nodes. Since 90% of the nodes
> we check are still HTML, this line is still good for performance. How about
> either adding an nsINode::eXForms or using 
>  if (content->GetNameSpaceID()  != kNameSpaceID_HTML) {
> I think it's mostly the QI's that are potentially slow. But, we could cache the
> namespace id and use that several times instead of refetching to test with
> SVG/MathML. That's probably my mistake, but we should fix it.

> Same thing here:
> -    if (mBindingManager && !parentContent->IsNodeOfType(nsINode::eHTML)) {
> Is there any way we can keep the HTML optimization and use a nsamespace check
> instead? It's only an issue for XForms, right? HTML doesn't use XBL for
> anything, I don't think. But, I suppose if HTML namespaced elements are allowed
> to use XBL, then we should be safe and keep your change.

Right, 90% is HTML nodes and 99% of them hasn't XBL binding. But XForms hosted in XHTML document uses XHTML nodes to add XBL bindings like calendar widget or slider widget. 

> The calendar widget doesn't look like it ever has direct text children, so it
> could inherit from nsAccessible instead of nsHyperTextAccessible, right? It
> would never implement nsIAccessibleText, would it?
> 

Looks so, it shouldn't implement nsIAccessibleText I guess.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 247183 [details] [diff] [review]
patch3

The patch has few accessibility issues that should be fixed per AaronL's some comments. But I guess it shouldn't stop to review xforms changes.

AaronR, will you have time to do review before your vacation?

Comment 19

[aaronr]

(In reply to comment #18)
> (From update of attachment 247183 [details] [diff] [review] [edit])
> The patch has few accessibility issues that should be fixed per AaronL's some
> comments. But I guess it shouldn't stop to review xforms changes.
> 
> AaronR, will you have time to do review before your vacation?
> 

Yes, I will get it done before I leave.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch4

1) updated to trunk
2) calendar widget is inherited from nsAccessibleWrap
3) additional XBL actions for xhtml are saved til AaronL's further comments

Comment 21

[aaronr]

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

>Index: accessible/public/nsIAccessible.idl
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/public/nsIAccessible.idl,v
>retrieving revision 1.65
>diff -u -8 -p -r1.65 nsIAccessible.idl
>--- accessible/public/nsIAccessible.idl	1 Nov 2006 23:02:12 -0000	1.65
>+++ accessible/public/nsIAccessible.idl	6 Dec 2006 16:59:21 -0000
>@@ -482,17 +482,18 @@ interface nsIAccessible : nsISupports
>   const unsigned long ROLE_HEADING = 105;
>   const unsigned long ROLE_PAGE = 106;
>   const unsigned long ROLE_SECTION = 107;
>   const unsigned long ROLE_REDUNDANT_OBJECT = 108;
>   const unsigned long ROLE_FORM = 109;
>   const unsigned long ROLE_IME = 110;
>   const unsigned long ROLE_APP_ROOT = 111;
>   const unsigned long ROLE_PARENT_MENUITEM = 112;
>-  const unsigned long ROLE_LAST_ENTRY = 113; // Important -- helps ensure nsRoleMap's are synchronized
>+  const unsigned long ROLE_CALENDAR = 113;
>+  const unsigned long ROLE_LAST_ENTRY = 114; // Important -- helps ensure nsRoleMap's are synchronized
> 

nit: changing the interface, so you should change the uuid, too.

>Index: accessible/public/nsIAccessibleProvider.idl
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/public/nsIAccessibleProvider.idl,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 nsIAccessibleProvider.idl
>--- accessible/public/nsIAccessibleProvider.idl	6 Dec 2006 13:44:18 -0000	1.11
>+++ accessible/public/nsIAccessibleProvider.idl	6 Dec 2006 16:59:21 -0000
>@@ -103,36 +103,44 @@ interface nsIAccessibleProvider : nsISup
>   const long XULToolbarSeparator = 0x00001031;
>   const long XULTooltip = 0x00001032;
> 
> 
>   /**
>    * Constants set is used by XForms elements.
>    */
> 
>+   /** Used for xforms elements that provide accessible object for itself as
>+    * well for anonymous content. This property are used for upload,
>+    * input[type="xsd:gDay"] and input[type="xsd:gMonth"] */
>+   const long XFormsContainer = 0x00002000;
>+

nit: changing the interface so need to change the uuid, too.

Rest of the code looks ok to me.  With those, r=me

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> 
> nit: changing the interface so need to change the uuid, too.
> 
> Rest of the code looks ok to me.  With those, r=me
> 

Neil said once if I change constants of interface then I don't need to update uuid.

Comment 23

[[e-mail]]

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

I think in the case you're thinking of you were only adding a constant; when changing constants around I'd change the uuid too to be on the safe side.

Comment 24

[alexander :surkov (:asurkov)]

Attachment: for checkin

Comment 25

[Håkan Waara]

Checked in to trunk

Comment 26

[alexander :surkov (:asurkov)]

Setting 'xf-to-branch' whiteboard status since changes of xforms resources should go into 1.8 branch.

Comment 27

[aaronr]

checked into 1.8 branch on 2007-04-12 and checked into 1.8.0 branch on 2007-04-16 the xforms pieces, not the accessibility pieces