Bug 45383 – [Gtk] Incorrect exposure of list items whose children are elements

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Created attachment 66882 [details]
test case

As a result of the fix for bug #45190, the bullet/number of a list item was made part of the text. (Thanks!) This change seems to have had an unintended side effect for list items whose children are elements.

Steps to reproduce:
1. Open the attached test case.
2. In Accerciser, examine the accessible hierarchy.

Prior to the fix for bug #45190, each child of the list was a list item. Now, the list item whose immediate child is a paragraph is exposed to us as a paragraph. (And the bullet seems to be missing.)

[Mario Sanchez Prada]

(In reply to comment #0)
> [...]
> Prior to the fix for bug #45190, each child of the list was a list item. Now, the list item whose 
> immediate child is a paragraph is exposed to us as a paragraph. (And the bullet seems to be 
> missing.)

I was able to reproduce this, but also with the patch 45190 reverted so the problem must be somewhere else. Keep looking...

[Mario Sanchez Prada]

It looks this could be related to stuff done for bug 27085

[Mario Sanchez Prada]

Created attachment 67918 [details]
Patch proposal

After some work I think the most simple way to fix this is by just forcing inclusion in accessibility of the list items in the platform-dependant part of the accessibilityIsIgnored() mechanism, as I found the behaviour of not exposing the list item when its immediate child is a non inline object is the expected behaviour, as coded in AccessibilityRenderObject::accessibilityIsIgnored():

    [...]
        
    if (m_renderer->isBlockFlow() && m_renderer->childrenInline())
        return !toRenderBlock(m_renderer)->firstLineBox() && !mouseButtonListener();
    
    [...]

As that part of code is what it's making the difference between a <li>plain text</li> item and a <li><p>paragraph text</p></li> item, I thought the best way to fix this for GTK without bothering other ports was the exposed above.

Then there's an ugly part in the patch that is required to deal with the special case of the list item markers, which are not exposed to AT's but must be taken into account anyway when considering offets and those kind of things.

Hence, now asking for review. Shoot your questions, I'm eager to continue working on this patch.

[Mario Sanchez Prada]

Btw, while working on this bug I found another strange behaviour with the markers of items with non inline children so I filed this bug:

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

[chris fleizach]

Comment on attachment 67918 [details]
Patch proposal

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

> WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:76
> +    // Include all list items, regardless they have or not inline children

seems like you already have "role" here, so you shouldn't have to call roleValue() again

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2031
> +            if (role != TableRole && axRenderObject->renderer()->childrenInline())

you should verify renderer() is not nil before accessing inline children

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2036
> +                AccessibilityObject* axRenderChild = axRenderObject->firstChild();

Can't you just access ->children() of the axRenderObject and then check if it has an item in it? that would be the first accessible element.

[Mario Sanchez Prada]

Created attachment 68206 [details]
Patch proposal

(In reply to comment #5)
> (From update of attachment 67918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67918&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:76
> > +    // Include all list items, regardless they have or not inline children
> 
> seems like you already have "role" here, so you shouldn't have to call roleValue() again

Done and also fixed the same mistake in the next line (introduced some commits ago).

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2031
> > +            if (role != TableRole && axRenderObject->renderer()->childrenInline())
> 
> you should verify renderer() is not nil before accessing inline children

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2036
> > +                AccessibilityObject* axRenderChild = axRenderObject->firstChild();
> 
> Can't you just access ->children() of the axRenderObject and then check if it has an item in it? that would be the first accessible element.

Done.

[chris fleizach]

Comment on attachment 68206 [details]
Patch proposal

can we get a layout test for this behavior. this has the appearance that if something upstream in WebCore changed, it could cause breakage.

[Mario Sanchez Prada]

Created attachment 68261 [details]
Patch proposal

(In reply to comment #7)
> (From update of attachment 68206 [details])
> can we get a layout test for this behavior. this has the appearance that if something upstream in WebCore changed, it could cause breakage.

Attaching a new version of the patch with a Layout Test (GTK specific) to check that list items are always exposed like that

[chris fleizach]

Comment on attachment 68261 [details]
Patch proposal

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

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2038
> +                if (children.size() > 0) {

sorry...

this should just be
if (children.size())

[Mario Sanchez Prada]

Created attachment 68289 [details]
Patch proposal

(In reply to comment #9)
> (From update of attachment 68261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68261&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2038
> > +                if (children.size() > 0) {
> 
> sorry...
> 
> this should just be
> if (children.size())

No problem. Attaching a new patch with that issue fixed.

[chris fleizach]

Comment on attachment 68289 [details]
Patch proposal

r=me

[WebKit Commit Bot]

Comment on attachment 68289 [details]
Patch proposal

Clearing flags on attachment: 68289

Committed r68023: <http://trac.webkit.org/changeset/68023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.