Bug 44778 – AX: Support AccessibilityTextMarkers in DRT

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

In order to effectively make changes in the text marker system that AX exposes, we need support in DRT to access and manipulate text markers.

This bug is about adding basic coverage to DRT to do some of the common text marker operations

[chris fleizach]

Created attachment 65728 [details]
patch

[WebKit Review Bot]

Attachment 65728 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/DumpRenderTree/AccessibilityTextMarker.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/DumpRenderTree/AccessibilityTextMarker.h:97:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WebKitTools/DumpRenderTree/AccessibilityUIElement.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 65731 [details]
patch

fix style warnings

[WebKit Review Bot]

Attachment 65731 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3831027

[chris fleizach]

Created attachment 65774 [details]
patch

attempt to fix gtk

[WebKit Review Bot]

Attachment 65774 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3824036

[chris fleizach]

Created attachment 65796 [details]
patch

fix build on gtk

[WebKit Review Bot]

Attachment 65796 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3888046

[Darin Adler]

Comment on attachment 65796 [details]
patch

Build is still broken on GTK. Clearing review flag so you can fix that.

[chris fleizach]

Created attachment 65926 [details]
patch

try to fix GTK build failure

[David Kilzer (:ddkilzer)]

Comment on attachment 65926 [details]
patch

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

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2527
> +    // Used only by DumpRenderTree (so far)
Nit: Need period at the end of the sentence.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2528
> +    if ([attribute isEqualToString: @"AXStartTextMarkerForTextMarkerRange"]) {
Nit: No space after "isEqualToString:" and its argument.  (I guess this is the prevailing style in this file, but it should eventually be fixed.)

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2533
> +    if ([attribute isEqualToString: @"AXEndTextMarkerForTextMarkerRange"]) {
Ditto.

> WebKitTools/DumpRenderTree/AccessibilityTextMarker.cpp:49
> +    return JSValueMakeBoolean(context, toTextMarker(thisObject)->isEqual(toTextMarker(otherMarker)));
This method might read better as:

{
    if (argumentCount != 1)
        return JSValueMakeBoolean(context, false);

    JSObjectRef otherMarker = JSValueToObject(context, arguments[0], exception);
    return JSValueMakeBoolean(context, toTextMarker(thisObject)->isEqual(toTextMarker(otherMarker)));
}

> WebKitTools/DumpRenderTree/AccessibilityTextMarker.cpp:103
> +    return JSValueMakeBoolean(context, toTextMarkerRange(thisObject)->isEqual(toTextMarkerRange(otherMarker)));
Same comment as isMarkerEqualCallback().

> WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:727
> +    return 0;
Would it be clearer to say this instead:

    return AccessibilityTextMarkerRange(0);

Maybe the implicit constructor is okay, though.

> WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:750
> +AccessibilityUIElement AccessibilityUIElement::accessibilityElementForTextMarker(AccessibilityTextMarker*)
Should this return a pointer instead of an object?  I guess there is at least one other method that returns a AccessibilityUIElement object.

> WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:690
> +			developmentRegion = English;
This change should not be checked in.

> WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:692
> +			knownRegions = (
This change should not be checked in.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:28
> +#import "AccessibilityTextMarker.h"
Nit: Alphabetize the #Import statements here.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:32
> +#if SUPPORTS_AX_TEXTMARKERS
Not sure if this is needed since it's always defined as 1 for PLATFORM(MAC), but it's fine as-is.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:38
> +        CFRetain(m_textMarker);
It would be nice to use a RetainPtr<>() in AccessibilityTextMarker instead of having to manually retain/release the object in the implementation.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:45
> +        CFRetain(m_textMarker);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:51
> +        CFRelease(m_textMarker);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:66
> +        CFRetain(m_textMarkerRange);
It would be nice to use a RetainPtr<>() in AccessibilityTextMarkerRange instead of having to manually retain/release the object in the implementation.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:73
> +        CFRetain(m_textMarkerRange);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:79
> +        CFRelease(m_textMarkerRange);
Ditto.

> LayoutTests/platform/mac/accessibility/element-for-text-marker.html:13
> +<div id="text1" tabindex=0>text block</div>
Nit:  tabindex="0"

> LayoutTests/platform/mac/accessibility/element-for-text-marker.html:21
> +    description("This tests the text marker system will return the right element when given a text marker");
Nit:  Change "right element" to "correct element".
Nit:  Add a period to the end of the sentence.

> LayoutTests/platform/mac/accessibility/text-marker-length.html:12
> +<div id="text" tabindex=0>text block</div>
Nit:  tabindex="0"

r=me but consider the comments above.

[chris fleizach]

Will make all changes possible

> > WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:727
> > +    return 0;
> Would it be clearer to say this instead:
> 
>     return AccessibilityTextMarkerRange(0);
> 
> Maybe the implicit constructor is okay, though.
>

leaving as is, since the rest of the file follows that format.
 
> > WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:750
> > +AccessibilityUIElement AccessibilityUIElement::accessibilityElementForTextMarker(AccessibilityTextMarker*)
> Should this return a pointer instead of an object?  I guess there is at least one other method that returns a AccessibilityUIElement object.
> 

The rest of the file does not return pointers, so leaving as is. will consider changing that in the future.

[chris fleizach]

> > WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:38
> > +        CFRetain(m_textMarker);
> It would be nice to use a RetainPtr<>() in AccessibilityTextMarker instead of having to manually retain/release the object in the implementation.
> 

Since m_textMarker is an id, RetainPtr is not working. were you thinking of something besides

RetainPtr<PlatformTextMarkerRange> m_textMarkerRange;

[chris fleizach]

(In reply to comment #13)
> > > WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:38
> > > +        CFRetain(m_textMarker);
> > It would be nice to use a RetainPtr<>() in AccessibilityTextMarker instead of having to manually retain/release the object in the implementation.
> > 
> 
> Since m_textMarker is an id, RetainPtr is not working. were you thinking of something besides
> 
> RetainPtr<PlatformTextMarkerRange> m_textMarkerRange;

Figured out the RetainPtr stuff now. should be good to go

[chris fleizach]

http://trac.webkit.org/changeset/67095

[chris fleizach]

fix GTK and win
http://trac.webkit.org/changeset/67105