Bug 105214 - AX: Allow requesting the full plain text for an object with textUnderElement()
Summary: AX: Allow requesting the full plain text for an object with textUnderElement()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-17 13:34 PST by Dominic Mazzoni
Modified: 2013-07-10 09:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch proposal (19.80 KB, patch)
2013-07-02 08:25 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (19.98 KB, patch)
2013-07-05 10:43 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.20 KB, patch)
2013-07-10 03:53 PDT, Mario Sanchez Prada
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2012-12-17 13:34:38 PST
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().
Comment 1 Mario Sanchez Prada 2013-07-02 08:25:11 PDT
Created attachment 205917 [details]
Patch proposal

This patch fixes the issue for GTK while (I believe) not disturbing other platforms.
Comment 2 Simon Pena 2013-07-05 10:20:07 PDT
I have just confirmed that fixing this patch gets the test accessibility/listitem-title.html passing.
Comment 3 Mario Sanchez Prada 2013-07-05 10:43:40 PDT
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.
Comment 4 chris fleizach 2013-07-09 17:26:01 PDT
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 = ..
Comment 5 Mario Sanchez Prada 2013-07-10 02:10:25 PDT
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.
Comment 6 Mario Sanchez Prada 2013-07-10 03:53:08 PDT
Created attachment 206377 [details]
Patch proposal

I believe this patch addresses all the issues pointed out before.
Comment 7 Mario Sanchez Prada 2013-07-10 09:21:22 PDT
Committed r152537: <http://trac.webkit.org/changeset/152537>