Need to implement ARIA role="grid
Created attachment 30282 [details] patch
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?
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()
hit commit by accident. still going thru this...
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
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
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?
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
landed. http://trac.webkit.org/changeset/43669 now to see if anything breaks
windows failed. working on fix
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
http://trac.webkit.org/changeset/43700
that was for the windows fix
those warnings actually not caused by this check in
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.
alright will file a new bug for that
filed https://bugs.webkit.org/show_bug.cgi?id=25802