365973 - can't expose <listbox> with multiple columns

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lynn Monsanto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20060601 Firefox/2.0.0.1 (Ubuntu-edgy)
Build Identifier: version 3 alpha 1 (20070104)

The Certificates table (Recipient, Status, Issued, Expires) in the Message Security dialog only contains Recipient information. There are no table entries for the other three columns.  AT-POKE shows that the table is modeled as a list with the table headers as labels, and the table cell as "list entries".

This problem means assistive technologies cannot speak the Certificates table.

Reproducible: Always

Steps to Reproduce:
1. Enable assistive technology on the desktop
2. Start Thunderbird and create a new message.
3. From the View menu, launch the Message Security dialog
4. Start at-poke and expand the Message Security dialog. You will see the Certificates list is malformed and/or incomplete.
Actual Results:  
See above

Expected Results:  
The Certificates table should be presented as an AccessibleTable.

Comment 1

[Evan Yan]

Actually, the table indeed is a <listbox>.
The problem is, <listbox> can have columns, how can we expose a <listbox> with multiple columns?

Aaron, any idea here?

Comment 2

[Aaron Leventhal]

Are there DOM Nodes for each item or is it more like a tree view with no indent level? Tree views use an API to get cell text at a row/column and doesn't have a separate DOM node for each item.

If it's very much like a tree view you could try to reuse a lot of the nsXULTreeAccessible code, and just change the role etc. based on the tag name. Or inherit from that class and override the necessary methods.

Comment 3

[Evan Yan]

I think there are DOM node for each item.
The structure of <listbox> with multiple columns is like
<listbox>
  <listcols>
    <listcol />
    ...
  </listcols>
  <listhead>
    <listheader />
    ...
  </listhead>
  <listitem>
    <listcell />
    ...
  </listitem>
  ...
</listbox>

Is it good for us to expose <listbox> with two possible roles, ROLE_LIST and ROLE_TREE_TABLE? There is only the number of listcol from which we can identify the role. The implementation may be complex and buggy.

How about we still expose it as ROLE_LIST, and fix GetName() to return all the text in a row?

Lynn, does that work for you?

Comment 4

[Lynn Monsanto]

Aaron, Your proposal is fine with one caveat: all columns in the row need to be returned, even empty listcells. Indicating an empty listcell with two double-quotes ("") would work.

Are there cases where a listcell could contain another component like an image. I don't know how getName() would handle this call.  Return the image description?

Comment 5

[Evan Yan]

(In reply to comment #4)
> Are there cases where a listcell could contain another component like an image.
> I don't know how getName() would handle this call.  Return the image
> description?
> 
<listcell> contains text only. see
http://xulplanet.com/references/elemref/ref_listcell.html

Comment 6

[Aaron Leventhal]

Please see bug 365973

We're going to have to redo how we do all tree views and grids to match that. It does allow arbitrary content in a cell.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Please see bug 365973

Would you say it is bug 367905?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (In reply to comment #4)
> > Are there cases where a listcell could contain another component like an image.
> > I don't know how getName() would handle this call.  Return the image
> > description?
> > 
> <listcell> contains text only. see
> http://xulplanet.com/references/elemref/ref_listcell.html
> 

Actually listcell may contain anything (see http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/listbox.xml#1005).

Comment 9

[Aaron Leventhal]

I'm not sure we will get to this for Firefox 3.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

Does it go with idea of 367905?

Comment 11

[Aaron Leventhal]

Surkov, we're not going to get to bug 367905 for Firefox 3.

Did you test this with any screen reader?

Comment 12

[Aaron Leventhal]

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

Clearing until I find out what testing was done with screen readers.

Comment 13

[Evan Yan]

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

clear review request till Aaron's question get answered.

Comment 14

[Aaron Leventhal]

Evan said he'd love to test this patch.  :-)

Comment 15

[alexander :surkov (:asurkov)]

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

so, if Evan would love to test it :)

Comment 16

[Evan Yan]

Sorkov, could you create a testcase and try it on your box please? I'll look for some time to test it on Linux tomorrow.

Comment 17

[Evan Yan]

some questions:

1) nsXULListboxAccessible implements nsIAccessibleTable, while we didn't expose its role as table. How AT could use it?

2) As comment #8, listcell may contains anything, shouldn't nsXULListCellAccessible implements HyperText?

