[Gtk] Layout tables should indicate that they are not data tables via an object attribute

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

The fix for bug #35418 causes "layout tables" (i.e. tables which are not "data tables") to be exposed to Gtk ATs as tables. This is consistent with how HTML content is presented to ATs by Gecko and with what ATs on the GNOME desktop expect. However, it places the burden on the ATs to determine if a table should be treated as a layout table or a data table.

The way this has been handled by Gecko is via an object attribute associated with the accessible table: layout-guess. If Gecko believes that a table is a "layout table", it sets the value of layout-guess to 'true'. If Gecko believes that a table is a "data table", it does not associate the layout-guess object attribute with the table. The AT can then take this under advisement when determining how to treat the table's contents.

I don't see any reason why we should not follow Gecko's lead in this case.

[Joanmarie Diggs]

Created attachment 49566 [details]
test case: layout table

For this table, the value of the 'layout-guess' object attribute for the table should be set to 'true'.

[Joanmarie Diggs]

Created attachment 49567 [details]
test case: data table

For this table, there should not be any 'layout-guess' object attribute associated with the table.

[Joanmarie Diggs]

Calling dibs on this one. (I don't have edit bug privs.)

[Mario Sanchez Prada]

Created attachment 71628 [details]
Patch proposal + Unit test

I'm attaching a patch that would fix this issue, along with an unit test to check it's behaviour.

The ChangeLog entry included is pretty descriptive and extense so I'd just like to explain here why I didn't include a Layout test:

  1. The code I'm changing doesn't really add new functionality other than splitting the behavior of the former isDataTable() function in two: isAccessibilityTable() and a more coherent version of the former isDataTable() function. It's more like a reorganization of the code to allow asking those two questions separately, where the new isAccessibilityTable() function is basically the same than the old isDataTable() in all the ports but the GTK one.

  2. Such differentiation is only used now in the GTK port (the patch makes sure the rest of the ports keep working exactly as usual), and that change is tested by the unit test provided along with this patch.

  3. I ran all the Layout tests with this patch and they keep passing, confirming the former behaviour keeps working (after replacing isDataTable() with isAccessibilityTable() where needed).

Because of these reasons, I thought a layout test was not actually needed. Now head to the ChangeLog entry if you want more, deeper, details on this patch.

Have a nice weekend!

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
>   1. The code I'm changing doesn't really add new functionality other than splitting the 
>  behavior of the former isDataTable() function in two: isAccessibilityTable() and a more 
>  coherent version of the former isDataTable() function. It's more like a reorganization of the 
>  code to allow asking those two questions separately, where the new isAccessibilityTable() 
>  function is basically the same than the old isDataTable() in all the ports but the GTK one.
> 
>   2. Such differentiation is only used now in the GTK port (the patch makes sure the rest of 
>  the ports keep working exactly as usual), and that change is tested by the unit test provided 
>  along with this patch.
>  [...]

Even thouugh the changes in WebCore a11y code are really simple (I basically splitted a function in two, but the non-GTK ports keep working exactly as they did before) I think it might be a good idea to get this patch reviewed by someone from Apple. And when I join Apple + a11y code, Chris Fleizach cames to my mind :-). So adding him to CC. 

Feeling like reviewing this patch then, Chris? O:-)

[chris fleizach]

Comment on attachment 71628 [details]
Patch proposal + Unit test

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

i agree with the refactoring. just have some questions inline

> WebCore/accessibility/AccessibilityTable.cpp:87
> +    return m_isAccessibilityTable;

where does m_isAccessibilityTable get set now that isTableExposableThroughAccessibility() is const

> WebCore/accessibility/AccessibilityTable.cpp:95
> +    // don not consider it a data table is it has an ARIA role

spelling. end sentence with period. capitalize start of sentence

> WebCore/accessibility/AccessibilityTable.cpp:100
> +    // Only "data" tables should be exposed as tables.

sentence should start with capital letter

> WebCore/accessibility/AccessibilityTable.cpp:258
> +

