830748 - [AccessFu] Improve reading of table semantics

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Attachment: Patch

This bug introduces better reading of tables:

1. When entering a table, AccessFu speaks the row and column count, as well as any name deduced by our APIs from caption or @summary.

2. For each data cell, the row and column number are spoken, if the cell spans multiple rows or columns, and the row and column headers. It is intelligent so that, if the column changes, but the row doesn't, the row number and row header information are suppressed. Likewise if the column doesn't change, but the row does, column number and header are suppressed.

3. Layout tables are ignored.

Comment 1

[Marco Zehe (:MarcoZ)]

Comment on attachment 702289 [details] [diff] [review]
Patch

Adding davidb as discussed on IRC. Whoever gets to this first... ;-)

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 702289 [details] [diff] [review]
Patch

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

Great patch. The overall approach looks correct, except for the block below that might require some general reworking of some of our code. R minusing it for that reason.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +444,5 @@
>      return utterance;
> +  },
> +
> +  _oldColumnIndex: -1,
> +  _oldRowIndex: -1

I am concerned about putting this here. The utterance module has been so far stateless. So if a user is navigating tables in two different tabs, this logic will trip up a bit. To fix that it would need to be associated with the pivot or document.

The idea of storing the old index seems similar to what we do in PresenterContext when we have the old pivot position and subtract the lineage context from the current position before we send it to the utterance generator. I think that is where it belongs. Granted, the utterance API might need to change a bit to accommodate that.

In the PresenterContext I see there being getters such as rowChanged and columnChanged that would return true or false by querying the old accessible and checking if it is a child of a different cell.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 702289 [details] [diff] [review]
Patch

Thanks, Eitan! Cancelling review from David.

Sigh, why do I always have to pick those bugs that are hard to implement? I have no idea yet where to start except for the one pointer you gave me. Esp the API expansion part I have no clue yet as to which approach I should take. May take a while.

Comment 4

[Marco Zehe (:MarcoZ)]

OK, I have played around a bit, but haven't found a reliable way to track if a column and/or row have changed without using the table interface. For example, a row can have changed, but the column stayed the same, in which way we don't want to speak the column header again. Using parent/child relationships is no reliable or effective way to find this out.
So, I have to use the nsIAccessibleTable interface in both the presenterContext as well as utteranceGenerator, as ugly as this sounds. :( In presenterContext, I have to query the new row and column indizes and compare against the old ones. In utteranceGenerator, I have to query this info again to actually speak the information. I am hesitant to pass around large information objects here that hold these values and such. Instead, I want to introduce a new set of flags that will be used on some accessibles to tell the individual generators to gather specific infos. The indicators whether these flags should be used will be true/false values passed to the generate function in utteranceGenerator.
That's the current idea, but the patch is currently in an unpresentable state. Just storing the thoughts here, and if you have thoughts, please let me know!

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Marco, would you be able to give some suggestions about table output in case of braille. Thanks in advance :).

Comment 6

[Marco Zehe (:MarcoZ)]

For a start, I'd say this:
RxCy row header|column header|cell content
where x and y are of course the row and column headers. Unlike for speech, these should always be added, whereas as I wrote in the speech patch, it can be smarter when just changing columns or rows.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748

This is a WIP. I just wanted to get some feedback on TableCell object and the way relevant old table cell accessible is looked up. Thanks!

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 765208 [details] [diff] [review]
Patch for 830748

Wow, nifty! As far as I can see, all that I was hoping for is in there. f=me.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v2

Still the same WIP, just some fixes and improvements added.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 765357 [details] [diff] [review]
Patch for 830748 v2

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

I am having a hard time understanding the TableCell object and how it is used. On a pivot change, we could have more than one cell in the newAncestry. I think it could be very simple.

Here is some pseudo code:

