Bug 144639 – AX: [ATK] We need to be smarter about flattening and the accessible text implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Given:

  <div>
    <span>Span!</span>
    <h1>Foo</h1>
    <h2>Bar</h2>
    <p>Hello world</p>
  </div>

There is an object of ATK_ROLE_SECTION which implements the accessible text interface and contains the text of all of its children:

  'Span!\nFoo\n\nBar\n\nHello world'

Getting rid of the span results in the ATK_ROLE_SECTION going away.

While the above test case is not a big deal, given a page with a ton of content in a div with a single span, we're doing a lot of work getting all the text. That's bad for performance. Furthermore, for a variety of reasons, Orca is going to have to implement its own caret navigation for WebKitGtk. Having duplicated text (the headings and paragraphs are still there in the tree, correctly implementing AtkText) is really screwing things up. So we need to be smarter about our flattening.

I have some ideas which I'll pursue.

[Radar WebKit Bug Importer]

<rdar://problem/20824771>

[Joanmarie Diggs]

The idea, which I'm pursuing, will be to defer to WebCore Accessibility regarding inclusion of anonymous blocks. What we're doing in ATK at the moment is a heuristic hack. But before we can defer to WebCore, we need to sort out things like bug 144653. (And there will likely be many more such issues.)

[Joanmarie Diggs]

Created attachment 277439 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 277439 [details]
Patch

Chris: When you have a spare moment, please review. Thanks!

[chris fleizach]

Comment on attachment 277439 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-2651
> -    if (node && (node->hasTagName(tdTag) || node->hasTagName(thTag)))

might be worth putting this in a helper method

> Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:91
> +        return renderObject->firstChildSlow() ? IncludeObject : DefaultBehavior;

will this handle inlines? would doing children().size() ? be a better check

[Joanmarie Diggs]

Created attachment 277554 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 277439 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2652
> +        || (m_renderer->isAnonymous() && m_renderer->isTablePart()))

The helper method makes sense, but after making some additional test cases, I discovered that we don't want the anonymous check (fully-formed CSS tables which include "display:table-cell" have a non-anonymous renderer). "is<RenderTableCell>(m_renderer)" seems to be what we need. So I wound up not making the helper method in the new patch.

>> Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:91
>> +        return renderObject->firstChildSlow() ? IncludeObject : DefaultBehavior;
> 
> will this handle inlines? would doing children().size() ? be a better check

Since we're looking at the RenderObject children, it should. And if you mean calling AccessibilityObject::children().size(), is it safe to get the children of the AccessibilityObject in the method being used to determine if that AccessibilityObject should be included or ignored? Also, I don't think we want to base our decision on what AccessibilityObject children are present. (Though perhaps I am missing something.)

All of that said, your question led me to make additional test cases in order to see where WebCore Accessibility failed to include a paragraph element I'd expect included. And the only place I've found a difference is when that paragraph element's renderer has a RenderBlock child. So the new check looks explicitly for that condition and leaves the rest to WebCore Accessibility. And the new patch has more paragraph tests.

[WebKit Commit Bot]

Comment on attachment 277554 [details]
Patch

Clearing flags on attachment: 277554

Committed r200188: <http://trac.webkit.org/changeset/200188>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.