fix this comment (even though it's not yours to look more like a real sentence

> WebCore/accessibility/AccessibilityTable.cpp:263
> +    // expose it as a table, unless, of course, the aria role is a table

ditto

> WebCore/accessibility/AccessibilityTable.cpp:266
> +

does this still account for role="table"?

> WebCore/accessibility/AccessibilityTable.cpp:272
> +

you can probably do this in one line

return tableNode && tableNode->hasTagName(tableTag)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:336
> +    if (coreObject) {

you should use an early return here if !coreObject)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:344
> +        // Technologies know when an exposed table is not data table

end the sentence with a period

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:345
> +        if (coreObject->isAccessibilityTable() && !coreObject->isDataTable()) {

not sure why you want an AXTable, but not a dataTable... wouldn't that mean the layout-guess is false, because you know it's an accessibility table.
can you explain to me why this is right?

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:347
> +            attributeSet = addAttributeToSet(attributeSet, "layout-guess", value.utf8().data());

can you not do 


attributeSet = addAttributeToSet(attributeSet, "layout-guess", "true");
?

[Mario Sanchez Prada]

Created attachment 71861 [details]
Patch proposal + Unit test

Attaching a new patch

(In reply to comment #6)
> (From update of attachment 71628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71628&action=review
> 
> i agree with the refactoring. just have some questions inline

That's a good start!

> > WebCore/accessibility/AccessibilityTable.cpp:87
> > +    return m_isAccessibilityTable;
> 
> where does m_isAccessibilityTable get set now that isTableExposableThroughAccessibility() is const

It's still set in the same place than before, in the constructor:

   AccessibilityTable::AccessibilityTable(RenderObject* renderer)
       : AccessibilityRenderObject(renderer),
       m_headerContainer(0)
   {
   #if ACCESSIBILITY_TABLES
       m_isAccessibilityTable = isTableExposableThroughAccessibility();
   #else
       m_isAccessibilityTable = false;
   #endif
   }

Other than that, is also redefined in subclasses, such as AccessibilityARIAGrid.

> > WebCore/accessibility/AccessibilityTable.cpp:95
> > +    // don not consider it a data table is it has an ARIA role
> 
> spelling. end sentence with period. capitalize start of sentence

Fixed. Sorry.

> > WebCore/accessibility/AccessibilityTable.cpp:100
> > +    // Only "data" tables should be exposed as tables.
> 
> sentence should start with capital letter

Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:258
> > +
> 
> fix this comment (even though it's not yours to look more like a real sentence

No problem. Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:263
> > +    // expose it as a table, unless, of course, the aria role is a table
> 
> ditto

Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:266
> > +
> 
> does this still account for role="table"?

Not sure what you mean here... what I can tell is that it basically keeps the same behaviour than before, but I'm afraid I didn't enter in such a level of detail to answer this question :-/

> > WebCore/accessibility/AccessibilityTable.cpp:272
> > +
> 
> you can probably do this in one line
> 
> return tableNode && tableNode->hasTagName(tableTag)

Sure. Fixed.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:336
> > +    if (coreObject) {
> 
> you should use an early return here if !coreObject)

Fixed.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:344
> > +        // Technologies know when an exposed table is not data table
> 
> end the sentence with a period

Fixed.
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:345
> > +        if (coreObject->isAccessibilityTable() && !coreObject->isDataTable()) {
> 
> not sure why you want an AXTable, but not a dataTable... wouldn't that mean the layout-guess is false, because you know it's an accessibility table.
> can you explain to me why this is right?

Yes. The point is that in the GTK port we expose to accessibiltity every table, no matter it's a layout or a data table, so isAccessibilityTable() is going to return always 'true' as long as the coreObject is an isntance of AccessibilityTable. However, and this is the main reason behind splitting the former isDataTable() function, we need to know when those accessibility tables are not data tables, according to the heuristic used. When that happens we just set an extra 'layout-guess' attribute to 'true' so ATs can treat them like that (if you have more doubts about why to use this mechanism instead of doing it in other way, I guess Joanmarie'd better answer that).

Hope this helps.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:347
> > +            attributeSet = addAttributeToSet(attributeSet, "layout-guess", value.utf8().data());
> 
> can you not do 
> 
> 
> attributeSet = addAttributeToSet(attributeSet, "layout-guess", "true");
> ?

Definitely. Fixed

Asking for review again, then.

[chris fleizach]

Comment on attachment 71861 [details]
Patch proposal + Unit test

r=me

[WebKit Commit Bot]

Comment on attachment 71861 [details]
Patch proposal + Unit test

Clearing flags on attachment: 71861

Committed r70554: <http://trac.webkit.org/changeset/70554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mario Sanchez Prada]

(In reply to comment #8)
> (From update of attachment 71861 [details])
> r=me

Thanks Chris. From now on, there's no need for you to set the cq+ flag anymore, as I already got commit permissions granted, so I can use webkit-patch myself \o/

[Eric Seidel (no email)]

Many of us us the CQ anyway. :) But you're always welcome to commit things "by hand" if you prefer.

[Mario Sanchez Prada]

(In reply to comment #12)
> Many of us us the CQ anyway. :) But you're always welcome to commit things "by hand" if you prefer.

You mean using webkit-patch safe land? I'm new here and completely open-minded to suggestions. And using the commit-queue does sound good to me as well (I just did it this way as I thought it was the right way)