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

RESOLVED FIXED in Firefox 41

Status

()

Core
Disability Access APIs
P2
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: MarcoZ, Assigned: fredw)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla41
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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

Updated

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

Comment 4

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

Comment 6

3 years ago
Created attachment 8625860 [details]
Testcase
(Assignee)

Updated

3 years ago
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Created attachment 8625870 [details] [diff] [review]
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
(Assignee)

Comment 8

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dab8bb08e77
Depends on: 1001641
(Assignee)

Comment 9

3 years ago
Created attachment 8626050 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e272a6697a7
Attachment #8625870 - Attachment is obsolete: true
(Assignee)

Comment 10

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

Comment 11

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

Updated

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

Updated

3 years ago
Attachment #8626050 - Attachment description: Patch V2 → Part 2 - Expose basic NSAccessibility attributes for tables
(Assignee)

Comment 13

3 years ago
Created attachment 8626087 [details] [diff] [review]
Part 1 - Map tabular accessibles to appropriate roles
(Assignee)

Comment 14

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

Updated

3 years ago
Attachment #8626087 - Flags: review+
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 16

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

3 years ago
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+
(Assignee)

Comment 18

3 years ago
Created attachment 8626365 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables
(Assignee)

Comment 19

3 years ago
Created attachment 8626435 [details] [diff] [review]
Part 2 - Expose basic NSAccessibility attributes for tables
Attachment #8626050 - Attachment is obsolete: true
Attachment #8626365 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff2bf7fa1c03
(Assignee)

Updated

3 years ago
Blocks: 1177637
(Assignee)

Updated

3 years ago
Blocks: 1177640

Comment 21

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/96a1f71786c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/289802193a68
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96a1f71786c4
https://hg.mozilla.org/mozilla-central/rev/289802193a68
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Blocks: 1178272
(Assignee)

Updated

2 years ago
Blocks: 1210023
You need to log in before you can comment on or make changes to this bug.