Bug 25534 – [GTK] Objects of ROLE_TABLE should implement the accessible table interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Steps to reproduce:

1. Open the (to be) attached test case.

2. In Accerciser, select the table in the list of objects in the left-hand pane. Then use Accerciser's Interface Viewer to view the information exposed via the accessible table interface.

Expected results: The accessible table interface would be implemented, exposing all of the information detailed here: http://library.gnome.org/devel/atk/unstable/AtkTable.html

Actual results: The accessible table interface does not appear to be implemented for accessibles of ROLE_TABLE.

Note: I believe I have all of the needed patches associated with bug 21546.

Also, please note that the accessible hierarchy of the table seems unusual. The table cells appear to each be represented by two children within the table rather than one. First they appear in row order as children of objects of ROLE_LIST_ITEM. Then they appear again in column order as children of objects of ROLE_UNKNOWN. Please let me know if you'd like me to open a separate bug for this latter issue.

[Joanmarie Diggs]

Created attachment 29971 [details]
aforementioned test case

[Joanmarie Diggs]

Calling dibs on this one as I've already done quite a bit of the implementation (and don't seem to have the necessary privs to assign this to myself).

[Joanmarie Diggs]

Created attachment 41481 [details]
Part 1

Part 1:
* accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
(getCell):
(getCellIndex):
(webkit_accessible_table_ref_at):
(webkit_accessible_table_get_index_at):
(webkit_accessible_table_get_n_columns):
(webkit_accessible_table_get_n_rows):
(webkit_accessible_table_get_column_extent_at):
(webkit_accessible_table_get_row_extent_at):
(webkit_accessible_table_get_column_header):
(webkit_accessible_table_get_row_header):
(atk_table_interface_init):
(AtkInterfacesInitFunctions):
(GetAtkInterfaceTypeFromWAIType):

[Xan Lopez]

Comment on attachment 41481 [details]
Part 1

>+
>+static guint getCellIndex(AccessibilityTableCell* AXCell, AccessibilityTable* AXTable)
>+{
>+    // Calculate the cell's index as if we had traditional Gtk+ tables.

So how does that work exactly? Does it simply put row after row, the index being n_columns*row + column or something like that? The way you are doing it in the patch you are simply relying on cells to be ordered the way you want, so I guess it wouldn't hurt to be a bit more explicit.

>+    AccessibilityObject::AccessibilityChildrenVector allCells;
>+    AXTable->cells(allCells);
>+    unsigned cellCount = allCells.size();
>+    for (unsigned k = 0; k < cellCount; ++k)
>+        if (allCells[k] == AXCell)
>+            return k;

if allCells is a normal C++ vector I think you can use std::find to get the index of the element intsead of looking for it manually. It will return an iterator, so to get the index value you just have to substract from it the first iterator in the vector (so... something like pos = find(allCells.begin(), allCells.end(), AXCell) - allCells.begin();)

Now that I just wrote this I wonder if it actually matters or if the code is more readable that way... feel free to ignore it :)

>+    return -1;

Don't think returning '-1' for error will work having a 'guint' return value :)

