Bug 25755 - Implement ARIA grid role
Summary: Implement ARIA grid role
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-13 12:46 PDT by chris fleizach
Modified: 2009-05-14 12:56 PDT (History)
1 user (show)

See Also:


Attachments
patch (57.69 KB, patch)
2009-05-13 12:50 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (58.09 KB, patch)
2009-05-13 14:50 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (59.46 KB, patch)
2009-05-13 15:01 PDT, chris fleizach
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2009-05-13 12:46:33 PDT
Need to implement ARIA role="grid
Comment 1 chris fleizach 2009-05-13 12:50:21 PDT
Created attachment 30282 [details]
patch
Comment 2 Beth Dakin 2009-05-13 14:07:38 PDT
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? 
Comment 3 chris fleizach 2009-05-13 14:39:26 PDT
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()

Comment 4 chris fleizach 2009-05-13 14:39:51 PDT
hit commit by accident. still going thru this...
Comment 5 chris fleizach 2009-05-13 14:46:41 PDT
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
Comment 6 chris fleizach 2009-05-13 14:50:02 PDT
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
Comment 7 Beth Dakin 2009-05-13 14:51:55 PDT
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?
Comment 8 chris fleizach 2009-05-13 15:01:22 PDT
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
Comment 9 chris fleizach 2009-05-13 16:45:10 PDT
landed.
http://trac.webkit.org/changeset/43669

now to see if anything breaks
Comment 10 chris fleizach 2009-05-13 16:48:34 PDT
windows failed. working on fix
Comment 11 chris fleizach 2009-05-13 16:51:20 PDT
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
Comment 12 chris fleizach 2009-05-13 16:53:22 PDT
http://trac.webkit.org/changeset/43700
Comment 13 chris fleizach 2009-05-13 16:53:33 PDT
that was for the windows fix
Comment 14 chris fleizach 2009-05-13 17:03:26 PDT
those warnings actually not caused by this check in
Comment 15 Timothy Hatcher 2009-05-14 12:52:06 PDT
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.
Comment 16 chris fleizach 2009-05-14 12:55:28 PDT
alright will file a new bug for that
Comment 17 chris fleizach 2009-05-14 12:56:17 PDT
filed https://bugs.webkit.org/show_bug.cgi?id=25802