VoiceOver cannot navigate the itunes album view table

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

On iOS, VoiceOver cannot navigate the iTunes album view table.

This is a regression from
https://bugs.webkit.org/show_bug.cgi?id=57463

[chris fleizach]

The problem is how rows of an ARIA grid are determined. The mentioned bug was strangely and wrongly only adding children 1 level deep in the hierarchy. This means that it doesn't look any farther for table rows.

[chris fleizach]

Created attachment 96501 [details]
patch

[David Kilzer (:ddkilzer)]

Comment on attachment 96501 [details]
patch

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

r=me with the unsigned => size_t changes.

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:119
> +            unsigned length = children.size();

This should be size_t instead of unsigned.

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:120
> +            for (unsigned i = 0; i < length; ++i)

unsigned => size_t

[chris fleizach]

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

[chris fleizach]

fix the leopard test
http://trac.webkit.org/changeset/88843

[chris fleizach]

I see a GTK test is failing as well. May need Mario's help to fix that one

 AXRole: table
 AXRole: table
 AXRole: table cell
+AXRole: unknown
 AXRole: table cell
-AXRole: table cell
-AXRole: table cell
+AXRole: unknown
 Test passed

[chris fleizach]

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r88840%20(23362)/results.html

[chris fleizach]

rdar://9600922. Also affects the desktop

[Ryosuke Niwa]

It seems like the test is still faling on Leopard.
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r88877%20(33189)/results.html

Could you look into it?

[chris fleizach]

(In reply to comment #9)
> It seems like the test is still faling on Leopard.
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r88877%20(33189)/results.html
> 
> Could you look into it?

looking into why my skipped list change didn't work

[chris fleizach]

Fixed for real
http://trac.webkit.org/changeset/88887

[Mario Sanchez Prada]

(In reply to comment #6)
> I see a GTK test is failing as well. May need Mario's help to fix that one
> 
>  AXRole: table
>  AXRole: table
>  AXRole: table cell
> +AXRole: unknown
>  AXRole: table cell
> -AXRole: table cell
> -AXRole: table cell
> +AXRole: unknown
>  Test passed

It seems the problem in GTK with this patch is that the addChild() method expects a row object with the cells as its children, and doesn't care initially whether that row object is ignoring a11y or not: it will just consider that later on, when deciding whether to add the row object itself to the hierarchy, or just the raw list of children:

  bool AccessibilityARIAGrid::addChild(AccessibilityObject* child, HashSet<AccessibilityObject*>& appendedRows, unsigned& columnCount)
  {
      if (!child || !child->isTableRow() || child->ariaRoleAttribute() != RowRole)
          return false;
        
      [...]

      // Try adding the row if it's not ignoring accessibility,
      // otherwise add its children (the cells) as the grid's children.
      if (!row->accessibilityIsIgnored())
          m_children.append(row);
      else
          m_children.append(row->children());

      appendedRows.add(row);
      return true;
  }

Thus, when we do the following in AccessibilityARIAGrid::addChildren()...

      // The children of this non-row will contain all non-ignored elements (recursing to find them). 
      // This allows the table to dive arbitrarily deep to find the rows.
      AccessibilityChildrenVector children = child->children();
      unsigned length = children.size();
      for (unsigned i = 0; i < length; ++i)
          addChild(children[i].get(), appendedRows, columnCount);
      }

...nothing is being added to the hierarchy in GTK, because navigating through this AccessibilityChildrenVector, as Chris explained in the comment, will mean navigating through non-ignored elements only, which in GTK means just the cells, as rows won't be exposed in that platform (yest, weird, but at the moment it's like that, will change in the future, though [1][2]).

So, the problem I see with this patch, from GTK's selfish point of view, is that it assumes that rows are not being ignored, which is right for the Mac, but not for GTK at this point.

My proposal to keep this regression fixed while still keep all the parties happy, is just to combine the code before and after this patch with #if - #endif  regions, doing something like this:

   #if PLATFORM(GTK)
      // Do not navigate children through the Accessibility children vector to let addChild() check
      // the result of accessibilityIsIgnored() and make the proper decision.
      AccessibilityObject* grandChild = 0;
      for (grandChild = child->firstChild(); grandChild; grandChild = grandChild->nextSibling())
          addChild(grandChild, appendedRows, columnCount);
   #else
     // The children of this non-row will contain all non-ignored elements (recursing to find them). 
     // This allows the table to dive arbitrarily deep to find the rows.
     AccessibilityChildrenVector children = child->children();
     size_t length = children.size();
     for (size_t i = 0; i < length; ++i)
         addChild(children[i].get(), appendedRows, columnCount);
  #endif


Why I propose this ugly thing? Well, because after all this hack is needed because of a very specific requirement in GTK (not exposing rows) that is needed at the moment, but that was already decided it will change in the future (see ATK hackfest agenda and conclusions in [1] and [2]), so when that happens it will be just a matter of removing this #if PLATFORM(GTK) - #endif region and just leaving the generic one Chris wrote, and that should work for all the parties.

As this seems to be a too-specific thing for GTK, I will file a new bug and propose this patch there, keeping the test skipped while it's not fixed.

[1] https://live.gnome.org/Hackfests/ATK2011/Agenda
[2] https://live.gnome.org/Hackfests/ATK2011/Agenda/table

[Mario Sanchez Prada]

(In reply to comment #12)
> [...]
> As this seems to be a too-specific thing for GTK, I will file a new bug and propose this patch there, keeping the test skipped while it's not fixed.

Filed bug 62718 for tracking down this issue. Test skipped in the meanwhile: http://trac.webkit.org/changeset/88919