>+}
>+
>+static AtkObject* webkit_accessible_table_ref_at(AtkTable* table, gint row, gint column)
>+{
>+    AccessibilityTableCell* AXCell = getCell(table, row, column);
>+    if (!AXCell)
>+        return NULL;
>+    return AXCell->wrapper();
>+}
>+
>+static gint webkit_accessible_table_get_index_at(AtkTable* table, gint row, gint column)
>+{
>+    AccessibilityTableCell* AXCell = getCell(table, row, column);
>+    AccessibilityTable* AXTable = static_cast<AccessibilityTable*>(core(table));
>+    return getCellIndex(AXCell, AXTable);
>+}
>+
>+static gint webkit_accessible_table_get_n_columns(AtkTable* table)
>+{
>+    AccessibilityObject* accTable = core(table);
>+    if (accTable->isAccessibilityRenderObject())
>+        return static_cast<AccessibilityTable*>(accTable)->columnCount();
>+    return 0;
>+}
>+
>+static gint webkit_accessible_table_get_n_rows(AtkTable* table)
>+{
>+    AccessibilityObject* accTable = core(table);
>+    if (accTable->isAccessibilityRenderObject())
>+        return static_cast<AccessibilityTable*>(accTable)->rowCount();
>+    return 0;
>+}
>+
>+static gint webkit_accessible_table_get_column_extent_at(AtkTable* table, gint row, gint column)
>+{
>+    AccessibilityTableCell* AXCell = getCell(table, row, column);
>+    if (AXCell) {
>+        pair<int, int> columnRange;
>+        AXCell->columnIndexRange(columnRange);
>+        return columnRange.second;

Just a sanity check: what comes in that pair, the indexes of the columns the cell occupies? If the function returns the extent shouldn't you return second - first?

>+    }
>+    return 0;
>+}
>+
>+static gint webkit_accessible_table_get_row_extent_at(AtkTable* table, gint row, gint column)
>+{
>+    AccessibilityTableCell* AXCell = getCell(table, row, column);
>+    if (AXCell) {
>+        pair<int, int> rowRange;
>+        AXCell->rowIndexRange(rowRange);
>+        return rowRange.second;
>+    }
>+    return 0;
>+}
>+
>+static AtkObject* webkit_accessible_table_get_column_header(AtkTable* table, gint column)
>+{
>+    // Something is not quite right here. :-( We get headers but not the child
>+    // text which holds the header content. Also note that the nearly identical
>+    // get_row_header works as expected. Might be related to the funky hierarchy.
>+    AccessibilityObject* accTable = core(table);
>+    if (accTable->isAccessibilityRenderObject()) {
>+        AccessibilityObject::AccessibilityChildrenVector allColumnHeaders;
>+        static_cast<AccessibilityTable*>(accTable)->columnHeaders(allColumnHeaders);
>+
>+        unsigned colCount = allColumnHeaders.size();
>+        for (unsigned k = 0; k < colCount; ++k) {
>+            AccessibilityObject* columnObject = allColumnHeaders[k]->parentObject();
>+            if (static_cast<AccessibilityTableColumn*>(columnObject)->columnIndex() == column)
>+                return allColumnHeaders[k]->wrapper();
>+        }
>+    }
>+    return NULL;
>+}

I'm sure you'll figure it out in a follow-up patch ;)

