744790 - [Mac] HTML table semantics are not communicated to VoiceOver at all.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

Even the simplest tables are not recognized. So reading data tables on the web isn't as convenient as in Safari.

Note that only those tables should be exposed that are NOT layout tables. So, if we can, we should be smart about which tables we expose to VoiceOver. Safari also renders many layout tables to VoiceOver, which is very confusing. Being smart about it may give us an edge here.

Comment 1

[Marco Zehe (:MarcoZ)]

Not crutial to an initial release, but definitely one of the features that should be done soon. Setting priority/imprtance to P2.

Comment 2

[Hubert Figuiere [:hub]]

If I do a <table>, Safari sees it as grouped elements. It does not find it with "Next Table"
On the other hand, Nightly say I'm in a listbox. Does not find a table either. There is obviously something wrong (that I can fix).

What kind of table are you talking about? I might be missing something.

Comment 3

[Marco Zehe (:MarcoZ)]

Real HTML:table with html:tr and html:td inside, possibly also html:th in the first html:tr element so it's definitely considered a data table.

Comment 4

[Frédéric Wang (:fredw)]

(In reply to Marco Zehe (:MarcoZ) from comment #0)
> So reading data tables on the web isn't as convenient as in Safari.

I like this euphemism :-)

> Note that only those tables should be exposed that are NOT layout tables.
> So, if we can, we should be smart about which tables we expose to VoiceOver.
> Safari also renders many layout tables to VoiceOver, which is very
> confusing. Being smart about it may give us an edge here.

I think we already have heuristics to determine "layout" table that we can reuse here. How should layout tables be exposed?

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to Frédéric Wang (:fredw) from comment #4)
> > Note that only those tables should be exposed that are NOT layout tables.
> > So, if we can, we should be smart about which tables we expose to VoiceOver.
> > Safari also renders many layout tables to VoiceOver, which is very
> > confusing. Being smart about it may give us an edge here.
> 
> I think we already have heuristics to determine "layout" table that we can
> reuse here. How should layout tables be exposed?

I think they should be regular block elements. Basically all the contents rendered without the table semantics.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Testcase

Comment 7

[Frédéric Wang (:fredw)]

Attachment: Patch V1

I attach a first patch to try and add basic table support. For now, the "is layout table" heuristic is not used.

As I read the WebKit code, here are the attributes used by VoiceOver:

1) table attributes
  NSAccessibilityRowsAttribute
  It seems that a list of rows is expected here. The patch returns the list of children of the table accessible (which I expect are rows).

  NSAccessibilityVisibleRowsAttribute
  NSAccessibilityVisibleColumnsAttribute
  NSAccessibilityVisibleCellsAttribute
  I'm not sure our accessibility code can determine visibility of table parts, so these are ignored for now.

  NSAccessibilityColumnsAttribute
  I'm not sure our accessibility code can easily handle columns, so this is ignored for now.

  NSAccessibilityColumnHeaderUIElementsAttribute
  NSAccessibilityRowHeaderUIElementsAttribute
  It seems that a list of accessibles is expected here. Probably the list of header cell accessibles. This is not exposed by the patch, so probably the table headers will not be read correctly at the moment.

  NSAccessibilityHeaderAttribute
  As we discussed with Alex, this is probably the <caption> accessible, so that's what the patch returns.

  NSAccessibilityColumnCountAttribute
  NSAccessibilityRowCountAttribute
  The patch returns TableAccessible:RowCount and TableAccessible:ColCount.

2) row attribute
  NSAccessibilityIndexAttribute
  The patch returns the row (actually child) index.

3) column attributes
  NSAccessibilityIndexAttribute
  NSAccessibilityHeaderAttribute
  NSAccessibilityRowsAttribute
  NSAccessibilityVisibleRowsAttribute
  Again, these are not exposed by the patch at the moment, because we don't have a convenient way to handle columns.

4) cell attributes
  NSAccessibilityRowIndexRangeAttribute
  NSAccessibilityColumnIndexRangeAttribute
  For these, the patch returns a range [startIndex, endIndex] using TableCellAccessible::ColIdx, TableCellAccessible::RowIdx, TableCellAccessible::ColExtent, TableCellAccessible::RowExtent. I guess this is used by cells spanning several rows/columns.

  NSAccessibilityColumnHeaderUIElementsAttribute
  NSAccessibilityRowHeaderUIElementsAttribute
  Same as above. These are probably the lists provided by TableCellAccessible::ColHeaderCells and TableCellAccessible::RowHeaderCells

Comment 8

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dab8bb08e77

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Part 2 - Expose basic NSAccessibility attributes for tables

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e272a6697a7

Comment 10

[Frédéric Wang (:fredw)]

So I just uploaded a new patch after some testing with Alex:

> 1) table attributes
>   NSAccessibilityRowsAttribute
>   It seems that a list of rows is expected here. The patch returns the list
> of children of the table accessible (which I expect are rows).

The new version returns only children that are actually rows, otherwise this does not work for e.g. <caption>.

>   NSAccessibilityHeaderAttribute
>   As we discussed with Alex, this is probably the <caption> accessible, so
> that's what the patch returns.

Apparently, this was some kind of anonymous AXGroup?? I removed this attribute for now, but hopefully caption can still be read as they appear in the accessible tree.

> 2) row attribute
>   NSAccessibilityIndexAttribute
>   The patch returns the row (actually child) index.
> 

The new version now really returns the row index, not child index (again, these can be different if the table has a <caption>

> 4) cell attributes
>   NSAccessibilityRowIndexRangeAttribute
>   NSAccessibilityColumnIndexRangeAttribute
>   For these, the patch returns a range [startIndex, endIndex] using
> TableCellAccessible::ColIdx, TableCellAccessible::RowIdx,
> TableCellAccessible::ColExtent, TableCellAccessible::RowExtent. I guess this
> is used by cells spanning several rows/columns.

