Bug 45383 - [Gtk] Incorrect exposure of list items whose children are elements
Summary: [Gtk] Incorrect exposure of list items whose children are elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2010-09-08 05:18 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2010-09-22 00:49 PDT (History)
4 users (show)

See Also:


Attachments
test case (137 bytes, text/html)
2010-09-08 05:18 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Patch proposal (4.37 KB, patch)
2010-09-17 10:47 PDT, Mario Sanchez Prada
cfleizach: review-
Details | Formatted Diff | Diff
Patch proposal (4.44 KB, patch)
2010-09-21 03:25 PDT, Mario Sanchez Prada
cfleizach: review-
Details | Formatted Diff | Diff
Patch proposal (8.39 KB, patch)
2010-09-21 11:00 PDT, Mario Sanchez Prada
cfleizach: review-
Details | Formatted Diff | Diff
Patch proposal (8.39 KB, patch)
2010-09-21 14:09 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2010-09-08 05:18:43 PDT
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.)
Comment 1 Mario Sanchez Prada 2010-09-16 10:39:56 PDT
(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...
Comment 2 Mario Sanchez Prada 2010-09-17 02:14:29 PDT
It looks this could be related to stuff done for bug 27085
Comment 3 Mario Sanchez Prada 2010-09-17 10:47:24 PDT
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.
Comment 4 Mario Sanchez Prada 2010-09-17 10:48:39 PDT
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
Comment 5 chris fleizach 2010-09-20 11:19:10 PDT
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.
Comment 6 Mario Sanchez Prada 2010-09-21 03:25:46 PDT
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.
Comment 7 chris fleizach 2010-09-21 09:59:45 PDT
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.
Comment 8 Mario Sanchez Prada 2010-09-21 11:00:45 PDT
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
Comment 9 chris fleizach 2010-09-21 13:53:51 PDT
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())
Comment 10 Mario Sanchez Prada 2010-09-21 14:09:43 PDT
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.
Comment 11 chris fleizach 2010-09-21 14:26:49 PDT
Comment on attachment 68289 [details]
Patch proposal

r=me
Comment 12 WebKit Commit Bot 2010-09-22 00:49:44 PDT
Comment on attachment 68289 [details]
Patch proposal

Clearing flags on attachment: 68289

Committed r68023: <http://trac.webkit.org/changeset/68023>
Comment 13 WebKit Commit Bot 2010-09-22 00:49:52 PDT
All reviewed patches have been landed.  Closing bug.