>+
>+static AtkObject* webkit_accessible_table_get_row_header(AtkTable* table, gint row)
>+{
>+    AccessibilityObject* accTable = core(table);
>+    if (accTable->isAccessibilityRenderObject()) {
>+        AccessibilityObject::AccessibilityChildrenVector allRowHeaders;
>+        static_cast<AccessibilityTable*>(accTable)->rowHeaders(allRowHeaders);
>+
>+        unsigned rowCount = allRowHeaders.size();
>+        for (unsigned k = 0; k < rowCount; ++k) {
>+            AccessibilityObject* rowObject = allRowHeaders[k]->parentObject();
>+            if (static_cast<AccessibilityTableRow*>(rowObject)->rowIndex() == row)
>+                return allRowHeaders[k]->wrapper();
>+        }
>+    }
>+    return NULL;
>+}
>+
>+static void atk_table_interface_init(AtkTableIface* iface)
>+{
>+    iface->ref_at = webkit_accessible_table_ref_at;
>+    iface->get_index_at = webkit_accessible_table_get_index_at;
>+    iface->get_n_columns = webkit_accessible_table_get_n_columns;
>+    iface->get_n_rows = webkit_accessible_table_get_n_rows;
>+    iface->get_column_extent_at = webkit_accessible_table_get_column_extent_at;
>+    iface->get_row_extent_at = webkit_accessible_table_get_row_extent_at;
>+    iface->get_column_header = webkit_accessible_table_get_column_header;
>+    iface->get_row_header = webkit_accessible_table_get_row_header;
>+}
>+
> static const GInterfaceInfo AtkInterfacesInitFunctions[] = {
>     {(GInterfaceInitFunc)atk_action_interface_init,
>      (GInterfaceFinalizeFunc) NULL, NULL},
>@@ -991,6 +1124,8 @@ static const GInterfaceInfo AtkInterfacesInitFunctions[] = {
>     {(GInterfaceInitFunc)atk_component_interface_init,
>      (GInterfaceFinalizeFunc) NULL, NULL},
>     {(GInterfaceInitFunc)atk_image_interface_init,
>+     (GInterfaceFinalizeFunc) NULL, NULL},
>+    {(GInterfaceInitFunc)atk_table_interface_init,
>      (GInterfaceFinalizeFunc) NULL, NULL}
> };
> 
>@@ -999,7 +1134,8 @@ enum WAIType {
>     WAI_EDITABLE_TEXT,
>     WAI_TEXT,
>     WAI_COMPONENT,
>-    WAI_IMAGE
>+    WAI_IMAGE,
>+    WAI_TABLE
> };
> 
> static GType GetAtkInterfaceTypeFromWAIType(WAIType type)
>@@ -1015,6 +1151,8 @@ static GType GetAtkInterfaceTypeFromWAIType(WAIType type)
>       return ATK_TYPE_COMPONENT;
>   case WAI_IMAGE:
>       return ATK_TYPE_IMAGE;
>+  case WAI_TABLE:
>+      return ATK_TYPE_TABLE;
>   }
> 
>   return G_TYPE_INVALID;
>@@ -1048,6 +1186,10 @@ static guint16 getInterfaceMaskFromObject(AccessibilityObject* coreObject)
>     if (coreObject->isImage())
>         interfaceMask |= 1 << WAI_IMAGE;
> 
>+    // Table
>+    if (role == TableRole)
>+        interfaceMask |= 1 << WAI_TABLE;
>+
>     return interfaceMask;
> }
> 
>-- 
>1.6.3.3
>

Looks great to me but for those small comments, let's fix them and commit it.

[Joanmarie Diggs]

Created attachment 41642 [details]
Part 1 - Take 2

(In reply to comment #4)
> So how does that work exactly? Does it simply put row after row, the index
> being n_columns*row + column or something like that?

Something very much like that. But we also have to keep cell spans in mind. What we get from a traditional Gtk+ table which does not manage its descendants is a table whose children are table cells, no intervening row or column accessibles, indexed from 0 to childCount - 1.

> The way you are doing it
> in the patch you are simply relying on cells to be ordered the way you want, so
> I guess it wouldn't hurt to be a bit more explicit.

I added a bit more detail to the original comment. Please let me know if further detail would be beneficial. Any concerns over the underlying order changing in the vector down the road?

> if allCells is a normal C++ vector I think you can use std::find to get the
> index of the element intsead of looking for it manually. It will return an
> iterator, so to get the index value you just have to substract from it the
> first iterator in the vector (so... something like pos = find(allCells.begin(),
> allCells.end(), AXCell) - allCells.begin();)

Cool. Being new to C++, I love these sorts of tips. Thanks!!
 
> Now that I just wrote this I wonder if it actually matters or if the code is
> more readable that way... feel free to ignore it :)

Too late. I went with the change. :-)

> >+    return -1;
> 
> Don't think returning '-1' for error will work having a 'guint' return value :)

Merely a test to see if you were paying attention. Yeah... A test... Because... I obviously was not. Thanks for catching this. Sorry it needed catching. Fixed.

