Bug 233620 - AX: Support accessibility attributes for <model>
Summary: AX: Support accessibility attributes for <model>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-29 22:59 PST by chris fleizach
Modified: 2021-12-01 17:11 PST (History)
10 users (show)

See Also:


Attachments
patch (28.66 KB, patch)
2021-11-29 23:17 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (28.46 KB, patch)
2021-11-30 00:07 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (28.96 KB, patch)
2021-11-30 15:02 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (29.69 KB, patch)
2021-11-30 15:06 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (29.36 KB, patch)
2021-11-30 15:11 PST, chris fleizach
dino: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (29.44 KB, patch)
2021-11-30 21:35 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (175.02 KB, patch)
2021-11-30 22:10 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (175.00 KB, patch)
2021-12-01 10:20 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (174.99 KB, patch)
2021-12-01 11:13 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2021-11-29 22:59:00 PST
Add accessibility attributes and ability to dive into the model hierarchy for <model> elements.
Comment 1 Radar WebKit Bug Importer 2021-11-29 22:59:17 PST
<rdar://problem/85852073>
Comment 2 chris fleizach 2021-11-29 23:17:18 PST
Created attachment 445384 [details]
patch
Comment 3 chris fleizach 2021-11-30 00:07:49 PST
Created attachment 445389 [details]
patch
Comment 4 Andres Gonzalez 2021-11-30 07:50:56 PST
(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?
Comment 5 chris fleizach 2021-11-30 15:02:52 PST
Created attachment 445474 [details]
patch
Comment 6 chris fleizach 2021-11-30 15:06:48 PST
Created attachment 445475 [details]
patch
Comment 7 chris fleizach 2021-11-30 15:11:04 PST
Created attachment 445476 [details]
patch
Comment 8 Dean Jackson 2021-11-30 16:00:48 PST
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)
Comment 9 chris fleizach 2021-11-30 21:35:26 PST
Created attachment 445520 [details]
patch
Comment 10 chris fleizach 2021-11-30 22:10:11 PST
Created attachment 445522 [details]
Patch
Comment 11 Andres Gonzalez 2021-12-01 07:38:20 PST
(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.
Comment 12 chris fleizach 2021-12-01 10:20:57 PST
Created attachment 445589 [details]
Patch
Comment 13 chris fleizach 2021-12-01 11:13:00 PST
Created attachment 445593 [details]
Patch
Comment 14 EWS 2021-12-01 17:11:51 PST
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].