[Webkit-unassigned] [Bug 57463] [GTK] ARIA tables not exposing cells as HTML tables do

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Mario Sanchez Prada <msanchez at igalia.com>  2011-04-08 09:58:44 PST ---
(From update of attachment 88393)
View in context: https://bugs.webkit.org/attachment.cgi?id=88393&action=review

I'll wait for you to answer my question above (about the double check) before actually committing anything.

Thanks!

>> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:110
>> +            addChild(child.get(), appendedRows, columnCount);
> 
> if the ariaRoleAttribute == RowRole, is it not always isTableRow() = true?

Well, the definition of isTableRow() is as follows:

  bool AccessibilityTableRow::isTableRow() const
  {
      AccessibilityObject* table = parentTable();
      if (!table || !table->isAccessibilityTable())
          return false;

      return true;
 }

So my quick answer is "yes, it should be the same", but still I think I'd rather leave the double check to make sure we're actually doing the right thing, since ariaRoleAttribute() just checks an attribute, while isTableRow() checks that the parent object is a table, and I wonder whether those two methods could end up "disagreeing" under certain circunstances on whether something is a row or not.

Besides, I took the idea of the first check in AccessibilityARIAGrid::addChild(), so that's where my doubts come from... 

So what do you think? Shall we change it or leave it as it is now (double check)?

>> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:64
>> +        return 0;
> 
> seems like this check is not necessary since when we ask for the parent unignored, we'll check again for accessibilityTable() a few lines below. 
> I would however add a comment why we need to check parentObjectUnignored(), and the parentObjectUnignored() again

Yes, you're right. I just tried to keep the same logic than before my patch as much as possible, but I agree that it's not needed to make this check now having the second isAccessibilityTable() check right after getting the unignored parent for the second time.

Agreed on adding the comment as well.

>> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:90
>> +        for (unsigned k = 0; k < siblings.size(); ++k) {
> 
> don't call .size() in the for loop (that means you'll call a method for every iteration

Opps. Yes, sorry about that.

>> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:93
>> +                break;
> 
> i should note that if ARIA grid cells do have span columns, this code will be invalid

Yes, you're right. But in theory ARIA grids do not have span columns, right? Or at least that's what I thought... I'll double check that, just in case

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.