> >+
> >+static gint webkit_accessible_table_get_column_extent_at(AtkTable* table, gint row, gint column)
> >+{
> >+    AccessibilityTableCell* AXCell = getCell(table, row, column);
> >+    if (AXCell) {
> >+        pair<int, int> columnRange;
> >+        AXCell->columnIndexRange(columnRange);
> >+        return columnRange.second;
> 
> Just a sanity check: what comes in that pair, the indexes of the columns the
> cell occupies?

Nope. It's the index of the column and then the column span:

void AccessibilityTableCell::columnIndexRange(pair<int, int>& columnRange)
{
    if (!m_renderer || !m_renderer->isTableCell())
        return;
    
    RenderTableCell* renderCell = toRenderTableCell(m_renderer);
    columnRange.first = renderCell->col();
    columnRange.second = renderCell->colSpan();    
}

> If the function returns the extent shouldn't you return second -
> first?

The extent is indeed the span. Verified with testing with a variety of tables.

> I'm sure you'll figure it out in a follow-up patch ;)

Me too, as there really isn't a choice. :-) I've not quite figured it out yet, however. If you'd like, I can just rip that function out until I get to the bottom of it.

In addition to the above, this patch:

1. Renames a couple of private methods (fooBar instead of getFooBar) to be in keeping with the style guide.

2. Implements:

   webkit_accessible_table_get_caption
   webkit_accessible_table_get_column_at_index
   webkit_accessible_table_get_row_at_index

   along with a new helper method, cellAtIndex.

3. Includes a "stub" for webkit_accessible_table_get_summary. (I thought it was going to be pretty much like caption; I was wrong. If I shouldn't include such stubs, let me know.)

[Joanmarie Diggs]

I'm working on the totally-bogus-table-hierarchy issue now. The winner -- at least in part -- seems to be the presence of a table border. I kid thee not. :-)

=== Debugging/Test Conditions ===
Locate every last instance where accessibilityIsIgnored() could possibly return false and ensure that it returns true.

=== Expected Results Under These Conditions ===
Because accessibility is being ignored for ALL things, I would expect to find an accessible of ROLE_DOCUMENT_FRAME with no text and no children.

=== Table Which Works As Expected ===
<html>
<head>
<title>Tables Test</title>
</head>
<body>
<table>
<tr><td>One</td></tr>
<tr><td>Two</td></tr>
</table>
</body>
</html>
~~~~~
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderTable {TABLE} at (0,0) size 36x50
        RenderTableSection {TBODY} at (0,0) size 36x50
          RenderTableRow {TR} at (0,2) size 36x22
            RenderTableCell {TD} at (2,2) size 32x22 [r=0 c=0 rs=1 cs=1]
              RenderText {#text} at (1,1) size 27x20
                text run at (1,1) width 27: "One"
          RenderTableRow {TR} at (0,26) size 36x22
            RenderTableCell {TD} at (2,26) size 32x22 [r=1 c=0 rs=1 cs=1]
              RenderText {#text} at (1,1) size 30x20
                text run at (1,1) width 30: "Two"
#EOF
=== Table Which Does NOT Work As Expected ===
<html>
<head>
<title>Tables Test</title>
</head>
<body>
<table border="1">
<tr><td>One</td></tr>
<tr><td>Two</td></tr>
</table>
</body>
</html>
~~~~~
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderTable {TABLE} at (0,0) size 40x56 [border: (1px outset #808080)]
        RenderTableSection {TBODY} at (1,1) size 38x54
          RenderTableRow {TR} at (0,2) size 38x24
            RenderTableCell {TD} at (2,2) size 34x24 [border: (1px inset #808080)] [r=0 c=0 rs=1 cs=1]
              RenderText {#text} at (2,2) size 27x20
                text run at (2,2) width 27: "One"
          RenderTableRow {TR} at (0,28) size 38x24
            RenderTableCell {TD} at (2,28) size 34x24 [border: (1px inset #808080)] [r=1 c=0 rs=1 cs=1]
              RenderText {#text} at (2,2) size 30x20
                text run at (2,2) width 30: "Two"
#EOF
=== Hierarchy In Accerciser ===
--> document frame
    --> list item - seems to be cell with "one"
    --> list item - seems to be cell with "two"
    --> unknown
        -> table cell - "one"
        -> table cell - "two"
    --> unknown - (no clue what this corresponds to....)

Now to figure out what is doing the bogus rendering....

[Joanmarie Diggs]

Created attachment 41898 [details]
table 1 (breaking these apart for easier review)

It's rough being the new kid. ;-)

Okay. This is based on the original patch reviewed by Xan. Changes:

1. Makes the suggested (and then withdrawn) change to use std:find.

2. Correct the guint/gint error

3. Acknowledges, but makes no changes in response to, this comment:

> Just a sanity check: what comes in that pair, the indexes of the columns the
> cell occupies?

Nope. It's the index of the column and then the column span:

void AccessibilityTableCell::columnIndexRange(pair<int, int>& columnRange)
{
    if (!m_renderer || !m_renderer->isTableCell())
        return;

    RenderTableCell* renderCell = toRenderTableCell(m_renderer);
    columnRange.first = renderCell->col();
    columnRange.second = renderCell->colSpan();    
}

> If the function returns the extent shouldn't you return second -
> first?

I need the colSpan, columnRange.second contains the colSpan. Testing with tables with multiple spans would also seem to indicate that what I have done is sound.

4. Removes webkit_accessible_table_get_column_header. Xan suggests I'll figure it out in a follow-up patch. Agreed. Might as well just introduce it into the code then.

Please review. Thanks!

[Joanmarie Diggs]

Created attachment 41913 [details]
table 2

Second part of the implementation of AtkTable.

* accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
(cellAtIndex):
(webkit_accessible_table_get_column_at_index):
(webkit_accessible_table_get_row_at_index):
(webkit_accessible_table_get_caption):
(atk_table_interface_init):

[Joanmarie Diggs]

I took a look at what remains to be done in terms of implementing AtkTable. And there's a bunch left. :-( However, in terms of the immediate need to make basic content accessible to users of the GNOME desktop, there's only four left. :-)

* atk_table_get_column_description 
* atk_table_get_row_description    
* atk_table_get_column_header
* atk_table_get_summary

Well, that, plus a completely bizarre and possibly broken table hierarchy.

For the rest of the AtkTable items, I have opened bug 30799. I've also created a new metabug (bug 30796) so that we can track such things without them distracting us from the must-do-now items from bug 25531.

[Joanmarie Diggs]

Created attachment 41924 [details]
table 3

* accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
(nameFromChildren):
(webkit_accessible_get_name):
New convenience function to construct an object's name using the name(s) of any children it has.

(atk_table_interface_init):
(webkit_accessible_table_get_column_description):
(webkit_accessible_table_get_row_description):
Implemented.

(webkit_accessible_table_get_column_header):
Stub function added so that webkit_accessible_table_get_column_description could be implemented in the meantime.

[Xan Lopez]

Comment on attachment 41898 [details]
table 1 (breaking these apart for easier review)

>+static gint cellIndex(AccessibilityTableCell* AXCell, AccessibilityTable* AXTable)
>+{
>+    // Calculate the cell's index as if we had a traditional Gtk+ table in
>+    // which cells are all direct children of the table, arranged row-first.
>+    //

No need for that empty comment line.

>+    AccessibilityObject::AccessibilityChildrenVector allCells;
>+    AXTable->cells(allCells);
>+    AccessibilityObject::AccessibilityChildrenVector::iterator position;
>+    position = std::find(allCells.begin(), allCells.end(), AXCell);
>+    if (position == allCells.end())
>+        return -1;
>+    return position - allCells.begin();
>+}
>+
>+static AtkObject* webkit_accessible_table_ref_at(AtkTable* table, gint row, gint column)
>+{
>+    AccessibilityTableCell* AXCell = cell(table, row, column);
>+    if (!AXCell)
>+        return NULL;

In theory we try to use '0' instead of 'NULL' (per WebKit style guide), but I realize this file is a mess wrt that.

>+    return AXCell->wrapper();
>+}
>+

I'll land this fixing those small details, thanks!

[Xan Lopez]

Comment on attachment 41898 [details]
table 1 (breaking these apart for easier review)

Landed in r50140, clearing flags.

Also, there were a few "Trailing whitespace" warnings. Try to be careful with that. You can make git lart at you for that if you do chmod +x .git/hooks/pre-commit in your local repo.

[Xan Lopez]

Comment on attachment 41913 [details]
table 2

>+static AccessibilityTableCell* cellAtIndex(AtkTable* table, gint index)
>+{
>+    AccessibilityObject* accTable = core(table);
>+    if (accTable->isAccessibilityRenderObject()) {
>+        AccessibilityObject::AccessibilityChildrenVector allCells;
>+        static_cast<AccessibilityTable*>(accTable)->cells(allCells);
>+        if (0 <= index < allCells.size()) {

Mmm, I don't think this does what you think. '0 <= index' becomes a boolean (0/1), which is then compared against allCells.size(). So you'd get TRUE in cases like 0 <= 1000 < 10, because 0 <= 1000 -> 1, 1 < 10 -> 1.

What you need is simply if (0 <= index && index < allCells.size()).


>+            AccessibilityObject* accCell = allCells.at(index).get();
>+            return static_cast<AccessibilityTableCell*>(accCell);
>+        }
>+    }
>+    return NULL;

While you are it, s/NULL/0/

Marking r- for now, but looks good to me otherwise.

[Xan Lopez]

Comment on attachment 41924 [details]
table 3

>+static const gchar* nameFromChildren(AccessibilityObject* object)
>+{
>+    if (object) {
>+        AccessibilityRenderObject::AccessibilityChildrenVector children = object->children();
>+        // Currently, object->stringValue() should be an empty String. This
>+        // might not be the case down the road.
>+        String name = object->stringValue();
>+        for (unsigned i = 0; i < children.size(); ++i)
>+            name += children.at(i).get()->stringValue();
>+        return returnString(name);
>+    }
>+
>+    return NULL;

I think it's better to do a if (!object)\nreturn 0; at the beginning here.

>+static AtkObject* webkit_accessible_table_get_column_header(AtkTable* table, gint column)
>+{
>+    // FIXME: This needs to be implemented.
>+    notImplemented();
>+    return NULL;
>+}

Same comment about NULL than in the other patches (sorry about that!).

r- for now, let's fix those two things.

[Joanmarie Diggs]

Created attachment 41959 [details]
table 2 - corrected as per review

>+static AccessibilityTableCell* cellAtIndex(AtkTable* table, gint index)
> >+{
> >+    AccessibilityObject* accTable = core(table);
> >+    if (accTable->isAccessibilityRenderObject()) {
> >+        AccessibilityObject::AccessibilityChildrenVector allCells;
> >+        static_cast<AccessibilityTable*>(accTable)->cells(allCells);
> >+        if (0 <= index < allCells.size()) {
> 
> Mmm, I don't think this does what you think. '0 <= index' becomes a boolean
> (0/1), which is then compared against allCells.size(). So you'd get TRUE in
> cases like 0 <= 1000 < 10, because 0 <= 1000 -> 1, 1 < 10 -> 1.
> 
> What you need is simply if (0 <= index && index < allCells.size()).

D'oh! Pythonism. Fixed.

> >+            AccessibilityObject* accCell = allCells.at(index).get();
> >+            return static_cast<AccessibilityTableCell*>(accCell);
> >+        }
> >+    }
> >+    return NULL;
> 
> While you are it, s/NULL/0/

Done.

Thanks for the review!

[Joanmarie Diggs]

Created attachment 41963 [details]
Table 2 - Adjusted to address compiler warning about comparing signed and unsigned

[Joanmarie Diggs]

Created attachment 41965 [details]
table 3 - corrected as per review

> I think it's better to do a if (!object)\nreturn 0; at the beginning here.

Done.
 
> Same comment about NULL than in the other patches (sorry about that!).

Done. No need to apologize. I need to re-read the style guide until it becomes second nature.

Thanks for the review!

[Joanmarie Diggs]

Created attachment 41999 [details]
Table Summary

atk_table_get_summary returns an AtkObject. Summary is an attribute; not an object. Therefore we need some other way to expose this information. Because a table summary describes the table, this patch exposes the summary attribute as the table's accessible description.

Please review. Thanks!

(Note that this patch does *not* depend on Table 2 or Table 3).

[Jan Alonzo]

Comment on attachment 41999 [details]
Table Summary

Looks fine. r=me.

[WebKit Commit Bot]

Comment on attachment 41999 [details]
Table Summary

Rejecting patch 41999 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11534 test cases.
accessibility/table-with-aria-role.html -> failed

Exiting early after 1 failures. 49 tests run.
3.15s total testing time

48 test cases (97%) succeeded
1 test case (2%) had incorrect layout
1 test case (2%) had stderr output

[Jan Alonzo]

Comment on attachment 41963 [details]
Table 2 - Adjusted to address compiler warning about comparing signed and unsigned

>      AccessibilityTableCell* AXCell = cell(table, row, column);

One minor nit: this should be axCell.

> +static gint webkit_accessible_table_get_column_at_index(AtkTable* table, gint index)
> +{
> +    AccessibilityTableCell* AXCell = cellAtIndex(table, index);

Ditto.

[Joanmarie Diggs]

Created attachment 42001 [details]
Table Summary - with ARIA check

I tried to run the tests (Jan, thanks for your help/pointers!). But they all claim to crash, e.g.:

  dom/html/level1/core/hc_attrremovechild1.html -> crashed

So.... I will add learning about the tests to my list of things to do. In the meantime, given the late hour....

The failed test is an ARIA one. I would think we could do a check for ARIA and not do the new functionality if it is ARIA.  That's what this patch does. To "test" I compared what Accerciser reported as the accessible description without my patch with what it reports with it. According to Accerciser, the values are the same. <fingers crossed>

If someone wouldn't mind tossing the regression tests at this patch, I would really, really appreciate it. Thanks!

[Jan Alonzo]

Comment on attachment 41999 [details]
Table Summary

One more time.

[Joanmarie Diggs]

(In reply to comment #23)
> (From update of attachment 41999 [details])
> One more time.

Just double-checking.... If you literally mean that attachment (41999), the one with the ARIA check (which should stop the tests from failing I hope) is attachment 42001 [details]. I obsoleted 41999.

[Jan Alonzo]

Comment on attachment 42001 [details]
Table Summary - with ARIA check

Still ok.

[WebKit Commit Bot]

Comment on attachment 42001 [details]
Table Summary - with ARIA check

Clearing flags on attachment: 42001

Committed r50219: <http://trac.webkit.org/changeset/50219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joanmarie Diggs]

(In reply to comment #27)
> All reviewed patches have been landed.  Closing bug.

Reopening. :-)

[Joanmarie Diggs]

.

[Eric Seidel (no email)]

This is caused directly by bug 28230.  However our tools just aren't really designed to work with more than one change per bug.  If you'd like to post a patch to fix bug 28230 you'd certainly be welcome to. :)  Eventually I'll get around to it.

[Joanmarie Diggs]

(In reply to comment #28)
> (In reply to comment #27)
> > All reviewed patches have been landed.  Closing bug.
> 
> Reopening. :-)

(In reply to comment #30)
> This is caused directly by bug 28230.  However our tools just aren't really
> designed to work with more than one change per bug.  If you'd like to post a
> patch to fix bug 28230 you'd certainly be welcome to. :)  Eventually I'll get
> around to it.

Eric, for the record, I was joking. (And if you thought otherwise, I'm truly sorry.) Because of several bugs which spiraled into uber bugs with a life all their own, we'd gotten into a silly pattern: review, commit queue, bug gets closed, bug gets reopened, rinse and repeat.

Your tools are awesome, and I wish we had stuff like that in GNOME. The problem is contributors like me. ;-) But to prove I am redeemable, I have just spun off two bugs from this one:

* Bug 30895 to deal with the bogus table hierarchy
* Bug 30896 to deal with atk_table_get_column_header

So, once the two remaining patches in need of final review and commit have both landed, we will never, ever re-open this bug again. :-)

And as for bug 28230, absolutely! Just as soon as all of the Atk a11y problems are resolved. ;-)

[Eric Seidel (no email)]

Oh, I certainly too no offense whatsoever! :)  I hope my terse response did not lead you to believe such.  Just wanted you to be informed as to why the bug got closed, that's all. :)  Thanks.

[Joanmarie Diggs]

Created attachment 42083 [details]
Table 2 - Address Jan's comments

(In reply to comment #21)
> (From update of attachment 41963 [details])
> >      AccessibilityTableCell* AXCell = cell(table, row, column);
> 
> One minor nit: this should be axCell.

Ok. Given that it's in existing code and not added by this patch, I left that particular one alone. Hope that's ok!(?) But I've already promised Xan I'd go through the Atk a11y stuff with a lint brush. My current list is:

* Trailing whitespace
* NULL
* AXFoo should be axFoo

Bug 30901 has just been created so y'all can add to the list. :-)

> > +static gint webkit_accessible_table_get_column_at_index(AtkTable* table, gint index)
> > +{
> > +    AccessibilityTableCell* AXCell = cellAtIndex(table, index);
> 
> Ditto.

Changed.

Thanks!

[Xan Lopez]

Comment on attachment 41965 [details]
table 3 - corrected as per review

Looks good to me.

[Xan Lopez]

Comment on attachment 42083 [details]
Table 2 - Address Jan's comments

Looks good to me too.

[WebKit Commit Bot]

Comment on attachment 41965 [details]
table 3 - corrected as per review

Rejecting patch 41965 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Xan Lopez', '--force']" exit_code: 1
Last 500 characters of output:
, 9 deletions(-)


-------------------------------------------------------------------
patching file WebCore/ChangeLog
Hunk #1 succeeded at 5 with fuzz 3.
patching file WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp
Hunk #1 succeeded at 137 (offset 6 lines).
Hunk #2 succeeded at 1297 (offset 113 lines).
Hunk #3 succeeded at 1321 (offset 99 lines).
Hunk #4 FAILED at 1349.
1 out of 4 hunks FAILED -- saving rejects to file WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp.rej

[Joanmarie Diggs]

(In reply to comment #36)
> (From update of attachment 41965 [details])
> Rejecting patch 41965 from commit-queue.
> 
> Failed to run
> "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> '--reviewer', 'Xan Lopez', '--force']" exit_code: 1
> Last 500 characters of output:
> , 9 deletions(-)
> 
> 
> -------------------------------------------------------------------
> patching file WebCore/ChangeLog
> Hunk #1 succeeded at 5 with fuzz 3.
> patching file WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp
> Hunk #1 succeeded at 137 (offset 6 lines).
> Hunk #2 succeeded at 1297 (offset 113 lines).
> Hunk #3 succeeded at 1321 (offset 99 lines).
> Hunk #4 FAILED at 1349.
> 1 out of 4 hunks FAILED -- saving rejects to file
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp.rej

Table 3 assumed Table 2 (i.e. had these been submitted in the reverse order, I believe the patch would've cleanly applied)

[Eric Seidel (no email)]

Ah yes.  The commit-queue is not smart enough to know what the implicit order of patches on a bug is. :)  It assumes first to last in attachment order...

[WebKit Commit Bot]

Comment on attachment 42083 [details]
Table 2 - Address Jan's comments

Clearing flags on attachment: 42083

Committed r50339: <http://trac.webkit.org/changeset/50339>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

OK.  I'll fix bug 28230 today.  It's kinda silly to close bugs with r+'d patches on them.  We'll start having the opposite problem of people forgetting to close their bugs.  But then again, we already have that problem. :)

[Eric Seidel (no email)]

Comment on attachment 41965 [details]
table 3 - corrected as per review

Since this supposedly will apply cleanly now, i'll cq+ it.

[Joanmarie Diggs]

(In reply to comment #42)
> (From update of attachment 41965 [details])
> Since this supposedly will apply cleanly now, i'll cq+ it.

Thanks Eric!

I just pulled the latest from git master and it does indeed apply cleanly. Hopefully it will pass all tests too. :-)

[WebKit Commit Bot]

Comment on attachment 41965 [details]
table 3 - corrected as per review

Clearing flags on attachment: 41965

Committed r50342: <http://trac.webkit.org/changeset/50342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.