We checked that indeed they are used for colspan/rowspan. I fixed the interpretation of "range" after checking the WebKit's code.

> 
>   NSAccessibilityColumnHeaderUIElementsAttribute
>   NSAccessibilityRowHeaderUIElementsAttribute
>   Same as above. These are probably the lists provided by
> TableCellAccessible::ColHeaderCells and TableCellAccessible::RowHeaderCells

The new patch returns these lists. Hopefully it's enough to support table headers.

Comment 11

[Frédéric Wang (:fredw)]

Comment on attachment 8626050 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables

Mac users: can one of you please try the new patch / try build and tell if that helps VoiceOver to read the tables of the testcase?

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0e272a6697a7/try-macosx64/firefox-41.0a1.en-US.mac.dmg

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 8626050 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables

I don't see any exposed table semantics in this try build yet. My suspicion is that the role/subrole/roledescription mapping isn't correct yet. So tables are still announced as list boxes, more to the point: table rows are.

Comment 13

[Frédéric Wang (:fredw)]

Attachment: Part 1 - Map tabular accessibles to appropriate roles

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Marco Zehe (:MarcoZ) from comment #12)
> My suspicion
> is that the role/subrole/roledescription mapping isn't correct yet. So
> tables are still announced as list boxes, more to the point: table rows are.

Thanks. It seems you are right... For some reason, RoleMap.h has mapping for rows and colums but not for tables / cells. Perhaps the intention was to do that in mozHTMLAccessible:role according to whether the table is a "layout table" or not. Anyway, for now I uploaded attachment 8626087 [details] [diff] [review] and submitted it to try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e51d0e7215

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 8626050 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables

Except for "0 columns" when entering a table, this looks good. Together with part1, that is.

Comment 16

[Frédéric Wang (:fredw)]

(In reply to Marco Zehe (:MarcoZ) from comment #15)
> Comment on attachment 8626050 [details] [diff] [review]
> Part 2 - Expose basic NSAccessibility attributes for tables
> 
> Except for "0 columns" when entering a table, this looks good. Together with
> part1, that is.

Thanks I suspect that it is related to the fact that "NSAccessibilityVisibleColumnsAttribute" and "3) column attributes" mentioned in comment 7 are not implemented by this patch. I suggest to move this and the "layout table" stuff in follow-up patches.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8626050 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables

Review of attachment 8626050 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: accessible/mac/mozAccessible.mm
@@ +84,5 @@
> +  // iterate through the list, and get each native accessible.
> +  uint32_t totalCount = aArray.Length();
> +  for (uint32_t i = 0; i < totalCount; i++) {
> +    Accessible* curAccessible = aArray.ElementAt(i);
> +    if (curAccessible) {

nit: you shouldn't need this check

@@ +245,5 @@
>                                                             nil];
>    }
>  
> +  if (!tableAttrs) {
> +    tempArray = [[NSMutableArray alloc] initWithArray:generalAttributes];

why you can't use tableAttrs directly? it cannot be NSMutableArray?

@@ +356,5 @@
>    }
>    if ([attribute isEqualToString:NSAccessibilityHelpAttribute])
>      return [self help];
>  
> +  if (accWrap->IsTable()) {

it'd be good to move this code into separate class, that seems goes with current architecture, and you can skip IsTable() checks

@@ +357,5 @@
>    if ([attribute isEqualToString:NSAccessibilityHelpAttribute])
>      return [self help];
>  
> +  if (accWrap->IsTable()) {
> +    TableAccessible* tableAcc = accWrap->AsTable();

nit: you may skip 'Acc' from names

@@ +358,5 @@
>      return [self help];
>  
> +  if (accWrap->IsTable()) {
> +    TableAccessible* tableAcc = accWrap->AsTable();
> +    if (tableAcc) {

not check is needed since IsTable returns true

@@ +367,5 @@
> +      if ([attribute isEqualToString:NSAccessibilityRowsAttribute]) {
> +        // Create a new array with the list of table rows.
> +        NSMutableArray* nativeArray = [[NSMutableArray alloc] init];
> +        for (Accessible* tempAcc = accWrap->FirstChild(); tempAcc;
> +             tempAcc = tempAcc->NextSibling()) {

ChildAt is more effective

@@ +380,5 @@
> +    }
> +  } else if (accWrap->IsTableRow()) {
> +    if ([attribute isEqualToString:NSAccessibilityIndexAttribute]) {
> +      // Count the number of rows before that one to obtain the row index.
> +      uint32_t i = 0;

nit: index or idx?

@@ +382,5 @@
> +    if ([attribute isEqualToString:NSAccessibilityIndexAttribute]) {
> +      // Count the number of rows before that one to obtain the row index.
> +      uint32_t i = 0;
> +      for (Accessible* tempAcc = accWrap->PrevSibling(); tempAcc;
> +           tempAcc = tempAcc->PrevSibling()) {

here too ChildAt

@@ +391,5 @@
> +      return [NSNumber numberWithUnsignedInteger:i];
> +    }
> +  } else if (accWrap->IsTableCell()) {
> +    TableCellAccessible* cellAcc = accWrap->AsTableCell();
> +    if (cellAcc) {

no check

Comment 18

[Frédéric Wang (:fredw)]

Attachment: Part 2 - Expose basic NSAccessibility attributes for tables

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Part 2 - Expose basic NSAccessibility attributes for tables

Comment 20

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff2bf7fa1c03

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96a1f71786c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/289802193a68

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96a1f71786c4
https://hg.mozilla.org/mozilla-central/rev/289802193a68