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.
<rdar://problem/109699222>
Created attachment 466459 [details] Patch
Created attachment 468207 [details] Patch
Created attachment 468300 [details] Patch
Created attachment 468307 [details] Patch
Created attachment 468318 [details] Patch
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?
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?
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!
Created attachment 468375 [details] Patch
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
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
(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.
Created attachment 468536 [details] Patch
Created attachment 468547 [details] Patch
(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;
(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.
(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.
Created attachment 468607 [details] Patch
Created attachment 468608 [details] Patch
Created attachment 468610 [details] Patch
Created attachment 468659 [details] Patch
Created attachment 468661 [details] Patch
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].