Bug 79080 – Row and column headers not exposed by WebCore accessibility

• Status: NEW
• Component: Accessibility

[Aaron Leventhal]

Created attachment 127921 [details]
Variety of HTML table markup combinations to be tested for correct exposure of row/column header role 

Table cells which use <th> and associated markup are not being exposed as row/column headers. 

This is fundamental to tables, and getting the roles right likely makes bug 15138 and bug 30799 easier to fix.

[Aaron Leventhal]

Expected: Expose RowHeaderRole and ColumnHeaderRole as appropriate for <th> elements.

The row/colum-ness of the header can be indicated by @role, @scope, <thead> or (heuristically) by it's position within the table.

[Aaron Leventhal]

Created attachment 127926 [details]
Implement row & col header role support

    1. Implement AccessibilityTableCell so that it uses semantics or reasonable heuristics to determine whether a <th> is
    2. Add comments to clarify what some of the table methods do
    3. Move hasAriaRole() to where it can help multiple classes.
    4. Add another helper, hasAriaLandmarkRole() -- landmark roles are weak roles which should not affect the accessibilit

[Dominic Mazzoni]

Unofficially looks on the right track to me - needs a patch in the correct format and a layout test. Be sure to check both LayoutTests/accessibility and LayoutTests/platform/*/accessibility for existing tests that may need to be updated.

There's no aria "table", you should say aria "grid".

Fix typo in comment you're modifying: Do not consider it a data table is it -> Do not consider it a data table if it

Why did you move hasAriaLandmarkRole to AccessibilityRenderObject? It's not clear you're using it there.

Is it possible for if (!cellElement) to return false? If not, add a notreached assertion?

[Aaron Leventhal]

Created attachment 131187 [details]
 Same approach, but fix Dominic's nits, add tests. Note: I kept hasAriaLandmarkRole() where it is because 1) I believe we'll need it in other places, as a landmark real

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


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

[Aaron Leventhal]

Created attachment 131190 [details]
UTF8 encoding for patch, hopefully this helps

[Aaron Leventhal]

Created attachment 131191 [details]
ChangeLog entries are required as well

[Aaron Leventhal]

Created attachment 131192 [details]
Remove alteration of prepare-ChangeLog script

[chris fleizach]

Comment on attachment 131192 [details]
Remove alteration of prepare-ChangeLog script

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

> LayoutTests/ChangeLog:9
> +        * accessibility/table-headers.html: Added.

These tests, test too many things and are unclear in their results. 
Dumping the whole AX tree makes it impossible to figure out what's happening in the test.
You should make specific calls to verify that specific information is correct.
Also, i think there should be a separate test per <table> because they test different things. It will make it much easier to debug future problems

What platforms does this test actually work on?

> Source/WebCore/ChangeLog:5
> +

they are exposed just fine on the Mac, through AXColumnHeaderUIElements and AXRowHeaderUIElements. This change log needs more information as to what platform its affecting and what is actually missing.

> Source/WebCore/accessibility/AccessibilityObject.h:351
> +    // accessible table interface.

this comment could be on one line

> Source/WebCore/accessibility/AccessibilityObject.h:355
>      virtual bool isDataTable() const { return false; }

as far as i can tell, isDataTable() does not need to be on AccessibilityObject. I see it only used within AccessibilityTable. we can move this method to AccessibilityTable. 
The double negative comment is hard to understand. 
The comment should read "Returns true if heuristics determine the HTML table appears to be exposing a grid of data, and not used for layout purposes."

> Source/WebCore/accessibility/AccessibilityObject.h:358
> +    // Return true if something is a cell, a rowheader or a columnheader

comments need periods

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1737
> +        return false;

checking m_renderer isn't important here. there's nothing in this method that uses m_renderer. ariaRoleAttribute() takes care of that

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1739
> +    AccessibilityRole ariaRole = ariaRoleAttribute();

this can all be made into a one line function

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3289
> +{

i don't think you need this method.
bool AccessibilityObject::isLandmark() const
already exists

> Source/WebCore/accessibility/AccessibilityRenderObject.h:275
> +    

both of these do not need to be on the render object. they should be on AccessibilityObject

> Source/WebCore/accessibility/AccessibilityTable.cpp:86
>          return false;

this line makes it seem like to me that something can still be a table even if it's role is a landmark role. that seems wrong

> Source/WebCore/accessibility/AccessibilityTable.h:91
> +    // Return true if it is viable to expose an a11y table interface for

a11y is not a word. please use a real word.

> Source/WebCore/accessibility/AccessibilityTable.h:94
>      bool isTableExposableThroughAccessibility() const;

this comment is wrong. this is a private method to determine if the table should be exposed as a table interface. isAccessibilityTable() returns the result of this method (which is stored in a bool)

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:-98
> -    return CellRole;

There is a lot of work that is being performed below. the result should be cached whether it's a Cell, ColumnHeader or RowHeader, or if it should ask AccessibilityRenderObject::roleValue() for the answer so that this does not need to be calculated every time

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:101
> +    static_cast<HTMLTableCellElement*>(node());

this line doesn't need to be on two lines

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:103
> +        ASSERT_NOT_REACHED();

this seems like a useless check. when would cellElement ever == 0? i suppose only if node() was nil, so it seems like you should just check if node() == 0 and return

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:134
> +    // based on the position of the <th> in the table

comments should end with periods.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:152
> +         static_cast<AccessibilityTable*>(parentTable());

you should use toAccessibilityTable() instead of this and this can be on one line

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    Element* nextCellElt = static_cast<Element*>(nextCellInRow->node());     

what's an "Elt"

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    if (!nextCellElt) {

why are you casting to Element? hasTagName exists on node()

> Source/WebCore/accessibility/AccessibilityTableCell.h:44
> +    // Return true if either table cell or column/row header

this should be a full sentence

> Tools/Scripts/prepare-ChangeLog:65
> +use lib "Tools/Scripts/";

I doubt this fix requires a change to prepare-Changelog. if it does, it should be in a separate patch

[Aaron Leventhal]

Comment on attachment 131192 [details]
Remove alteration of prepare-ChangeLog script

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

>> Source/WebCore/accessibility/AccessibilityTable.cpp:86
>>          return false;
> 
> this line makes it seem like to me that something can still be a table even if it's role is a landmark role. that seems wrong

Hi Chris. What about a data table used in a sidebar?
<table role="complementary">some data</table>
We should come to an agreement about this. At least in Mozilla, we always took the position that a landmark role didn't affect the API role exposed. It's just a navigation hint which complements the other semantics present.

There are a number of legit use cases here.
For example, <textarea role="search"> or <ul role="navigation">. 

Unfortunately, looking at http://www.w3.org/TR/wai-aria-implementation/#mapping_role -- I think the original intent of the spec has been lost since I left the group. I'll have to look into it. For now I'm fine with removing this code. Let me know what you think.

[James Craig]

Hybridized roles make for ambiguous implementations and authoring. The current version of the spec is pretty clear that an element should have only one role, landmark or otherwise, so it can either be a list, or navigation, but not both.

That said, you can easily get the result you're looking for by replacing this:

	<table role="complementary">…
	<ul role="navigation">…

With this:

	<div role="complementary"><table>…
	<nav role="navigation"><ul>…

[Aaron Leventhal]

Created attachment 132849 [details]
Table with multi-layer column headers.

[Aaron Leventhal]

Thanks, let's put the landmark issue to the side.

My next question is about the current table header implementation in WebKit. Are there any plans to allow >1 row header per row or >1 column header per row? I've attached an example, titled "Table with multi-layer column headers".

[chris fleizach]

(In reply to comment #13)
> Thanks, let's put the landmark issue to the side.
> 
> My next question is about the current table header implementation in WebKit. Are there any plans to allow >1 row header per row or >1 column header per row? I've attached an example, titled "Table with multi-layer column headers".

I think we can do the work in WebCore to support it. It looks like on the mac side, there's no API to allow this yet, so we'd have to do some special work to make it work on the Mac.

[Aaron Leventhal]

(In reply to comment #14)
> (In reply to comment #13)
> > Thanks, let's put the landmark issue to the side.
> > 
> > My next question is about the current table header implementation in WebKit. Are there any plans to allow >1 row header per row or >1 column header per row? I've attached an example, titled "Table with multi-layer column headers".
> 
> I think we can do the work in WebCore to support it. It looks like on the mac side, there's no API to allow this yet, so we'd have to do some special work to make it work on the Mac.

Do you need me to file a bug for it? If it's planned, then we may as well reuse the existing table header code in determining the appropriate table header role for other platforms.

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Thanks, let's put the landmark issue to the side.
> > > 
> > > My next question is about the current table header implementation in WebKit. Are there any plans to allow >1 row header per row or >1 column header per row? I've attached an example, titled "Table with multi-layer column headers".
> > 
> > I think we can do the work in WebCore to support it. It looks like on the mac side, there's no API to allow this yet, so we'd have to do some special work to make it work on the Mac.
> 
> Do you need me to file a bug for it? If it's planned, then we may as well reuse the existing table header code in determining the appropriate table header role for other platforms.

We probably need a bug for it

[Aaron Leventhal]

Chris, there is a block of code for ARIA tables in AccessibilityTableColumn::headerObject()

However, there is no corresponding ARIA table code in AccessibilityTableRow::headerObject()

Is that on purpose?

[chris fleizach]

(In reply to comment #17)
> Chris, there is a block of code for ARIA tables in AccessibilityTableColumn::headerObject()
> 
> However, there is no corresponding ARIA table code in AccessibilityTableRow::headerObject()
> 
> Is that on purpose?

AccessibilityARIAGridRow is the version of the table row used when an ARIA table is encountered

[Aaron Leventhal]

Thanks Chris. I'm trying to keep changes minimal, and reuse the existing code. But I either have to reorganize the existing code a bit or we end up with extra iterations for every cell.

For example, I can implement
AccessibilityTableColumn::isHeaderObject()
and call it on initialization of a cell so that the role is properly set.
However, for AccessibilityTableColumn::isHeaderObject() it will end up iterating through the same <thead> or <tbody> cells for each item in the column.

What do you think?

It might be better to make each cell responsible for calculating and caching its role. Then AccessibilityTableColumn::headerObject() could be dumb, and simply iterate through the column and check the role until the ColumnHeaderRole is found, similar to what it does for ARIA tables. 

Advantages:
- Better performance
- It would be easy to later change it to support multiple row and column headers.

Disadvantage:
- Bigger change

[Aaron Leventhal]

Created attachment 133905 [details]
1) Address review comments, 2) Do work to determine cell role only once, 3) Separate/clarify tests and provided -expected files for Chromium-win port

I'm having a bit of trouble using uploat-webkit-patch and prepare-changeLog from Windows, so for now here are the comments inline.

Ensure that the table header roles (row/column) are exposed correctly. Neither IAccessible2 nor ATK/AT-SPI use something like AXColumnHeaderUIElements/AXRowHeaderUIElements ... Windows/Linux ATs expect the role to provide this information.

At first I tried to use the results of AccessibilityTable[Row|Column]->headerObject() but given that the role needed to be const I ran into dependency issues getting the information at cell construction time. For bug 81681 (multiple row/column headers per cell) it might make sense to reuse the information from the role calculation for AXColumnHeaderUIElements and AXRowHeaderUIElements.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/accessibility/table-headers-ar..." exit_code: 1
Source/WebCore/accessibility/AccessibilityTableCell.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/accessibility/AccessibilityTableCell.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/accessibility/AccessibilityTableCell.h:60:  m_table_cell_role is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 15 files


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

[Aaron Leventhal]

Created attachment 133916 [details]
1) Address review comments, 2) Do work to determine cell role only once, 3) Separate/clarify tests and provided -expected files for Chromium-win port

Updated patch to fix style checker issues. Other comments from previous patch still apply.

[Dominic Mazzoni]

Comment on attachment 133916 [details]
1) Address review comments, 2) Do work to determine cell role only once, 3) Separate/clarify tests and provided -expected files for Chromium-win port

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

> LayoutTests/platform/chromium-win/Accessibility/table-headers-aria-div-grid-expected.txt:1
> +PASS successfullyParsed is true

I'm pretty sure the expectations can be in platform/chromium, not platform/chromium-win

Since you don't expect (most of?) these tests to pass on Mac, you need to add them to platform/mac/test_expectations.txt - mark them as SKIPPED with a comment saying Mac doesn't have column and row header roles.

Alternatively, the expectations could be added for Mac but they wouldn't be very interesting - they'd just give "Cell" as the role for everything.

[chris fleizach]

Comment on attachment 133916 [details]
1) Address review comments, 2) Do work to determine cell role only once, 3) Separate/clarify tests and provided -expected files for Chromium-win port

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

there's no change logs in this patch.

There's a method in js-pre that allows you to output the purpose of the test. you'll see that in the platform/mac/accessibility tests. I would add those to each of the tests, so it's clear what the test is for (and where to find that purpose)

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:15
> +        <span role="gRidcell">Cell</span>

strange capitalization here

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:35
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

too much whitespace after table

> LayoutTests/accessibility/table-headers-aria-table-grid.html:21
> +        <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:26
> +        <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:35
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

too much white space

> LayoutTests/accessibility/table-headers-heuristic-column-only.html:29
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

spacing

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:118
> +    // 1. If grandparent is a <thead> this is a column header, for both <th> and <td>

I would remove the --- from the comment as well as the empty line. There's nothing extraordinary about this comment

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:122
> +    ASSERT(tr->hasTagName(trTag)); // DOM has element even if not in doc.

place comment before the assert line. also that comment does not explain why you need to assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:129
> +    // 2. All <td> elements not inside a <thead> are cells 

comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:130
> +    if (currentNode->hasTagName(tdTag))

this is the 2nd time you called hasTagName(tdTag). you should probably cache it

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:135
> +    // 3. Check scope attribute

ditto for this comment block. comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:153
> +        return ColumnHeaderRole; // First row, but not first col.

i would put comment above if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:155
> +        return RowHeaderRole; // First col, but not first row.

i would put comment before if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    AccessibilityTable* parentTableAcc = toAccessibilityTable(parentTable());

the name parentTableAcc is strange to me because it abbreviates Accessibility in a non-standard way in webkit. i would just call it parentTable or parentTableObject

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    ASSERT(parentTableAcc); // Cannot be null if isTableCell().

comment before Assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:171
> +        parentTableAcc->cellForColumnAndRow(nextColInRow, rowRange.first);

does not seem like this needs to be on two lines

[Zoë Bijl]

Has there been any work done on this ticket? It seems that row/column headers are still not properly exposed in 8.0.6 (10600.6.3).

[Radar WebKit Bug Importer]

<rdar://problem/21853190>

[Zoë Bijl]

Created attachment 256899 [details]
Accessibility Inspector displays information about a table's column header element.

[Zoë Bijl]

Comment on attachment 256899 [details]
Accessibility Inspector displays information about a table's column header element.

Never mind.