3) With a simple <listbox> as follows,

  <listbox id="listbox" flex="1">
    <listcols>
      <listcol/>
      <listcol/>
    </listcols>
    <listhead>
      <listheader label="listhead1"/>
      <listheader label="listhead2"/>
    </listhead>
    <listitem>
      <listcell label="listcell11"/>
      <listcell label="listcell12"/>
    </listitem>
    <listitem>
      <listcell label="listcell21"/>
      <listcell label="listcell22"/>
    </listitem>
  </listbox>

 Accerciser shows its exposed accessible tree is like below:

  -list
    -list
      -column header ("listheader1")
      -column header ("listheader2")
    -list item
      -table cell ("listcell11")
      -table cell ("listcell12")
    -list item
      -table cell ("listcell21")
      -table cell ("listcell22")

  I think we should deal with <listheader>s. At least CellRefAt() should count <listheader> in.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> some questions:
> 
> 1) nsXULListboxAccessible implements nsIAccessibleTable, while we didn't expose
> its role as table. How AT could use it?

I do not very well AT side but it sounds correct to expose table role.

> 2) As comment #8, listcell may contains anything, shouldn't
> nsXULListCellAccessible implements HyperText?

Right.

>   I think we should deal with <listheader>s. At least CellRefAt() should count
> <listheader> in.
> 

Sorry, can you give more details?

Comment 19

[Evan Yan]