function cell(aAccessible, aRoleStr, aStates, aFlags, aContext) {
  function getCellIface(acc) {
    try {
      return acc.queryInterface(Ci.nsIAccessibleTableCell);
    catch (x) {
      return null;
    }
  }

  function getPreviousCell(accs, aTable) {
    for (let acc of accs) {
       let cell = getCellIface(acc);
       if (cell && cell.table == aTable)
         return cell;
    }
    return null;
  }

  let cellAcc = getCellIface(aAccessible);
  let previousCellAcc = getPreviousCell(aContext.oldAncestry, cellAcc.table);
  let newCol = previousCellAcc ? cellAcc.columnIndex != previousCellAcc.columnIndex : true;
  let newRow = previousCellAcc ? cellAcc.rowIndex != previousCellAcc.rowIndex : true;

  // Get extents, construct utterance, etc.
}

::: accessible/src/jsat/OutputGenerator.jsm
@@ +411,5 @@
> +        // We don't want to speak any table information for layout tables.
> +        if (accessibleTable.isProbablyForLayout()) {
> +          return utterance;
> +        }
> +        utterance.push(gStringBundle.formatStringFromName('tableInfo',

Is this already in AccessFu.properties?

@@ +447,5 @@
> +      return this.objectUtteranceFunctions.cell.apply(this, arguments);
> +    }
> +  },
> +
> +  _addCellChanged: function _addCellChangedUtterance(aUtterance, aChanged, aString, aIndex) {

I could see why subroutines like _addExtent, _addChellChanged, and _addHeaders, are useful here. I would make the functions in the cell() function scope instead as private method members, since they are only used by cell.

::: accessible/src/jsat/Utils.jsm
@@ +577,5 @@
>      return this._traverse(this._accessible, aPreorder, aStop);
>    },
>  
> +  getAccessibleTableCell: function getAccessibleTableCell(aAccessible) {
> +    if (!this._accessibleTableCell) {

How can you cache _accessibleTableCell for any given argument in this function?

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #10)

Yes, caching of _accessibleTableCell would be gone (an artifact I did not remove).

AccessFu.properties is going to be updated with this patch as well.

Yes my thinking was that I the TableCell object could be used to simplify access to some of the cell attributes. And yes it would not be cached, so more than one cell could be cached..

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v3

WIP with addressed comments.

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 702289 [details] [diff] [review]
Patch

Marking my old patch as obsolete since that's what it is.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v4

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for the patch v1

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

I should be able to add braille shortly.

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 766221 [details] [diff] [review]
Tests for the patch v1

f=me. The only thing I'm missing is a test for a layout table, e. g. that we do not get an utterance for a table that Firefox specifically recognizes as probably for layout. mochitest/tables should have some example tables that are definitely recognized as such. Please re-use that and make sure this situation is tested for.

Comment 18

[Marco Zehe (:MarcoZ)]

Comment on attachment 766220 [details] [diff] [review]
Patch for 830748 v4

Is this patch ready to be tested? e. g. can I put it in my MQ and build locally with it to test?

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Marco Zehe (:MarcoZ) from comment #18)
> Comment on attachment 766220 [details] [diff] [review]
> Patch for 830748 v4
> 
> Is this patch ready to be tested? e. g. can I put it in my MQ and build
> locally with it to test?

Yes, I believe so.

Comment 20

[Marco Zehe (:MarcoZ)]

Comment on attachment 766220 [details] [diff] [review]
Patch for 830748 v4

I compiled locally with this build and like what I see. :)

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

More comments about the braille output from Marco:
http://krijnhoetmer.nl/irc-logs/accessibility/20130624#l-121

Comment 22

[Eitan Isaacson [:eeejay]]

Comment on attachment 766220 [details] [diff] [review]
Patch for 830748 v4

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

Overall this looks great. I'm concerned that maybe the re-implementation of newAncestry is not functionally identical. Big plus besides that (and other small issues).

::: accessible/src/jsat/Utils.jsm
@@ +392,5 @@
>    get newAncestry() {
>      if (!this._newAncestry) {
> +      this._newAncestry = [currentAncestor for (
> +        [index, currentAncestor] of Iterator(this.currentAncestry)) if (
> +          currentAncestor !== this.oldAncestry[index])];

This blows my mind! I don't think it is functionally identical to what it replaces. Previously, we would work backwards in both lists because that is how they would align (ie. old root equals new root, etc. until we encounter the first divergence).

So the index you use here from the currentAncestry does not align with oldAncestry at all.

I might be wrong, because this line is hard to read. Am I?

@@ +439,5 @@
>    },
>  
> +  getCellInfo: function getCellInfo(aAccessible) {
> +    if (!this._cells) {
> +      this._cells = new Map();

I know I said on IRC that we don't need a WeakMap here because this is a short lived object. But maybe it is not a bad idea after all :P

Using the accessible as a key in WeakMap won't work AFAIK because it does not support weak references afaik. So the key should be aAccessible.DOMNode.

@@ +474,5 @@
> +
> +    cellInfo.current = getAccessibleCell(aAccessible);
> +
> +    cellInfo.previous = null;
> +    let table = cellInfo.current.table;

cellInfo.current could be null. Might be worth looging a warning, putting in a map entry with a value of null, and returning.

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +83,5 @@
> +# Description of a table or grid:
> +# 1 is a dynamically retrieved localized role of either 'table' or 'grid'.
> +# 2 is the number of columns within the table.
> +# 3 is the number of rows within the table or grid.
> +tableInfo = %S with %S columns and %S rows

You probably want to have plural rules here.

https://developer.mozilla.org/en-US/docs/Localization_and_Plurals

Comment 23

[Eitan Isaacson [:eeejay]]

Comment on attachment 766221 [details] [diff] [review]
Tests for the patch v1

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

Overall good. Some surprising "expected" results. But I think that is an implementation issue.

::: accessible/tests/mochitest/jsat/test_tables.html
@@ +42,5 @@
> +      }, {
> +        // Test pivot to table1_th1 from table1.
> +        accOrElmOrID: "table1_th1",
> +        oldAccOrElmOrID: "table1",
> +        expected: ["table with 2 columns and 2 rows", "Column 1 Row 1", "col1"]

I don't think the first element of the expected utterance should be there, same with the similar tests below.

Comment 24

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v5

I tested the old version of newAncestry, and in case where the pivot was from parent table to a cell in its subtree, the table was still announced.

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for the patch v2

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 767193 [details] [diff] [review]
Tests for the patch v2

Cancelling review. Still need to add layout test.

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for the patch v3

Added the layout table test.

Comment 28

[Marco Zehe (:MarcoZ)]

Comment on attachment 767215 [details] [diff] [review]
Tests for the patch v3

As I reviewed this test, I noticed that I had forgotten to give one additional piece of info. As soon as a layout table is encountered, all cell-related description info should be omitted. So the user should not be told that they're in a cell, just all the elements within the td should be treated as if they were in a layout element.
There is a way to get to the parent table from a cell, it's within the NSIAccessibleTableCell interface, so it should be possible to just query that table accessible for the NSIAccessibleTable interface and get the info if it's a layout table, and if so, just utter the name but leave out all column, row, and other extra information pertaining to the cell itself.

Sorryabout that, my bad that I forgot to mention this!

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v6

Comment 30

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for the patch v4

Comment 31

[Eitan Isaacson [:eeejay]]

Comment on attachment 767251 [details] [diff] [review]
Patch for 830748 v6

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

Looking good!

Comment 32

[Eitan Isaacson [:eeejay]]

Comment on attachment 767252 [details] [diff] [review]
Tests for the patch v4

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

Great. Like we said on IRC, we need tests for description-last too. But that could come later.

Comment 33

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 830748 v7

Minor update to Accessfu.properties (tableColumnInfoAbbr and tableRowInfoAbbr) to eliminate PluralForm warning.

Comment 34

[Yura Zenevich [:yzen] (please use "needinfo")]

try: https://tbpl.mozilla.org/?tree=Try&rev=295d689f5a9a

Comment 35

[Eitan Isaacson [:eeejay]]

Comment on attachment 767600 [details] [diff] [review]
Patch for 830748 v7

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

Looks great!

Comment 36

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3d8befb0e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac97b384249

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb3d8befb0e9
https://hg.mozilla.org/mozilla-central/rev/2ac97b384249