Bug 242401 – AX: Expose suggestion, insertion, deletion roles and attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

This change is necessary for AX clients to describe these elements.

[Radar WebKit Bug Importer]

<rdar://problem/96534144>

[Joshua Hoffman]

Created attachment 460721 [details]
Patch

[Joshua Hoffman]

Created attachment 460722 [details]
Patch

[Joshua Hoffman]

Created attachment 460723 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #2)
> Created attachment 460721 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h

@@ -503,6 +504,8 @@ ALWAYS_INLINE String accessibilityRoleToString(AccessibilityRole role)
         return "StaticText"_s;
     case AccessibilityRole::Subscript:
         return "Subscript"_s;
+    case AccessibilityRole::Suggestion:
+            return "Suggestion"_s;

Wrong indentation. Style checker should've gotten it.

--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
+++ a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

+- (BOOL)accessibilityIsInsertion
+{
+    if (![self _prepareAccessibilityCall])
+        return nil;

nil -> NO

+    for (AXCoreObject* obj = self.axBackingObject->parentObject(); obj; obj = obj->parentObject()) {

You can probably replace this for loop with a Accessibility::findAncestor. But if you keep the for loop:

Change AXCoreObject* obj -> auto* object

+        if (obj) {

It is unnecessary since it is the condition in the for statement.

+            if (obj->hasTagName(bodyTag))
+                return NO;

You can simply assume that if parentObject() is null you have reached the root of the tree, this is a rather expensive way of checking if you got to the root.

In short, I think you can re-write this as:

return Accessibility::findAncestor(*self.backingObject, true, [] (const auto& object) {
   return object.roleValue() == AccessibilityRole::Insertion;
});

+- (BOOL)accessibilityIsDeletion

Same comments as above apply here.

Further more, these methods may belong to AXCoreObject, in which case, in the platform wrapper you should only need to do 

return self.axBackingObject->isInsertion();

Some of the comments above apply also to the following methods:

+- (BOOL)accessibilityIsFirstItemInSuggestion

and

+- (BOOL)accessibilityIsLastItemInSuggestion

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -860,6 +860,14 @@ static void AXAttributeStringSetStyle(NSMutableAttributedString* attrString, Ren

+        if (auto* axObject = node->document().axObjectCache()->getOrCreate(node)) {

axObjectCache() can be null.

+            if (axObject->roleValue() == AccessibilityRole::Insertion)
+                AXAttributeStringSetNumber(attrString, @"AXIsSuggestedInsertion", @YES, range);
+            else if (axObject->roleValue() == AccessibilityRole::Deletion)
+                AXAttributeStringSetNumber(attrString, @"AXIsSuggestedDeletion", @YES, range);
+            if (axObject->roleValue() == AccessibilityRole::Suggestion)
+                AXAttributeStringSetNumber(attrString, @"AXIsSuggestion", @YES, range);

You may want to use a switch here, or the last if should be an else if.

-    if (backingObject->isTextControl()) {
+    if (backingObject->isTextControl() || backingObject->isStaticText()) {

Why we need to change this. I think some of these properties apply to text controls but not to static text.

--- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
+++ a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm

The four new methods in the AccessibilityUIElement class return NO. Are we testing this on the Mac?

[Joshua Hoffman]

Created attachment 460794 [details]
Patch

[chris fleizach]

Comment on attachment 460794 [details]
Patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2799
> +    for (AXCoreObject* parent = self.axBackingObject->parentObject(); parent; parent = parent->parentObject()) {

auto* parent

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2801
> +            auto children = parent->children();

I wonder if there's a cleaner way to write this. something like

auto *suggestion = Accessibility::findAncestor(*self.axBackingObject, false, [] (const auto& object) {
        return object.roleValue() == AccessibilityRole::Suggestion;
  });

auto children = suggestion.children();
return children.size() > 1 && children[0].isAncestorOrSelf(self);

[chris fleizach]

Comment on attachment 460794 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:863
> +        if (auto* axObjectCache = node->document().axObjectCache()) {

do we need to make AXObjects here or can we get away by checking attributes on the Node

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1006
> +    [attrString appendAttributedString:[[NSAttributedString alloc] initWithString:text.createNSStringWithoutCopying().get()]];

I think this is going to leak the NSAttributedString. you would need to autorelease it

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4039
> +        if ([attribute isEqualToString: (NSString*)kAXAttributedStringForRangeParameterizedAttribute])

extra space
ing: (NSStr

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4040
> +            return rangeSet ? [self doAXAttributedStringForRange:range] : nil;

it doesn't look like you're using reangeSet here. do you mean to change range?

[Joshua Hoffman]

Created attachment 460801 [details]
Patch

[chris fleizach]

Comment on attachment 460801 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4040
> +        if ([attribute isEqualToString:(NSString*)kAXAttributedStringForRangeParameterizedAttribute])

(NSString*) -> (NSString *)

> LayoutTests/accessibility/ios-simulator/suggestion-insertion-deletion-attributes.html:18
> +		let output = "This test makes sure that suggestions, insertions, and deletions expose the right attributes.\n";

replace tabs with spaces (and only use 4 spaces)

> LayoutTests/accessibility/mac/attributed-string-includes-insertion-deletion.html:21
> +

bad indentations in this file

> LayoutTests/accessibility/mac/suggestion-subrole-and-roledescription.html:22
> +		var ins1 = accessibilityController.accessibleElementById("ins1");

bad indentations in this file

[Joshua Hoffman]

Created attachment 460802 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #11)
> Created attachment 460802 [details]
> Patch

--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
+++ a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

+- (BOOL)accessibilityIsFirstItemInSuggestion
+{
+    if (![self _prepareAccessibilityCall])
+        return NO;
+
+    auto *suggestion = Accessibility::findAncestor(*self.axBackingObject, false, [] (const auto& object) {

* in the wrong side, should be auto* suggestion since it is a C++ object.

+    auto children = suggestion->children();

suggestion may be nullptr, so you need to check.

+    return children.size() > 0 && (children[0]->isAncestorOfObject(self.axBackingObject) || children[0] == self.axBackingObject);

Don't need > 0, just return children.size() && ...

I would swap the operands of || since children[0] == self.axBackingObject is cheaper and more likely.

Same comments for 

+- (BOOL)accessibilityIsLastItemInSuggestion

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -4025,6 +4036,11 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         }
     }

+    if (backingObject->isTextControl() || backingObject->isStaticText()) {
+        if ([attribute isEqualToString:(NSString *)kAXAttributedStringForRangeParameterizedAttribute])

Do we need to cast kAXAttributedStringForRangeParameterizedAttribute?

+            return [self doAXAttributedStringForRange:range];

Why we don't need to check for rangeSet as before?

--- a/Tools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
+++ a/Tools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
@@ -1038,3 +1038,4 @@ void AccessibilityUIElement::rowHeaders(Vector<AccessibilityUIElement>&) const
 {
     // FIXME: implement
 }
+

Seems that the only change in this file is the addition of a blank line, could you undo that?

--- a/Tools/WebKitTestRunner/InjectedBundle/atspi/AccessibilityUIElementAtspi.cpp
+++ a/Tools/WebKitTestRunner/InjectedBundle/atspi/AccessibilityUIElementAtspi.cpp

+}
+
+
+bool AccessibilityUIElement::isFirstItemInSuggestion() const

Extra blank line before this function and the next one.

--- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
+++ a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm

Why we return NO in all the new methods on the Mac? If we don't need this on the Mac perhaps we should say notImplemented()?

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #12)
> (In reply to Joshua Hoffman from comment #11)
> > Created attachment 460802 [details]
> > Patch

> --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> +++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> @@ -4025,6 +4036,11 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
>          }
>      }
> 
> +    if (backingObject->isTextControl() || backingObject->isStaticText()) {
> +        if ([attribute isEqualToString:(NSString
> *)kAXAttributedStringForRangeParameterizedAttribute])
> 
> Do we need to cast kAXAttributedStringForRangeParameterizedAttribute?
> 
> +            return [self doAXAttributedStringForRange:range];
> 
> Why we don't need to check for rangeSet as before?

On second thought, I will add the rangeSet check back in (especially in the context of the checks below it).

The cast prevents Xcode warnings, in addition to maintaining consistency (we cast CFStrings to NSStrings throughout WebKit).

> 
> --- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
> +++ a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
> 
> Why we return NO in all the new methods on the Mac? If we don't need this on
> the Mac perhaps we should say notImplemented()?

Yeah, we don't need these implementations on the Mac. Good call-out about adding notImplemented—I will make that change.

[Joshua Hoffman]

Created attachment 460827 [details]
Patch

[Joshua Hoffman]

Created attachment 460835 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #15)
> Created attachment 460835 [details]
> Patch

--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
+++ a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

+- (BOOL)accessibilityIsFirstItemInSuggestion
+{
...
+    auto children = suggestion->children();
+    auto parent = self.axBackingObject->parentObject();
+    BOOL isFirstChildOfParent = parent && parent->children().size() && parent->children()[0] == self.axBackingObject;
+    return children.size() && (children[0] == self.axBackingObject || (isFirstChildOfParent && children[0]->isAncestorOfObject(self.axBackingObject)));

I think this logic fails in the following case:

<suggestion>
    <grandparent>
        <uncle>
            <child1>
        <parent>
            <child2>

it will return true for child2.

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #16)
> (In reply to Joshua Hoffman from comment #15)
> > Created attachment 460835 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
> +++ a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
> 
> +- (BOOL)accessibilityIsFirstItemInSuggestion
> +{
> ...
> +    auto children = suggestion->children();
> +    auto parent = self.axBackingObject->parentObject();
> +    BOOL isFirstChildOfParent = parent && parent->children().size() &&
> parent->children()[0] == self.axBackingObject;
> +    return children.size() && (children[0] == self.axBackingObject ||
> (isFirstChildOfParent &&
> children[0]->isAncestorOfObject(self.axBackingObject)));
> 
> I think this logic fails in the following case:
> 
> <suggestion>
>     <grandparent>
>         <uncle>
>             <child1>
>         <parent>
>             <child2>
> 
> it will return true for child2.

I'd think a traversal up, where in each iteration we check both that a child is always the first element and check for a suggestion ancestor, could fix this? (not using FindAncestor to allow us to break early) Something like:

for (AXCoreObject* obj = self.axBackingObject->parentObject(); obj; obj = obj->parentObject())
{
    if (auto parent = obj->parentObject()) {
        if (parent->children().size() && parent->children()[0] != self.axBackingObject)
            return NO;
        if (parent->roleValue() == AccessibilityRole::Suggestion)
            return YES;
    }
}

[Joshua Hoffman]

Created attachment 460871 [details]
Patch

[Joshua Hoffman]

Created attachment 460896 [details]
Patch

[Joshua Hoffman]

Created attachment 460927 [details]
Patch

[EWS]

Committed 252516@main (3395d7913749): <https://commits.webkit.org/252516@main>

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