AX: Allow requesting the full plain text for an object with textUnderElement()

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

In order for these tests to pass:

accessibility/button-title-uses-inner-img-alt.html
accessibility/focusable-div.html [ Failure ]

...there need to be more GTK accessibility tests that cover textUnderElement in cases where it should return an object replacement character. Then we can try getting rid of a special case for GTK in AccessibilityRenderObject::textUnderElement().

[Mario Sanchez Prada]

Created attachment 205917 [details]
Patch proposal

This patch fixes the issue for GTK while (I believe) not disturbing other platforms.

[Simon Pena]

I have just confirmed that fixing this patch gets the test accessibility/listitem-title.html passing.

[Mario Sanchez Prada]

Created attachment 206160 [details]
Patch proposal

(In reply to comment #2)
> I have just confirmed that fixing this patch gets the test accessibility/listitem-title.html passing.

That's right. I've already pushed a commit skipping that one and associating it to this bug, so I'm now updating the patch here to include the unskipping of it, once it gets reviewed.

[chris fleizach]

Comment on attachment 206160 [details]
Patch proposal

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

> Source/WebCore/ChangeLog:3
> +        AX: Need more GTK accessibility tests of the object replacement character

This title is misleading I think for what it does

> Source/WebCore/ChangeLog:8
> +        Allow specifying different funtion modes for textUnderElement(),

funtion -> function

> Source/WebCore/ChangeLog:11
> +        element (without omitting any child in the subtree), needed for

the portion of this sentence - ", needed for..." Should be its own sentence

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1509
> +    if (mode == TextUnderElementModeFullText)

I think these modes should be called

TextUnderElementIncludeAllChildren
TextUnderElementSkipIgnoredChildren (Default)

> Source/WebCore/accessibility/AccessibilityRenderObject.h:147
> +    virtual String textUnderElement(AccessibilityTextUnderElementMode) const;

this should take a default value so that we don't need to modify every instances
TextUnderElementMode = ..

[Mario Sanchez Prada]

Thanks for the review, Chris. I will provide an updated patch asap.

(In reply to comment #4)
> (From update of attachment 206160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206160&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        AX: Need more GTK accessibility tests of the object replacement character
> 
> This title is misleading I think for what it does

Yes, that's right. I'm now changing it to something that will hopefully be more accurate

> > Source/WebCore/ChangeLog:8
> > +        Allow specifying different funtion modes for textUnderElement(),
> 
> funtion -> function

Ok.

> > Source/WebCore/ChangeLog:11
> > +        element (without omitting any child in the subtree), needed for
> 
> the portion of this sentence - ", needed for..." Should be its own sentence

Ok.

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1509
> > +    if (mode == TextUnderElementModeFullText)
> 
> I think these modes should be called
> 
> TextUnderElementIncludeAllChildren
> TextUnderElementSkipIgnoredChildren (Default)

Agreed. It definitely sounds better and looks more clear

> > Source/WebCore/accessibility/AccessibilityRenderObject.h:147
> > +    virtual String textUnderElement(AccessibilityTextUnderElementMode) const;
> 
> this should take a default value so that we don't need to modify every instances
> TextUnderElementMode = ..

Ok. I'll add that then.

[Mario Sanchez Prada]

Created attachment 206377 [details]
Patch proposal

I believe this patch addresses all the issues pointed out before.

[Mario Sanchez Prada]

Committed r152537: <http://trac.webkit.org/changeset/152537>