Bug 233620 – AX: Support accessibility attributes for <model>

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

[chris fleizach]

Add accessibility attributes and ability to dive into the model hierarchy for <model> elements.

[Radar WebKit Bug Importer]

<rdar://problem/85852073>

[chris fleizach]

Created attachment 445384 [details]
patch

[chris fleizach]

Created attachment 445389 [details]
patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Created attachment 445389 [details]
> patch

--- a/LayoutTests/accessibility/model-element-accessibility.html
+++ a/LayoutTests/accessibility/model-element-accessibility.html

Don't include -accessibility as part of the file name, it is already under the accessibility dir. We can be more specific and call it model-element-alt-role or model-element-attributes.

--- a/Source/WebCore/Modules/model-element/scenekit/SceneKitModelPlayer.mm
+++ a/Source/WebCore/Modules/model-element/scenekit/SceneKitModelPlayer.mm

+#if PLATFORM(COCOA)
+Vector<RetainPtr<id>> SceneKitModelPlayer::accessibilityChildren()

Do we need to #if COCOA in this .mm file?

+    Vector<RetainPtr<id>> modelChildren(children.count);
+    for (id child in children)
+        modelChildren.append(child);
+    return modelChildren;
+}

Can we use instead:

template<typename VectorElementType> Vector<VectorElementType> makeVector(NSArray *array)

i.e.:

+    return makeVector(children);

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

+Vector<RetainPtr<id>> AccessibilityObject::modelElementChildren()
+{
+    Node* node = this->node();
+    if (!node || !is<HTMLModelElement>(node))

Check for !node is redundant since is<> does that.

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

+#if PLATFORM(COCOA) && ENABLE(MODEL_ELEMENT)
+    virtual Vector<RetainPtr<id>> modelElementChildren() = 0;
+#endif

Maybe beyond the scope of this change, but it would be good not to have to expose this in the interface, it should be part of children. We already have a different way of getting children for Widgets/Plugins, now this element. We should come up with a way of getting the children for all accessibility objects that is cross-platform and has the right abstraction.

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

     if ([attributeName isEqualToString:NSAccessibilityChildrenAttribute] || [attributeName isEqualToString:NSAccessibilityChildrenInNavigationOrderAttribute]) {
+#if ENABLE(MODEL_ELEMENT)
+        if (backingObject->isModel()) {
+            auto modelChildren = backingObject->modelElementChildren();
+            if (modelChildren.size()) {
+                NSMutableArray *children = [NSMutableArray array];
+                for (auto child : modelChildren)
+                    [children addObject:child.get()];
+                return children;
+            }
+        }
+#endif

Can we use instead:

template<typename CollectionType, typename MapFunctionType> RetainPtr<NSMutableArray> createNSArray(CollectionType&& collection, MapFunctionType&& function)

     if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
         if (!self.childrenVectorSize) {
-            NSArray *children = [self renderWidgetChildren];
+            NSArray *children = nil;
+            if (backingObject->isModel()) {
+                NSMutableArray *mutableChildren = [NSMutableArray array];
+                for (auto child : backingObject->modelElementChildren())
+                    [mutableChildren addObject:child.get()];
+                children = mutableChildren;
+            } else

createNSArray ? same as above.

--- a/Source/WebKit/WebProcess/Model/ARKitInlinePreviewModelPlayer.mm
+++ a/Source/WebKit/WebProcess/Model/ARKitInlinePreviewModelPlayer.mm

+#if PLATFORM(COCOA)
+Vector<RetainPtr<id>> ARKitInlinePreviewModelPlayer::accessibilityChildren()

Do we need #if COCOA in this .mm file?

[chris fleizach]

Created attachment 445474 [details]
patch

[chris fleizach]

Created attachment 445475 [details]
patch

[chris fleizach]

Created attachment 445476 [details]
patch

[Dean Jackson]

Comment on attachment 445476 [details]
patch

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

> LayoutTests/platform/mac/accessibility/model-element-attributes-exepcted.txt:1
> +This tests model elements have basic accessibility support

There is a typo in this filename. "exepcted"

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1279
> +    return isImage() || isInputImage() || isNativeImage() || isCanvas() || isModel() || (node() && node()->hasTagName(imgTag));

You probably want to guard the model stuff here and elsewhere with #if ENABLE(MODEL_ELEMENT)

[chris fleizach]

Created attachment 445520 [details]
patch

[chris fleizach]

Created attachment 445522 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #10)
> Created attachment 445522 [details]
> Patch

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        Test: accessibility/model-element-accessibility.html

ChangeLog still has the old file name.

--- a/LayoutTests/ChangeLog
+++ a/LayoutTests/ChangeLog

+        * accessibility/model-element-accessibility.html: Added.

Dito.

[chris fleizach]

Created attachment 445589 [details]
Patch

[chris fleizach]

Created attachment 445593 [details]
Patch

[EWS]

Committed r286406 (244753@main): <https://commits.webkit.org/244753@main>

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