Bug 222154 – Add [WebAccessibilityObjectWrapper textMarkerRangeForNSRange] to allow clients to efficiently get a TextMarkerRange from an NSRange.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Add [WebAccessibilityObjectWrapper textMarkerRangeForNSRange] to allow clients to efficiently get a TextMarkerRange from an NSRange.

[Radar WebKit Bug Importer]

<rdar://problem/74505496>

[Andres Gonzalez]

Created attachment 420906 [details]
Patch

[chris fleizach]

+CONSOLE MESSAGE: TypeError: axElement.textMarkerRangeForRange is not a function. (In 'axElement.textMarkerRangeForRange(0, i)', 'axElement.textMarkerRangeForRange' is undefined)

[chris fleizach]

Comment on attachment 420906 [details]
Patch

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

do we need to expose this method through a new attribute?

> Source/WebCore/accessibility/AccessibilityObject.h:829
> +inline bool AccessibilityObject::allowsTextRanges() const { return true; }

is this change necessary?

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1279
> +#if PLATFORM(MAC)

what's holding us back to make this a cocoa method?

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:285
> +    return textMarkerRangeFromVisiblePositions(axObjectCache(),

do we need to validate nsRange parameters coming in?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4019
> +            auto webRange = makeSimpleRange({ start, end });

will this crash if start or end is nil?

[Andres Gonzalez]

Created attachment 421103 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 420906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420906&action=review
> 
> do we need to expose this method through a new attribute?

Not necessarily, since client can call it directly as we do on iOS accessibility API. I feel reluctant to keep adding attributes to accessibilityAttributeValue, especially when it is a method that doesn't quite match an object property. In this case this method can involve DOM nodes that the current accessibility object does not represent.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:829
> > +inline bool AccessibilityObject::allowsTextRanges() const { return true; }
> 
> is this change necessary?

Yes, since we want to retrieve TextMarkerRanges for more than just text controls.
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1279
> > +#if PLATFORM(MAC)
> 
> what's holding us back to make this a cocoa method?

We need to port to iOS the utility function to convert TextMarkerRanges to/from VisiblePositionRanges, Mac only at the moment.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:285
> > +    return textMarkerRangeFromVisiblePositions(axObjectCache(),

> 
> do we need to validate nsRange parameters coming in?
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4019
> > +            auto webRange = makeSimpleRange({ start, end });
> 
> will this crash if start or end is nil?

[Darin Adler]

Comment on attachment 421103 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1281
> +    virtual AXTextMarkerRangeRef textMarkerRangeForNSRange(const NSRange&) const = 0;

I don’t understand the object lifetime here. Why doesn’t this return RetainPtr<AXTextMarkerRangeRef>? Is this using autorelease?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:406
> +    AXTextMarkerRangeRef textMarkerRangeForNSRange(const NSRange&) const  override;

Extra space here before override.

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:62
> +AXTextMarkerRangeRef AXIsolatedObject::textMarkerRangeForNSRange(const NSRange& nsRange) const

I suggest just naming this range.

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:65
> +        auto* axObject = associatedAXObject();

I suggest just naming this object.

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:66
> +        return axObject ? axObject->textMarkerRangeForNSRange(nsRange) : NULL;

Should use nullptr instead of NULL.

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:283
> +AXTextMarkerRangeRef AccessibilityObject::textMarkerRangeForNSRange(const NSRange& nsRange) const

Could just name this range.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3466
> +- (AXTextMarkerRangeRef)textMarkerRangeForNSRange:(const NSRange&)nsRange

Could just name this range.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1824
> +    NSRange range = NSMakeRange(location, length);
> +    BEGIN_AX_OBJC_EXCEPTIONS
> +    id textMarkerRange = [m_element textMarkerRangeForNSRange:range];
> +    return AccessibilityTextMarkerRange::create(textMarkerRange);

This would work better as a one-liner.

    return AccessibilityTextMarkerRange::create([m_element textMarkerRangeForNSRange:NSMakeRange(location, length)]);

[Andres Gonzalez]

Created attachment 421140 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #7)
> Comment on attachment 421103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421103&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1281
> > +    virtual AXTextMarkerRangeRef textMarkerRangeForNSRange(const NSRange&) const = 0;
> 
> I don’t understand the object lifetime here. Why doesn’t this return
> RetainPtr<AXTextMarkerRangeRef>? Is this using autorelease?

It is using autorelease. It ultimately calls:

    return (AXTextMarkerRangeRef)CFBridgingRelease(AXTextMarkerRangeCreate(kCFAllocatorDefault, startMarker, endMarker));

And 

// Casts a CoreFoundation object to an Objective-C object, transferring ownership to ARC (ie. no need to CFRelease to balance a prior +1 CFRetain count). NS_RETURNS_RETAINED is used to indicate that the Objective-C object returned has +1 retain count.  So the object is 'released' as far as CoreFoundation reference counting semantics are concerned, but retained (and in need of releasing) in the view of ARC. This function is intended for use while converting to ARC mode only.
NS_INLINE id _Nullable CFBridgingRelease(CFTypeRef CF_CONSUMED _Nullable X) NS_RETURNS_RETAINED {
    return [(id)CFMakeCollectable(X) autorelease];
}

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:406
> > +    AXTextMarkerRangeRef textMarkerRangeForNSRange(const NSRange&) const  override;
> 
> Extra space here before override.

Fixed.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:62
> > +AXTextMarkerRangeRef AXIsolatedObject::textMarkerRangeForNSRange(const NSRange& nsRange) const
> 
> I suggest just naming this range.

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:65
> > +        auto* axObject = associatedAXObject();
> 
> I suggest just naming this object.

Since in the implementation of AXIsolatedObject we have two kinds of accessibility objet, the isolated objects and the live objects (AccessibilityObject or AXObject for short), I've tried to be consistent in naming "object" the AXIsolatedObject instances, and "axObject" the AXObject instances. I believe it makes it clearer what kind of accessibility object we are talking to.

> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:66
> > +        return axObject ? axObject->textMarkerRangeForNSRange(nsRange) : NULL;
> 
> Should use nullptr instead of NULL.

Fixed.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:283
> > +AXTextMarkerRangeRef AccessibilityObject::textMarkerRangeForNSRange(const NSRange& nsRange) const
> 
> Could just name this range.

Done.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3466
> > +- (AXTextMarkerRangeRef)textMarkerRangeForNSRange:(const NSRange&)nsRange
> 
> Could just name this range.

Done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1824
> > +    NSRange range = NSMakeRange(location, length);
> > +    BEGIN_AX_OBJC_EXCEPTIONS
> > +    id textMarkerRange = [m_element textMarkerRangeForNSRange:range];
> > +    return AccessibilityTextMarkerRange::create(textMarkerRange);
> 
> This would work better as a one-liner.

Done.
> 
>     return AccessibilityTextMarkerRange::create([m_element
> textMarkerRangeForNSRange:NSMakeRange(location, length)]);

Thanks!

[Darin Adler]

(In reply to Andres Gonzalez from comment #9)
> It is using autorelease. It ultimately calls:
> 
>     return
> (AXTextMarkerRangeRef)CFBridgingRelease(AXTextMarkerRangeCreate(kCFAllocatorD
> efault, startMarker, endMarker));
> 
> And 
> 
> // Casts a CoreFoundation object to an Objective-C object, transferring
> ownership to ARC (ie. no need to CFRelease to balance a prior +1 CFRetain
> count). NS_RETURNS_RETAINED is used to indicate that the Objective-C object
> returned has +1 retain count.  So the object is 'released' as far as
> CoreFoundation reference counting semantics are concerned, but retained (and
> in need of releasing) in the view of ARC. This function is intended for use
> while converting to ARC mode only.
> NS_INLINE id _Nullable CFBridgingRelease(CFTypeRef CF_CONSUMED _Nullable X)
> NS_RETURNS_RETAINED {
>     return [(id)CFMakeCollectable(X) autorelease];
> }

Despite the fact that CFBridgingRelease is a forward-looking way to convert a CF type into an Objective-C type that works with both ARC and non-ARC, I think the way we are using it in AXObjectCacheMac.mm is a bad pattern for when we actually move WebCore to ARC.

The problem is the typecasting. For ARC it’s important to consider ownership whenever casting from an Objective-C pointer type to a CF type. The AXTextMarkerRangeRef type is not recognized by the compiler as an Objective-C type so this idiom won’t work well. Casting from one to the other changes ownership and can’t simply be done with a cast. It would work fine if the type was actually an Objective-C pointer type or if we used RetainPtr on a CF type, but combining the two like this is not good.

I suggest we move to RetainPtr and adoptCF for this. I know it’s a big change, but it will put us on much better footing for converting to ARC. As a side effect it might help performance by not putting objects into the autorelease pool.

Not for this patch, but important to future maintainability of WebKit.

Chris Dumez, drawing your attention to this, since it’s related to what you’re working on.

[Darin Adler]

(In reply to Darin Adler from comment #10)
> I suggest we move to RetainPtr and adoptCF for this.

Or, if we want to stick with autorelease, we can use CFAutorelease instead of CFBridgingRelease with a cast to a non-Objective-C type.

[Andres Gonzalez]

(In reply to Darin Adler from comment #11)
> (In reply to Darin Adler from comment #10)
> > I suggest we move to RetainPtr and adoptCF for this.
> 
> Or, if we want to stick with autorelease, we can use CFAutorelease instead
> of CFBridgingRelease with a cast to a non-Objective-C type.

Thanks for the clarification! Will address in a separate patch and copy you and Chris Dumez.

[Andres Gonzalez]

Created attachment 421147 [details]
Patch

Addresses assert crash in accessibility/mac/crash-bounds-for-range.html.

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 420906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420906&action=review
> 
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:285
> > +    return textMarkerRangeFromVisiblePositions(axObjectCache(),
> 
> do we need to validate nsRange parameters coming in?

visiblePositionForIndexUsingCharacterIterator handles out of range params appropriately. The test includes out of range cases.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4019
> > +            auto webRange = makeSimpleRange({ start, end });
> 
> will this crash if start or end is nil?

It will return WTF::nullopt.

Thanks.

[EWS]

Committed r273227: <https://commits.webkit.org/r273227>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421147 [details].