Bug 231528 - AX: Return AXEmptyGroup subrole for groups with no accessible content
Summary: AX: Return AXEmptyGroup subrole for groups with no accessible content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 231813
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-11 13:35 PDT by Tyler Wilcock
Modified: 2021-10-18 11:55 PDT (History)
12 users (show)

See Also:


Attachments
WIP (12.00 KB, patch)
2021-10-11 13:39 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2021-10-12 08:42 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (20.71 KB, patch)
2021-10-14 20:12 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (23.21 KB, patch)
2021-10-16 10:53 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2021-10-11 13:35:00 PDT
WebKit can calculate this more efficiently than AX clients.
Comment 1 Radar WebKit Bug Importer 2021-10-11 13:35:16 PDT
<rdar://problem/84113795>
Comment 2 Tyler Wilcock 2021-10-11 13:39:45 PDT
Created attachment 440830 [details]
WIP
Comment 3 Andres Gonzalez 2021-10-12 07:54:20 PDT
(In reply to Tyler Wilcock from comment #2)
> Created attachment 440830 [details]
> WIP

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

+    virtual bool hasAccessibleContent() const = 0;

I would call this hasContent. Everything here is already called "accessibility" or "accessible".

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

+bool AccessibilityObject::hasAccessibleContent() const
+{
+    auto* first = firstChild();
+    if (first && first->accessibilityIsIgnored())
+        first = first->nextSiblingUnignored(30);

where is the 30 coming from?

If first is null, we shouldn't need to calculate last and we can return early.

+    auto* last = lastChild();
+    if (last && last->accessibilityIsIgnored())
+        last = last->previousSiblingUnignored(30);

30?

+    if (first == last && first->roleValue() == AccessibilityRole::StaticText) {
+        Vector<AccessibilityText> text;
+        accessibilityText(text);
+        // Consider this object empty if its only child has the same static text as its accessibility text.
+        if (text.size())
+            return text[0].text != first->stringValue();
+    }

We don't need to calculate first and last to determine that the object has just 1 child.

I would think that if we have a group with just one static text child, we should expose the static text object and not the group. So this doesn't make much sense to me.
Comment 4 Tyler Wilcock 2021-10-12 08:42:16 PDT
Created attachment 440937 [details]
Patch
Comment 5 Tyler Wilcock 2021-10-12 08:46:02 PDT
> I would think that if we have a group with just one static text child, we
> should expose the static text object and not the group. So this doesn't make
> much sense to me.

This is how it generally works. The difference is that this case tests the situation where a group is given an accessible label (i.e. via `title` attribute or `aria-label`). If the group's accessible label and the static text are the same, the static text becomes redundant.
Comment 6 Andres Gonzalez 2021-10-12 08:52:39 PDT
(In reply to Tyler Wilcock from comment #5)
> > I would think that if we have a group with just one static text child, we
> > should expose the static text object and not the group. So this doesn't make
> > much sense to me.
> 
> This is how it generally works. The difference is that this case tests the
> situation where a group is given an accessible label (i.e. via `title`
> attribute or `aria-label`). If the group's accessible label and the static
> text are the same, the static text becomes redundant.

I would say the aria notation is redundant. I would favor the static text which is for example, selectable and visible on the screen, which the aria annotation is not.
Comment 7 Tyler Wilcock 2021-10-12 09:09:16 PDT
> I would say the aria notation is redundant. I would favor the static text which is for example, selectable and visible on the screen, which the aria annotation is not.

Yeah, that makes sense. This seems like a separate bug, so we can handle that outside this patch.
Comment 8 Tyler Wilcock 2021-10-14 20:12:36 PDT
Created attachment 441324 [details]
Patch
Comment 9 Tyler Wilcock 2021-10-14 20:15:01 PDT
accessibility/mac/aria-grouping-roles.html will fail because of https://bugs.webkit.org/show_bug.cgi?id=231756. I'll retry the tests after that one lands.
Comment 10 EWS 2021-10-15 07:32:43 PDT
Committed r284244 (243053@main): <https://commits.webkit.org/243053@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441324 [details].
Comment 11 WebKit Commit Bot 2021-10-15 09:56:29 PDT
Re-opened since this is blocked by bug 231813
Comment 12 ayumi_kojima 2021-10-15 15:28:58 PDT
*** Bug 231838 has been marked as a duplicate of this bug. ***
Comment 13 Tyler Wilcock 2021-10-16 10:53:30 PDT
Created attachment 441496 [details]
Patch
Comment 14 Tyler Wilcock 2021-10-18 10:42:34 PDT
I've re-run the iOS tests a bunch of times and the failures don't seem related. Most changes are also Mac-specific, so should be fine.
Comment 15 EWS 2021-10-18 11:55:35 PDT
Committed r284387 (243169@main): <https://commits.webkit.org/243169@main>

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