AX: textUnderElement does not find visibility:visible text when it's within a visibility:hidden container

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

When visibility:visible is placed within a visibility:hidden container, the elements within are rendered. But currently, AccessibilityNodeObject::textUnderElement stops immediately when encountering visibility:hidden, meaning we can miss this nested visibility:visible text.

[Radar WebKit Bug Importer]

<rdar://problem/125738704>

[Tyler Wilcock]

Created attachment 470703 [details]
Patch

[Tyler Wilcock]

Created attachment 470733 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 470733 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 50200f120372..a489f1db449e 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -2588,8 +2588,14 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     else if (attrName == disabledAttr)
         postNotification(element, AXDisabledStateChanged);
     else if (attrName == forAttr) {
-        if (RefPtr label = dynamicDowncast<HTMLLabelElement>(element))
+        if (RefPtr label = dynamicDowncast<HTMLLabelElement>(element)) {
             updateLabelFor(*label);
+
+            if (RefPtr oldControl = element->treeScope().getElementById(oldValue))
+                postNotification(oldControl.get(), AXTextChanged);
+            if (RefPtr newControl = element->treeScope().getElementById(newValue))
+                postNotification(newControl.get(), AXTextChanged);

AG: shouldn't it be LabelChanged?

+        }
     } else if (attrName == requiredAttr)
         postNotification(element, AXRequiredStatusChanged);
     else if (attrName == tabindexAttr) {
@@ -4740,6 +4746,13 @@ bool isNodeAriaVisible(Node* node)
     return !requiresAriaHiddenFalse || ariaHiddenFalsePresent;
 }

+// DOM component of hidden definition.
+// https://www.w3.org/TR/wai-aria/#dfn-hidden
+bool isDOMHidden(const RenderStyle* style)
+{
+    return style ? style->display() == DisplayType::None || style->usedVisibility() != Visibility::Visible : true;
+}
+
 AccessibilityObject* AXObjectCache::rootWebArea()
 {
     auto* root = getOrCreate(m_document->view());
@@ -5129,7 +5142,7 @@ void AXObjectCache::addLabelForRelation(Element& origin)

     // LabelFor relations are established for <label for=...> and for <figcaption> elements.
     if (RefPtr label = dynamicDowncast<HTMLLabelElement>(origin)) {
-        if (auto control = Accessibility::controlForLabelElement(*label))
+        if (RefPtr control = Accessibility::controlForLabelElement(*label))
             addedRelation = addRelation(&origin, control.get(), AXRelationType::LabelFor);
     } else if (origin.hasTagName(figcaptionTag)) {
         RefPtr parent = origin.parentNode();

[Tyler Wilcock]

> +            if (RefPtr oldControl =
> element->treeScope().getElementById(oldValue))
> +                postNotification(oldControl.get(), AXTextChanged);
> +            if (RefPtr newControl =
> element->treeScope().getElementById(newValue))
> +                postNotification(newControl.get(), AXTextChanged);
> 
> AG: shouldn't it be LabelChanged?
TW: Maybe, but it seems like handling AXLabelChanged runs updateNode and updateDependentProperties. The latter is not applicable here, which makes me think we've used this notification for a different purpose. Strictly based off the notification names, both kind of make sense — the label for the controls has changed, but so has the AX text, and AXTextChanged does only the necessary work: updateNode on the control. But open to restructuring things if there's a better way.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> > +            if (RefPtr oldControl =
> > element->treeScope().getElementById(oldValue))
> > +                postNotification(oldControl.get(), AXTextChanged);
> > +            if (RefPtr newControl =
> > element->treeScope().getElementById(newValue))
> > +                postNotification(newControl.get(), AXTextChanged);
> > 
> > AG: shouldn't it be LabelChanged?
> TW: Maybe, but it seems like handling AXLabelChanged runs updateNode and
> updateDependentProperties. The latter is not applicable here, which makes me
> think we've used this notification for a different purpose. Strictly based
> off the notification names, both kind of make sense — the label for the
> controls has changed, but so has the AX text, and AXTextChanged does only
> the necessary work: updateNode on the control. But open to restructuring
> things if there's a better way.

AG: I would think that TextChanged is intended for elements that contain text (inner text), not for changes on what we call AccessibleText. Maybe the handler of LabelChanged is doing too much, but conceptually this is a LabelChanged not a TextChanged.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 470733 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
index 0e6e94393c01..c463c56d2b55 100644
--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -2261,35 +2261,56 @@ static void appendNameToStringBuilder(StringBuilder& builder, String&& text)

 String AccessibilityNodeObject::textUnderElement(TextUnderElementMode mode) const
 {
-    Node* node = this->node();
-    if (RefPtr text = dynamicDowncast<Text>(node))
-        return text->wholeText();
+    auto isAriaVisible = [this] () {
+        return Accessibility::findAncestor<AccessibilityObject>(*this, true, [] (const AccessibilityObject& object) {

AG: I think you can have const auto& in the lambda param declaration.

+            return equalLettersIgnoringASCIICase(object.getAttribute(aria_hiddenAttr), "false"_s);
+        }) != nullptr;
+    };

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #5)
> > > +            if (RefPtr oldControl =
> > > element->treeScope().getElementById(oldValue))
> > > +                postNotification(oldControl.get(), AXTextChanged);
> > > +            if (RefPtr newControl =
> > > element->treeScope().getElementById(newValue))
> > > +                postNotification(newControl.get(), AXTextChanged);
> > > 
> > > AG: shouldn't it be LabelChanged?
> > TW: Maybe, but it seems like handling AXLabelChanged runs updateNode and
> > updateDependentProperties. The latter is not applicable here, which makes me
> > think we've used this notification for a different purpose. Strictly based
> > off the notification names, both kind of make sense — the label for the
> > controls has changed, but so has the AX text, and AXTextChanged does only
> > the necessary work: updateNode on the control. But open to restructuring
> > things if there's a better way.
> 
> AG: I would think that TextChanged is intended for elements that contain
> text (inner text), not for changes on what we call AccessibleText. Maybe the
> handler of LabelChanged is doing too much, but conceptually this is a
> LabelChanged not a TextChanged.
TW: We fire AXTextChanged for a lot more than objects that just contain text that has changed, e.g. when: alt, title, aria-label, and aria-labelledby attributes change. AXLabelChanged is confusing to use here because the corresponding "handle" function, handleLabelChanged, seems to have been designed around the fact that the object passed to handleLabelChanged, and thus the object that we fire AXLabelChanged for, is the actual label object. That is not the case here -- we are firing notifications for the controls who now have different accessibility text. Furthermore, using AXLabelChanged causes an unnecessary ancestry walk because it calls updateDependentProperties (because it was designed around handling the changed label, not the changed controls associated with the label).

[Tyler Wilcock]

Created attachment 470734 [details]
Patch

[Tyler Wilcock]

> +    auto isAriaVisible = [this] () {
> +        return Accessibility::findAncestor<AccessibilityObject>(*this,
> true, [] (const AccessibilityObject& object) {
> 
> AG: I think you can have const auto& in the lambda param declaration.
TW: Fixed!

[EWS]

Committed 277004@main (ae013fff20db): <https://commits.webkit.org/277004@main>

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