Bug 25755 – Implement ARIA grid role

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Need to implement ARIA role="grid

[chris fleizach]

Created attachment 30282 [details]
patch

[Beth Dakin]

Overall this patch looks good. My concern is with all of the static_casts in the AccessibilityTable classes -- both the casts that are added with the patch and the ones that already existed. Now that these AccessibilityGrid classes inherit from AccessibilityTable classes and implement the ARIA role, it seems to me that there is no guarantee that the renderers will be table parts as they are often cast to be. Are you sure these casts are still safe? If so, why? 

[chris fleizach]

i'm not a C++ wrangler yet.

If i do this
static_cast<AccessibilityTableRow*>(m_rows[k].get())->headerObject();
I assume it uses the virtual method to get the right one, so i'm guessing those are not the casts you're talking about

i assume you're talking about these casts
RenderTable* table = static_cast<RenderTable*>(m_renderer);

So in those cases, WebKit would crash pretty fast if the thing wasn't a RenderTable. To counter those, i've carefully looked at where those are used across the three classes and i have virtual methods that override each one

here are the classes that use static casts and why this patch doesn't interfere
AccessibilityTable::isTableExposableThroughAccessibility()

[chris fleizach]

hit commit by accident. still going thru this...

[chris fleizach]

AccessibilityTable::isTableExposableThroughAccessibility()
  -- returns as soon as it sees the renderer is not a RenderTable

AccessibilityTable::addChildren()
  -- overridden by AccessibilityAriaGrid 

AccessibilityTableCell* AccessibilityTable::cellForColumnAndRow
  -- overriden by AccessibilityAriaGrid

AccessibilityTable::title
   -- this assumes a HTMLTableElement node... i need to protect against this and will do so, and update the patch

AccessibilityTableColumn::headerObject()
   -- code that first checks if the parent table is an aria table and handles that separately. after that check, it then checks if it is a RenderTable before proceeding

AccessibilityTableCell::parentTable()  and AccessibilityTableCell::rowIndexRange,  and AccessibilityTableCell::columnIndexRange
  -- overridden by AccessibilityAriaGridCell

AccessibilityTableCell::titleUIElement()
  -- checks that the renderer is a RenderTableCell before proceeding

AccessibilityTableRow::parentTable() and AccessibilityTableRow::headerObject()
 -- overriden by AccessibilityAriaGridRow

[chris fleizach]

Created attachment 30288 [details]
patch

this patch makes sure that in AccessibilityTable, it checks that the Node is a HTMLTableElement before proceeding to take the caption as the title

[Beth Dakin]

Okay, that breakdown is really helpful, thanks Chris. Based on the breakdown, it looks like there is only one spot where we are currently at risk of crashing, so it is good that you will be patching it.

But for the sake of readable code and a coherent class, I think some of the other spots should be patched as well. The places that check that you are a table or return early if you are an aria table are all fine too. But when you see code that automatically casts a renderer to a RenderTable it could make you believe that any AccessibilityTable's renderer is a RenderTable, which we know is not true now that we have grids implemented. As you said, we won't crash in these spots because all of these functions are overridden in the various Grid classes, but for coherency I think you should patch them with early returns anyway. Does that make sense?

[chris fleizach]

Created attachment 30289 [details]
patch

adds checks to all the places where we do static casts to make sure the element is the correct type

[chris fleizach]

landed.
http://trac.webkit.org/changeset/43669

now to see if anything breaks

[chris fleizach]

windows failed. working on fix

[chris fleizach]

there's a number of gtk warnings. working on those

../../WebCore/page/AccessibilityTable.h:66: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:67: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityRenderObject.cpp:2118: warning: ‘WebCore::createARIARoleMap()::RoleEntry’ declared with greater visibility than the type of its field ‘WebCore::createARIARoleMap()::RoleEntry::ariaRole’
../../WebCore/page/AccessibilityTable.h:66: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:67: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.cpp:353: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.cpp:361: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:66: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:67: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:66: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:67: warning: type qualifiers ignored on function return type
../../WebCore/rendering/RenderTextControl.cpp:390: warning: ‘breakOffset’ may be used uninitialized in this function
../../WebCore/page/gtk/AccessibilityObjectWrapperAtk.cpp:95: warning: ‘WebCore::AccessibilityObject* core(AtkStreamableContent*)’ defined but not used
../../WebCore/page/gtk/AccessibilityObjectWrapperAtk.cpp:169: warning: ‘gint webkit_accessible_get_index_in_parent(AtkObject*)’ defined but not used
../../WebCore/page/AccessibilityTable.h:66: warning: type qualifiers ignored on function return type
../../WebCore/page/AccessibilityTable.h:67: warning: type qualifiers ignored on function return type

[chris fleizach]

http://trac.webkit.org/changeset/43700

[chris fleizach]

that was for the windows fix

[chris fleizach]

those warnings actually not caused by this check in

[Timothy Hatcher]

Comment on attachment 30282 [details]
patch

I'm not the right reviewer for this, but AccessibilityAriaGrid should be AccessibilityARIAGrid.

Per our style guidelines: "Use CamelCase. Capitalize the first letter, including all letters in an acronym, in a class, struct, protocol, or namespace name."

And ARIA is an acronym.

[chris fleizach]

alright will file a new bug for that

[chris fleizach]

filed https://bugs.webkit.org/show_bug.cgi?id=25802