Closed Bug 744790 Opened 12 years ago Closed 9 years ago

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

Categories

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

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: MarcoZ, Assigned: fredw)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
Not crutial to an initial release, but definitely one of the features that should be done soon. Setting priority/imprtance to P2.
Priority: -- → P2
Summary: [Mac] HTML table semantics are not comunicated to VoiceOver at all. → [Mac] HTML table semantics are not communicated to VoiceOver at all.
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.
Target Milestone: --- → mozilla14
Version: Trunk → Other Branch
Target Milestone: mozilla14 → ---
Version: Other Branch → Trunk
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.
(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?
(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.
Attached file Testcase
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attached patch Patch V1 (obsolete) — Splinter Review
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
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 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/fred.wang@free.fr-0e272a6697a7/try-macosx64/firefox-41.0a1.en-US.mac.dmg
Attachment #8626050 - Flags: feedback?(surkov.alexander)
Attachment #8626050 - Flags: feedback?(mzehe)
Attachment #8626050 - Flags: feedback?(dbolter)
Attachment #8626050 - Flags: feedback?(david)
Attachment #8626050 - Flags: feedback?(david)
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.
Attachment #8626050 - Flags: feedback?(mzehe) → feedback-
Attachment #8626050 - Attachment description: Patch V2 → Part 2 - Expose basic NSAccessibility attributes for tables
(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
Attachment #8626050 - Flags: review?(surkov.alexander)
Attachment #8626050 - Flags: feedback?(surkov.alexander)
Attachment #8626050 - Flags: feedback?(dbolter)
Attachment #8626050 - Flags: feedback-
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.
Attachment #8626050 - Flags: feedback+
(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 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
Attachment #8626050 - Flags: review?(surkov.alexander) → review+
Attachment #8626050 - Attachment is obsolete: true
Attachment #8626365 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 1177637
Blocks: 1177640
https://hg.mozilla.org/mozilla-central/rev/96a1f71786c4
https://hg.mozilla.org/mozilla-central/rev/289802193a68
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1178272
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: