Note: There are a few cases of duplicates in user autocompletion which are being worked on.

implement accessible objects for xforms date input controls

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access, fixed1.8.0.12, fixed1.8.1.4})

Trunk
access, fixed1.8.0.12, fixed1.8.1.4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

1012 bytes, application/xhtml+xml
Details
1.07 KB, application/xhtml+xml
Details
78.32 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

11 years ago
Depends on: 350186
(Assignee)

Updated

11 years ago
No longer depends on: 337250

Comment 1

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

Comment 2

11 years ago
Created attachment 237411 [details]
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").
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Created attachment 246258 [details] [diff] [review]
patch
Attachment #246258 - Flags: review?(aaronleventhal)
(Assignee)

Updated

11 years ago
Blocks: 358714

Updated

11 years ago
Keywords: access

Comment 4

11 years ago
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.
Attachment #246258 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 5

11 years ago
(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

11 years ago
(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

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

Comment 8

11 years ago
Created attachment 247076 [details] [diff] [review]
patch2
Attachment #246258 - Attachment is obsolete: true

Comment 9

11 years ago
I don't see an r= requested, not sure if that's intentional.
(Assignee)

Comment 10

11 years ago
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.
Attachment #247076 - Flags: review?(aaronleventhal)

Comment 11

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

Comment 12

11 years ago
Created attachment 247132 [details]
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

11 years ago
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.
Attachment #247076 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 14

11 years ago
Created attachment 247183 [details] [diff] [review]
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.
Attachment #247076 - Attachment is obsolete: true
Attachment #247183 - Flags: review?(aaronleventhal)

Comment 15

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

11 years ago
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?
Attachment #247183 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 17

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

Comment 18

11 years ago
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?
Attachment #247183 - Flags: review?(aaronr)

Comment 19

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

Comment 20

11 years ago
Created attachment 247689 [details] [diff] [review]
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
Attachment #247183 - Attachment is obsolete: true
Attachment #247689 - Flags: review?(aaronr)
Attachment #247183 - Flags: review?(aaronr)

Comment 21

11 years ago
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
Attachment #247689 - Flags: review?(aaronr) → review+
(Assignee)

Comment 22

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

Updated

11 years ago
Attachment #247689 - Flags: superreview?(neil)

Comment 23

11 years ago
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.
Attachment #247689 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 24

11 years ago
Created attachment 248386 [details] [diff] [review]
for checkin
Attachment #247689 - Attachment is obsolete: true

Comment 25

11 years ago
Checked in to trunk
(Assignee)

Comment 26

11 years ago
Setting 'xf-to-branch' whiteboard status since changes of xforms resources should go into 1.8 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 27

10 years ago
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
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
You need to log in before you can comment on or make changes to this bug.