(In reply to comment #18)
> Sorry, can you give more details?
> 
 ____________________
| header1  | header2 |
|____________________|
| cell1    | cell2   |
|____________________|

I think CellRefAt(0,0) should return cell1, not header1;
GetColumnDescription() could return headers;
GetColumnHeader() could return the parent of headers (refer to the accessible tree in comment #17).

Comment 20

[Aaron Leventhal]

I don't think this will make it into Firefox 3.

We all need to concentrate on our critical issues like crashes, shutdown problems, leaks and incorrect a11y info for basic HTML, XUL and JS. This isn't on the list.

The list is:
http://bugzilla.mozilla.org/show_bug.cgi?id=fox3access
or bugs
with "orca:urgent" "orca:immediate" or "orca:high" in the whiteboard.

Comment 21

[Evan Yan]

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

clear review request till we got new update on this.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> I don't think this will make it into Firefox 3.

I would like to check this approach on listbox. If it works fine the it will simplify code of xul:tree and allow to expose it easy as a table for IA2. Since most of critical bugs have been fixed then it's worth to try, right?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #17)
> > some questions:
> > 
> > 1) nsXULListboxAccessible implements nsIAccessibleTable, while we didn't expose
> > its role as table. How AT could use it?
> 
it sounds correct to expose table role.

Aaron, what do you think about table role on xul:listbox?

Comment 24

[Aaron Leventhal]

How about:
* If numColumns == 1, use ROLE_LIST
* If numColumns > 1, use ROLE_TABLE

Comment 25

[Aaron Leventhal]

(In reply to comment #22)
> I would like to check this approach on listbox. If it works fine the it will
> simplify code of xul:tree and allow to expose it easy as a table for IA2. Since
> most of critical bugs have been fixed then it's worth to try, right?

The bigger bug is bug 367905. I think it makes sense to deal with that all at once, but it will probably require some changes in various assistive technologies. If you are hot on this bug then look at that as well. Because of how much cooperation this will take with AT developers, I really don't think we will get to this in Firefox 3.

FWIW, I still see other important bugs in our fox3access blocker list:
https://bugzilla.mozilla.org/showdependencytree.cgi?id=396346&hide_resolved=1

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch

Aaron, will you have a time for review until Evan get back?

Comment 27

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 28

[alexander :surkov (:asurkov)]

Marco, could you try the patch?

Comment 29

[Marco Zehe (:MarcoZ)]

I ran with this patch, and I don't think this is the best solution. I'd prefer it if these were rendered as multicolumn list items. Using a table element here doesn't produce any useful results. Since this is initially a listbox, the items should be ListItems, nothing having to do with tables.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> I ran with this patch, and I don't think this is the best solution. I'd prefer
> it if these were rendered as multicolumn list items.

What does "multicolumn list items" mean?

> Using a table element here
> doesn't produce any useful results. Since this is initially a listbox, the
> items should be ListItems, nothing having to do with tables.
> 

i.e. we don't need here to expose table interface? because multicolumn listbox has all features of the table. Actually xul:listbox is like xul:tree excepting that xul:listbox is linear structure (we can consider it as subset of xul:tree). Therefore I think we started to expose xul:listbox as table.

Comment 31

[alexander :surkov (:asurkov)]

Now I think multicolumn listitems of xul:listbox exposes all their children and they should be accessible for AT, but we do not expose that xul:listbox looks like table.

Comment 32

[Aaron Leventhal]

For me this is not a priority right now. I would wait until after Firefox 3 and implement this along with bug 367905. It should receive the table interface support in order to have parity with what's expected under ATK/AT-SPI.

Comment 33

[alexander :surkov (:asurkov)]

It should nice feature for firefox3 still. I'm not sure I want to get one huge patch from bug 367905. I think implementation approaches should be different as well because listbox/tree provides special interfaces I'd prefer to work than to run through the tree in case of ARIA trees. It would be nice to move forward step by step to the big issue of the bug 367905. Also I think xul:tree support (I see it as next step) is major feature because xul:tree aren't static anymore, they allows to edit cells and it seems we don't support this.

We have patch already, do you think we should get regressions or it may take significant time to get it in?

Comment 34

[Aaron Leventhal]

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

Ok. I'd like to have Evan look first.

Comment 35

[Evan Yan]

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

looks good.

>+  "listbox reach option", //ROLE_RICH_OPTION
you mean "rich" not "reach", right?

How about implement QueryInterface for list accessible? so that QI to nsIAccessibleTable will fail when numColumns == 1

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> (From update of attachment 302562 [details] [diff] [review])
> looks good.
> 
> >+  "listbox reach option", //ROLE_RICH_OPTION
> you mean "rich" not "reach", right?
> 
> How about implement QueryInterface for list accessible? so that QI to
> nsIAccessibleTable will fail when numColumns == 1
> 

you're right, I'll fix it. Anything more?

Comment 37

[Evan Yan]

(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 302562 [details] [diff] [review] [details])
> > looks good.
> > 
> > >+  "listbox reach option", //ROLE_RICH_OPTION
> > you mean "rich" not "reach", right?
> > 
> > How about implement QueryInterface for list accessible? so that QI to
> > nsIAccessibleTable will fail when numColumns == 1
> > 
> 
> you're right, I'll fix it. Anything more?
> 
No. I would r+ the patch with those two issues fixed.

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 39

[Evan Yan]

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

>-static const char kRoleNames[][20] = {
[...]
>+  "combobox list",        //ROLE_COMBOBOX_LIST
>+  "combobox option",      //ROLE_COMBOBOX_OPTION
>+  "image map",            //ROLE_IMAGE_MAP

we don't need all the above white space changes now.

>+nsresult
>+nsXULListboxAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr)
>+{
>+  nsresult rv = nsXULSelectableAccessible::QueryInterface(aIID, aInstancePtr);
>+  if (*aInstancePtr)
>+    return rv;
>+
>+  if (aIID.Equals(NS_GET_IID(nsIAccessibleTable)) && IsTree()) {
>+    *aInstancePtr = static_cast<nsIAccessibleValue*>(this);

nsIAccessibleTable, not nsIAccessibleValue

> 
>-/** Inherit the ISupports impl from nsAccessible, we handle nsIAccessibleSelectable */
> NS_IMPL_ISUPPORTS_INHERITED0(nsXULListitemAccessible, nsAccessible)
> 
why did you remove the comment?

r=me with the above issues fixed.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)
> (From update of attachment 303834 [details] [diff] [review])
> >-static const char kRoleNames[][20] = {
> [...]
> >+  "combobox list",        //ROLE_COMBOBOX_LIST
> >+  "combobox option",      //ROLE_COMBOBOX_OPTION
> >+  "image map",            //ROLE_IMAGE_MAP
> 
> we don't need all the above white space changes now.

Yes, but I would like to keep them aligned.

> >+  if (aIID.Equals(NS_GET_IID(nsIAccessibleTable)) && IsTree()) {
> >+    *aInstancePtr = static_cast<nsIAccessibleValue*>(this);
> 
> nsIAccessibleTable, not nsIAccessibleValue

thank you

> > 
> >-/** Inherit the ISupports impl from nsAccessible, we handle nsIAccessibleSelectable */
> > NS_IMPL_ISUPPORTS_INHERITED0(nsXULListitemAccessible, nsAccessible)
> > 
> why did you remove the comment?

right I shouldn't

Comment 41

[Evan Yan]

(In reply to comment #40)
> (In reply to comment #39)
> > (From update of attachment 303834 [details] [diff] [review] [details])
> > >-static const char kRoleNames[][20] = {
> > [...]
> > >+  "combobox list",        //ROLE_COMBOBOX_LIST
> > >+  "combobox option",      //ROLE_COMBOBOX_OPTION
> > >+  "image map",            //ROLE_IMAGE_MAP
> > 
> > we don't need all the above white space changes now.
> 
> Yes, but I would like to keep them aligned.

After fixed "reach" to "rich", you can align them without white space changes.

Comment 42

[alexander :surkov (:asurkov)]

checked in with evan's comments

Comment 43

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381