Bug 257184 – AX: Content editable should not be textbox roles

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

We can't have contenteditable directly turn into textboxes. Textboxes, as an AX object, can't have complicated children objects (tables, lists, etc).

But frequently this is true and having this act as a textbox means these objects won't work.

[Radar WebKit Bug Importer]

<rdar://problem/109699222>

[chris fleizach]

Created attachment 466459 [details]
Patch

[Joshua Hoffman]

Created attachment 468207 [details]
Patch

[Joshua Hoffman]

Created attachment 468300 [details]
Patch

[Joshua Hoffman]

Created attachment 468307 [details]
Patch

[Joshua Hoffman]

Created attachment 468318 [details]
Patch

[chris fleizach]

Comment on attachment 468318 [details]
Patch

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

> Source/WebCore/accessibility/AXCoreObject.h:1574
> +        return ancestor.isTextControl() || ancestor.hasContentEditableAttributeSet();

do we need these both here? you're returning true in isTextControl

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:399
> +        return AccessibilityRole::Group;

I think this needs to be lower down the list. we can have things that are content editable but have other roles first

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2045
> +    if (m_renderer->isTextField() && !hasContentEditableAttributeSet()) {

does it matter if a text field has content editable? it seems like it won't do anything anyway

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:900
> +    if ([self accessibilityIsInContentEditable] && ![self accessibilityElementCount])

does it matter if we have element count?

[Tyler Wilcock]

Comment on attachment 468318 [details]
Patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:901
> +    if ([self accessibilityIsInContentEditable] && ![self accessibilityElementCount])
> +        traits |= [self _accessibilityTextEntryTraits];

Do we need to do a separate ancestry traversal here via accessibilityIsInContentEditable when we already do one for _accessibilityTraitsFromAncestors?

[Joshua Hoffman]

Comment on attachment 468318 [details]
Patch

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

>> Source/WebCore/accessibility/AXCoreObject.h:1574
>> +        return ancestor.isTextControl() || ancestor.hasContentEditableAttributeSet();
> 
> do we need these both here? you're returning true in isTextControl

Good catch, yeah we can remove this.

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:900
>> +    if ([self accessibilityIsInContentEditable] && ![self accessibilityElementCount])
> 
> does it matter if we have element count?

I only wanted to apply the editing trait to leaf nodes, and this seemed like the best approach. But it might not have an affect—I can see what removing that does.

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:901
>> +        traits |= [self _accessibilityTextEntryTraits];
> 
> Do we need to do a separate ancestry traversal here via accessibilityIsInContentEditable when we already do one for _accessibilityTraitsFromAncestors?

We should be able to combine them!

[Joshua Hoffman]

Created attachment 468375 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468375 [details]
Patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2776
> +    auto* ancestor = Accessibility::findAncestor(*self.axBackingObject, true, [] (const auto& object) {
> +        return object.hasContentEditableAttributeSet();
> +    });
> +
> +    return ancestor;

I don't think this temporary variable is necessary — can probably just be: 

return Accessibility::findAncestor(...)

> LayoutTests/accessibility/ios-simulator/content-editable.html:12
> +		<p id="text2">Nested text inside</p>

This <p> seems indented one level too far

> LayoutTests/accessibility/ios-simulator/content-editable.html:18
> +

Extra newline here

> LayoutTests/accessibility/ios-simulator/content-editable.html:20
> +	var output = "This test makes sure that the content editable method on iOS works as expected.\n";

Not sure if others have been liking this style, but I like not indenting a level when a script tag begins. So var output would be flush with the script tag, as would if (window.accessibilityController) {, etc

> LayoutTests/accessibility/ios-simulator/content-editable.html:35
> +

Extra newline here

[Tyler Wilcock]

Comment on attachment 468375 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:4157
> +        case AXContentEditableAttributeChanged:
> +            tree->updateNodeProperty(*notification.first, AXPropertyName::HasContentEditableAttributeSet);
> +            break;

Do we have a test that exercises this dynamic contenteditable change? One idea — given this markup:

<div id="container-div">
    <button id="foo">Activate</button>
</div>

#foo should initially have no editable ancestor. Then add contenteditable to #container-div. #foo's editable ancestor should then be #container-div. AccessibilityUIElement::editableAncestor() is available to use for this

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #12)
> Comment on attachment 468375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468375&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4157
> > +        case AXContentEditableAttributeChanged:
> > +            tree->updateNodeProperty(*notification.first, AXPropertyName::HasContentEditableAttributeSet);
> > +            break;
> 
> Do we have a test that exercises this dynamic contenteditable change? One
> idea — given this markup:
> 
> <div id="container-div">
>     <button id="foo">Activate</button>
> </div>
> 
> #foo should initially have no editable ancestor. Then add contenteditable to
> #container-div. #foo's editable ancestor should then be #container-div.
> AccessibilityUIElement::editableAncestor() is available to use for this

That is a good idea—I'll confirm that we don't have one already and add one if not.

[Joshua Hoffman]

Created attachment 468536 [details]
Patch

[Joshua Hoffman]

Created attachment 468547 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXCoreObject.cpp b/Source/WebCore/accessibility/AXCoreObject.cpp
index 1643613e0acf..cead8b5054b5 100644
--- a/Source/WebCore/accessibility/AXCoreObject.cpp
+++ b/Source/WebCore/accessibility/AXCoreObject.cpp
@@ -102,6 +102,9 @@ bool AXCoreObject::isButton() const

 bool AXCoreObject::isTextControl() const
 {
+    if (hasContentEditableAttributeSet())
+        return true;
+
     switch (roleValue()) {
     case AccessibilityRole::ComboBox:
     case AccessibilityRole::SearchField:
diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index 5360bd0d8a9a..b77e0de882c9 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -1380,6 +1380,8 @@ public:
     virtual AXCoreObject* highestEditableAncestor() = 0;
     virtual AXCoreObject* exposedTableAncestor(bool includeSelf = false) const = 0;

+    virtual bool hasContentEditableAttributeSet() const = 0;

AG: I don't think we should expose this lower level property in the AXCoreObject interface. This should be abstracted into one of the already existing roles behavior.

+
     virtual AccessibilityChildrenVector documentLinks() = 0;

     virtual String innerHTML() const = 0;
diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
index da48f2da2453..d3ecb6fb509d 100644
--- a/Source/WebCore/accessibility/AXLogger.cpp
+++ b/Source/WebCore/accessibility/AXLogger.cpp
@@ -559,6 +559,9 @@ TextStream& operator<<(TextStream& stream, AXObjectCache::AXNotification notific
     case AXObjectCache::AXNotification::AXColumnSpanChanged:
         stream << "AXColumnSpanChanged";
         break;
+    case AXObjectCache::AXNotification::AXContentEditableAttributeChanged:
+        stream << "AXContentEditableAttributeChanged";
+        break;
     case AXObjectCache::AXNotification::AXControlledObjectsChanged:
         stream << "AXControlledObjectsChanged";
         break;
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index c171238e6893..7a5a8766f5fe 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1165,7 +1165,7 @@ void AXObjectCache::handleTextChanged(AccessibilityObject* object)
         if (parent->supportsLiveRegion())
             postLiveRegionChangeNotification(parent);

-        if (!notifiedNonNativeTextControl && parent->isNonNativeTextControl()) {
+        if (!notifiedNonNativeTextControl && (parent->isNonNativeTextControl() || parent->hasContentEditableAttributeSet())) {

AG: Do we need to differentiate between isNonNativeTextControl() and hasContentEditableAttributeSet? or they can be lumped together?

             postNotification(parent, parent->document(), AXValueChanged);
             notifiedNonNativeTextControl = true;
         }
@@ -1288,7 +1288,7 @@ void AXObjectCache::handleChildrenChanged(AccessibilityObject& object)
             postLiveRegionChangeNotification(parent);

         // If this object is an ARIA text control, notify that its value changed.
-        if (parent->isNonNativeTextControl()) {
+        if (parent->isNonNativeTextControl() || parent->hasContentEditableAttributeSet()) {
             postNotification(parent, parent->document(), AXValueChanged);

             // Do not let any ancestor of an editable object update its children.
@@ -2439,6 +2439,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     else if (attrName == contenteditableAttr) {
         if (auto* axObject = get(element))
             axObject->updateRole();
+        postNotification(element, AXContentEditableAttributeChanged);

AG: maybe we should add a handleContentEditableChange method that does both update the role and post the appropriate notification.

     }
     else if (attrName == disabledAttr)
         postNotification(element, AXDisabledStateChanged);
@@ -4166,6 +4167,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXColumnSpanChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnIndexRange);
             break;
+        case AXContentEditableAttributeChanged:
+            tree->updateNodeProperty(*notification.first, AXPropertyName::HasContentEditableAttributeSet);

AG: again by now, this should already be condified into the existing AXCoreObject properties so that we don't need to cache this in the iso tree as a separate property.

+            break;
         case AXDisabledStateChanged:
             tree->updatePropertiesForSelfAndDescendants(*notification.first, { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled });
             break;
diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 23babdad974a..41346fd9659a 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -345,6 +345,7 @@ public:
         AXColumnCountChanged,
         AXColumnIndexChanged,
         AXColumnSpanChanged,
+        AXContentEditableAttributeChanged,
         AXControlledObjectsChanged,
         AXCurrentStateChanged,
         AXDescribedByChanged,
diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
index aad6ebc2b9bf..d928dceaa6f9 100644
--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -336,7 +336,7 @@ AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole()

 bool AccessibilityNodeObject::matchesTextAreaRole() const
 {
-    return is<HTMLTextAreaElement>(node()) || hasContentEditableAttributeSet();
+    return is<HTMLTextAreaElement>(node());
 }

 AccessibilityRole AccessibilityNodeObject::determineAccessibilityRoleFromNode(TreatStyleFormatGroupAsInline treatStyleFormatGroupAsInline) const
@@ -430,7 +430,7 @@ AccessibilityRole AccessibilityNodeObject::determineAccessibilityRoleFromNode(Tr
         return AccessibilityRole::Label;
     if (node()->hasTagName(dfnTag))
         return AccessibilityRole::Definition;
-    if (node()->hasTagName(divTag))
+    if (node()->hasTagName(divTag) && !hasContentEditableAttributeSet())
         return AccessibilityRole::Generic;
     if (is<HTMLFormElement>(node()))
         return AccessibilityRole::Form;
@@ -467,6 +467,9 @@ AccessibilityRole AccessibilityNodeObject::determineAccessibilityRoleFromNode(Tr
     if (auto* summaryElement = dynamicDowncast<HTMLSummaryElement>(node()); summaryElement && summaryElement->isActiveSummary())
         return AccessibilityRole::Summary;

+    if (hasContentEditableAttributeSet())
+        return AccessibilityRole::Group;
+
     // http://rawgit.com/w3c/aria/master/html-aam/html-aam.html
     // Output elements should be mapped to status role.
     if (isOutput())
@@ -2762,6 +2765,9 @@ bool AccessibilityNodeObject::canSetValueAttribute() const
     if (isNonNativeTextControl())
         return true;

+    if (hasContentEditableAttributeSet())
+        return true;
+

AG: merge the two if statements above.

     if (isMeter())
         return false;

diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index e6767f6ccfe3..cfc9553ff7d1 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -354,7 +354,7 @@ bool AccessibilityObject::isARIATextControl() const

 bool AccessibilityObject::isNonNativeTextControl() const
 {
-    return (isARIATextControl() || hasContentEditableAttributeSet()) && !isNativeTextControl();
+    return isARIATextControl() && !isNativeTextControl();

AG: well, this seems contrary to what I'm thinking the solution should look like.

 }

 bool AccessibilityObject::hasMisspelling() const
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index 05948a47b6c6..d7c9b304fc24 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -615,7 +615,7 @@ public:
     bool isBusy() const override { return false; }
     static const String defaultLiveRegionStatusForRole(AccessibilityRole);
     static bool contentEditableAttributeIsEnabled(Element*);
-    bool hasContentEditableAttributeSet() const;
+    bool hasContentEditableAttributeSet() const final;

AG: again in my view, this is contrary to the right solution.

     bool supportsReadOnly() const;
     virtual String readOnlyValue() const;
diff --git a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
index 5abae188d44c..726276ea768f 100644
--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
+++ b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
@@ -902,6 +902,9 @@ - (uint64_t)accessibilityTraits
         break;
     }

+    if ([self accessibilityIsInContentEditable])
+        traits |= [self _accessibilityTextEntryTraits];
+
     if (self.axBackingObject->isAttachmentElement())
         traits |= [self _axUpdatesFrequentlyTrait];

@@ -995,6 +998,9 @@ - (BOOL)determineIsAccessibilityElement
     case AccessibilityRole::Video:
         return [self accessibilityIsWebInteractiveVideo];

+    if (self.axBackingObject->hasContentEditableAttributeSet())
+        return true;
+
     // Links can sometimes be elements (when they only contain static text or don't contain anything).
     // They should not be elements when containing text and other types.
     case AccessibilityRole::WebCoreLink:
@@ -2750,6 +2756,13 @@ - (BOOL)accessibilityIsDeletion
     }) != nullptr;
 }

+- (BOOL)accessibilityIsInContentEditable
+{
+    return Accessibility::findAncestor(*self.axBackingObject, true, [] (const auto& object) {
+        return object.hasContentEditableAttributeSet();
+    });
+}
+
 - (BOOL)accessibilityIsFirstItemInSuggestion
 {
     if (![self _prepareAccessibilityCall])
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 8f9c2eea8e01..53c9fbae41a9 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -166,6 +166,7 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     setProperty(AXPropertyName::IsKeyboardFocusable, object.isKeyboardFocusable());
     setProperty(AXPropertyName::BrailleRoleDescription, object.brailleRoleDescription().isolatedCopy());
     setProperty(AXPropertyName::BrailleLabel, object.brailleLabel().isolatedCopy());
+    setProperty(AXPropertyName::HasContentEditableAttributeSet, object.hasContentEditableAttributeSet());

     RefPtr geometryManager = tree()->geometryManager();
     std::optional frame = geometryManager ? geometryManager->cachedRectForID(object.objectID()) : std::nullopt;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 192f347b8b27..794d9ecb8217 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -62,6 +62,7 @@ public:
     AXIsolatedObject* parentObject() const final { return parentObjectUnignored(); }
     AXIsolatedObject* editableAncestor() final { return Accessibility::editableAncestor(*this); };
     bool canSetFocusAttribute() const final { return boolAttributeValue(AXPropertyName::CanSetFocusAttribute); }
+    bool hasContentEditableAttributeSet() const final { return boolAttributeValue(AXPropertyName::HasContentEditableAttributeSet); }

 private:
     void detachRemoteParts(AccessibilityDetachmentType) final;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 7d52c3c0c2bc..63907f552b29 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -569,6 +569,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::AXColumnIndex:
             propertyMap.set(AXPropertyName::AXColumnIndex, axObject.axColumnIndex());
             break;
+        case AXPropertyName::HasContentEditableAttributeSet:
+            propertyMap.set(AXPropertyName::HasContentEditableAttributeSet, axObject.hasContentEditableAttributeSet());
+            break;
         case AXPropertyName::CanSetFocusAttribute:
             propertyMap.set(AXPropertyName::CanSetFocusAttribute, axObject.canSetFocusAttribute());
             break;

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #16)
> (In reply to Joshua Hoffman from comment #15)
> > Created attachment 468547 [details]
> > Patch
> 
> AG: Do we need to differentiate between isNonNativeTextControl() and
> hasContentEditableAttributeSet? or they can be lumped together?

It's possible that we can combine them, but I found the granularity is helpful for things like isAccessibilityElement. However, we could probably be smart about this and/or it may not matter that we also expose non-native text controls as groups as a fallback. I will try this out.

> AG: maybe we should add a handleContentEditableChange method that does both
> update the role and post the appropriate notification.
> 

Yeah, I can definitely clean that up.

[Joshua Hoffman]

(In reply to Joshua Hoffman from comment #17)
> (In reply to Andres Gonzalez from comment #16)
> > (In reply to Joshua Hoffman from comment #15)
> > > Created attachment 468547 [details]
> > > Patch
> > 
> > AG: Do we need to differentiate between isNonNativeTextControl() and
> > hasContentEditableAttributeSet? or they can be lumped together?
> 
> It's possible that we can combine them, but I found the granularity is
> helpful for things like isAccessibilityElement. However, we could probably
> be smart about this and/or it may not matter that we also expose non-native
> text controls as groups as a fallback. I will try this out.

I was able to combine them and not create any issues. I'll upload a new patch with the WebkitTestRunner/DumpRenderTree methods that are associated renamed to match using: isInContentEditable -> isInNonNativeTextArea.

[Joshua Hoffman]

Created attachment 468607 [details]
Patch

[Joshua Hoffman]

Created attachment 468608 [details]
Patch

[Joshua Hoffman]

Created attachment 468610 [details]
Patch

[Joshua Hoffman]

Created attachment 468659 [details]
Patch

[Tyler Wilcock]

Created attachment 468661 [details]
Patch

[EWS]

Committed 270935@main (0524e64fb276): <https://commits.webkit.org/270935@main>

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