Bug 118969 – [ATK] Implement tables-related attributesOf*() functions for AccessibilityUIElement

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Implementing some table-related functions in AccessibilityUIElement would help to run properly the following tests, now in TestExpectations:

  accessibility/table-attributes.html
  accessibility/table-sections.html

The list of functions that should be implemented are:
  - attributesOfColumnHeaders()
  - attributesOfRowHeaders()
  - attributesOfColumns()
  - attributesOfRows()
  - attributesOfHeader()
  - attributesOfVisibleCell()

Additionally, it should help to to provide more accurate (and useful!) results for accessibility/aria-tables.html

[Krzysztof Czech]

Created attachment 216024 [details]
patch

[WebKit Commit Bot]

Attachment 216024 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:439:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:449:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:455:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:374:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:390:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:398:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:399:  Missing space before ( in for(  [whitespace/parens] [5]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 216025 [details]
patch

[Krzysztof Czech]

I provided a proposed patch of missing implementation of table-related* functions. Generally it seems not every function could be implemented (example, attributesOfRows, attributesOfColumns, attributesOfHeader). Looks like columns, rows and header are not exposed in ATK so far.

So I guess we should provide new baselines for:
 accessibility/table-attributes.html
 accessibility/table-sections.html

[Mario Sanchez Prada]

Comment on attachment 216025 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216025&action=review

(In reply to comment #4)
> I provided a proposed patch of missing implementation of table-related* functions. Generally it seems not every function could be implemented (example, attributesOfRows, attributesOfColumns, attributesOfHeader). Looks like columns, rows and header are not exposed in ATK so far.
> 
> So I guess we should provide new baselines for:
>  accessibility/table-attributes.html
>  accessibility/table-sections.html

I think that's right. Please consider my review below though. Thanks!

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:382
> +static void getRowHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& rowHeaders)

This function only works for AtkTable, so I think you can already expect that type as the first parameter and avoid the casts below. Just make sure you check and cast properly to AtkTable at the caller points

Also, I think this is one of those cases where you can use the "move semantics" feature of C++11, which is already supported by Vector through its move contructor Vector(Vector&&). See https://www.webkit.org/blog/3172/webkit-and-cxx11/

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384
> +    for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessible)); ++row)

I think it's better to put the result of get_n_rows() in a variable and use it in the limit condition in the loop, to avoid too many calls to this function.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:388
> +static void getColumnHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& columnHeaders)

Same considerations than before here (and in the rest of the patch)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:400
> +    for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessibilityObject)); ++row)
> +        for (int column = 0; column < atk_table_get_n_columns(ATK_TABLE(accessibilityObject)); ++column)
> +            visibleCells.append(uiElement->cellForColumnAndRow(column, row));

Please use braces for the outermost loop. Also, you can store the get_n_rows() and get_n_columns() in variables too here

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:968
> +    Vector<AccessibilityUIElement> columnHeaders;
> +    getColumnHeaders(m_element, columnHeaders);

If the C++11 thing works, this will become one line :)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:979
> +    Vector<AccessibilityUIElement> rowHeaders;
> +    getRowHeaders(m_element, rowHeaders);

Ditto.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1002
> +    Vector<AccessibilityUIElement> visibleCells;
> +    getVisibleCells(this, visibleCells);

Ditto.

[Krzysztof Czech]

Created attachment 216039 [details]
patch

[Krzysztof Czech]

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:382
> > +static void getRowHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& rowHeaders)
> 
> This function only works for AtkTable, so I think you can already expect that type as the first parameter and avoid the casts below. Just make sure you check and cast properly to AtkTable at the caller points
Good point. Done.
> 
> Also, I think this is one of those cases where you can use the "move semantics" feature of C++11, which is already supported by Vector through its move contructor Vector(Vector&&). See https://www.webkit.org/blog/3172/webkit-and-cxx11/

Very good idea. Done.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384
> > +    for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessible)); ++row)
> 
> I think it's better to put the result of get_n_rows() in a variable and use it in the limit condition in the loop, to avoid too many calls to this function.
> 
You are right. Done.
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:388
> > +static void getColumnHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& columnHeaders)
> 
> Same considerations than before here (and in the rest of the patch)
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:400
> > +    for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessibilityObject)); ++row)
> > +        for (int column = 0; column < atk_table_get_n_columns(ATK_TABLE(accessibilityObject)); ++column)
> > +            visibleCells.append(uiElement->cellForColumnAndRow(column, row));
> 
> Please use braces for the outermost loop. Also, you can store the get_n_rows() and get_n_columns() in variables too here

Right, Done.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:968
> > +    Vector<AccessibilityUIElement> columnHeaders;
> > +    getColumnHeaders(m_element, columnHeaders);
> 
> If the C++11 thing works, this will become one line :)
Done.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:979
> > +    Vector<AccessibilityUIElement> rowHeaders;
> > +    getRowHeaders(m_element, rowHeaders);
> 
> Ditto.
Done.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1002
> > +    Vector<AccessibilityUIElement> visibleCells;
> > +    getVisibleCells(this, visibleCells);
> 
> Ditto.
Done.

Thanks.

[Mario Sanchez Prada]

Comment on attachment 216039 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216039&action=review

Looks good, except some nitpicks that would be great to fix. Same considerations apply to WKTR

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:374
> +    for (Vector<AccessibilityUIElement>::iterator it = elements.begin(); it != elements.end(); ++it) {

Sorry I haven't realized of this first, but as you don't need to modify this object, you can use const_iterator here instead

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:387
> +

I think this blank line is not needed

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:399
> +

Ditto

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:406
> +static Vector<AccessibilityUIElement> getVisibleCells(AccessibilityUIElement* uiElement)

In this file, we normally call this parameter "element", not "uiElement"

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:987
> +

Extra line not needed

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:997
> +

Ditto.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1019
> +

Ditto

[Krzysztof Czech]

Created attachment 216167 [details]
patch

[WebKit Commit Bot]

Attachment 216167 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:596:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:599:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 216168 [details]
patch

[Krzysztof Czech]

Applied all suggestions.

[Mario Sanchez Prada]

Comment on attachment 216168 [details]
patch

Lgtm. Thanks

[WebKit Commit Bot]

Comment on attachment 216168 [details]
patch

Clearing flags on attachment: 216168

Committed r158742: <http://trac.webkit.org/changeset/158742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.