Bug 261799 – AX: AXWebSessionID, AXIsMultiline, AXReadOnlyValue, AXDocumentURIAttribute, and AXDocumentEncodingAttribute are unused by AX clients

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

We should remove the effectively dead code supporting these attributes.

[Radar WebKit Bug Importer]

<rdar://problem/115764193>

[Tyler Wilcock]

Created attachment 467777 [details]
Patch

[Tyler Wilcock]

Created attachment 467778 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index bed612dfe2e1..d8895c0805d9 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -4132,7 +4132,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             tree->updateNodeProperty(*notification.first, AXPropertyName::IdentifierAttribute);
             break;
         case AXReadOnlyStatusChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::CanSetValueAttribute, AXPropertyName::ReadOnlyValue });
+            tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetValueAttribute);

AG: can we do it the other way around? i.e., remove AXPropertyName::CanSetValueAttribute and keep AXPropertyName::ReadOnlyValue or simply AXPropertyName::ReadOnly.

             break;
         case AXRequiredStatusChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::IsRequired);
diff --git a/Source/WebCore/accessibility/AccessibilityObjectInterface.h b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
index e727148f9bbb..5a0f1723343c 100644
--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -66,10 +66,6 @@ typedef WebCore::AccessibilityObjectAtspi AccessibilityObjectWrapper;
 class AccessibilityObjectWrapper;
 #endif

-namespace PAL {
-class SessionID;
-}
-
 namespace WTF {
 class TextStream;
 }
@@ -1053,7 +1049,7 @@ public:
     virtual String popupValue() const = 0;
     virtual bool supportsHasPopup() const = 0;
     virtual bool pressedIsPresent() const = 0;
-    virtual bool ariaIsMultiline() const = 0;
+    virtual bool ariaIsMultiline() const { return false; }

AG: why do we want to expose a client method that always return false?

     virtual String invalidStatus() const = 0;
     virtual bool supportsExpanded() const = 0;
     virtual bool supportsChecked() const = 0;
@@ -1327,7 +1323,7 @@ public:
     virtual const String liveRegionRelevant() const = 0;
     virtual bool liveRegionAtomic() const = 0;
     virtual bool isBusy() const = 0;
-    virtual String readOnlyValue() const = 0;
+    virtual String readOnlyValue() const { return { }; }

AG: same here?

     virtual String autoCompleteValue() const = 0;

     // Make this object visible by scrolling as many nested scrollable views as needed.
@@ -1427,9 +1423,6 @@ public:
     virtual AXCoreObject* highestEditableAncestor() = 0;
     virtual AXCoreObject* exposedTableAncestor(bool includeSelf = false) const = 0;

-    virtual PAL::SessionID sessionID() const = 0;
-    virtual String documentURI() const = 0;
-    virtual String documentEncoding() const = 0;
     virtual AccessibilityChildrenVector documentLinks() = 0;

     virtual String innerHTML() const = 0;

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 467778 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp
> b/Source/WebCore/accessibility/AXObjectCache.cpp
> index bed612dfe2e1..d8895c0805d9 100644
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ b/Source/WebCore/accessibility/AXObjectCache.cpp
> @@ -4132,7 +4132,7 @@ void AXObjectCache::updateIsolatedTree(const
> Vector<std::pair<RefPtr<Accessibili
>              tree->updateNodeProperty(*notification.first,
> AXPropertyName::IdentifierAttribute);
>              break;
>          case AXReadOnlyStatusChanged:
> -            tree->updateNodeProperties(*notification.first, {
> AXPropertyName::CanSetValueAttribute, AXPropertyName::ReadOnlyValue });
> +            tree->updateNodeProperty(*notification.first,
> AXPropertyName::CanSetValueAttribute);
> 
> AG: can we do it the other way around? i.e., remove
> AXPropertyName::CanSetValueAttribute and keep AXPropertyName::ReadOnlyValue
> or simply AXPropertyName::ReadOnly.
I don't think we could do it with this patch. The other components of CanSetValueAttribute besides readOnlyValue are not available off the main-thread, so caching readOnlyValue wouldn't have any use. We could consider trying to change that,  but seems like work for a future patch.

> @@ -1053,7 +1049,7 @@ public:
>      virtual String popupValue() const = 0;
>      virtual bool supportsHasPopup() const = 0;
>      virtual bool pressedIsPresent() const = 0;
> -    virtual bool ariaIsMultiline() const = 0;
> +    virtual bool ariaIsMultiline() const { return false; }
> 
> AG: why do we want to expose a client method that always return false?
It's virtual, so this declares the default implementation as returning false. It's overridden in AccessibilityObject. The only reason I kept it around at all is because ATSPI uses it, but they don't have ITM enabled, so I feel this is appropriate. Same situation for readOnlyValue().

[Tyler Wilcock]

Created attachment 467799 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> (In reply to Andres Gonzalez from comment #4)
> > (In reply to Tyler Wilcock from comment #3)
> > > Created attachment 467778 [details]
> > > Patch
> > 
> > diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp
> > b/Source/WebCore/accessibility/AXObjectCache.cpp
> > index bed612dfe2e1..d8895c0805d9 100644
> > --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> > +++ b/Source/WebCore/accessibility/AXObjectCache.cpp
> > @@ -4132,7 +4132,7 @@ void AXObjectCache::updateIsolatedTree(const
> > Vector<std::pair<RefPtr<Accessibili
> >              tree->updateNodeProperty(*notification.first,
> > AXPropertyName::IdentifierAttribute);
> >              break;
> >          case AXReadOnlyStatusChanged:
> > -            tree->updateNodeProperties(*notification.first, {
> > AXPropertyName::CanSetValueAttribute, AXPropertyName::ReadOnlyValue });
> > +            tree->updateNodeProperty(*notification.first,
> > AXPropertyName::CanSetValueAttribute);
> > 
> > AG: can we do it the other way around? i.e., remove
> > AXPropertyName::CanSetValueAttribute and keep AXPropertyName::ReadOnlyValue
> > or simply AXPropertyName::ReadOnly.
> I don't think we could do it with this patch. The other components of
> CanSetValueAttribute besides readOnlyValue are not available off the
> main-thread, so caching readOnlyValue wouldn't have any use. We could
> consider trying to change that,  but seems like work for a future patch.

Fine to leave for another patch, but I was actually referring to the naming ReadOnly vs. CanSetValue.
> 
> > @@ -1053,7 +1049,7 @@ public:
> >      virtual String popupValue() const = 0;
> >      virtual bool supportsHasPopup() const = 0;
> >      virtual bool pressedIsPresent() const = 0;
> > -    virtual bool ariaIsMultiline() const = 0;
> > +    virtual bool ariaIsMultiline() const { return false; }
> > 
> > AG: why do we want to expose a client method that always return false?
> It's virtual, so this declares the default implementation as returning
> false. It's overridden in AccessibilityObject. The only reason I kept it
> around at all is because ATSPI uses it, but they don't have ITM enabled, so
> I feel this is appropriate. Same situation for readOnlyValue().

The right thing would be to change ATSPI to use the AccessibilityObject, not the AXCoreObject in these cases and remove them from the AXCoreObject interface.

[Tyler Wilcock]

Created attachment 467801 [details]
Patch

[Tyler Wilcock]

Created attachment 467802 [details]
Patch

[Tyler Wilcock]

> > > AG: can we do it the other way around? i.e., remove
> > > AXPropertyName::CanSetValueAttribute and keep AXPropertyName::ReadOnlyValue
> > > or simply AXPropertyName::ReadOnly.
> > I don't think we could do it with this patch. The other components of
> > CanSetValueAttribute besides readOnlyValue are not available off the
> > main-thread, so caching readOnlyValue wouldn't have any use. We could
> > consider trying to change that,  but seems like work for a future patch.
> 
> Fine to leave for another patch, but I was actually referring to the naming
> ReadOnly vs. CanSetValue.
Ahh, yes I misunderstood. Yes let's consider this for a future patch.

> > > @@ -1053,7 +1049,7 @@ public:
> > >      virtual String popupValue() const = 0;
> > >      virtual bool supportsHasPopup() const = 0;
> > >      virtual bool pressedIsPresent() const = 0;
> > > -    virtual bool ariaIsMultiline() const = 0;
> > > +    virtual bool ariaIsMultiline() const { return false; }
> > > 
> > > AG: why do we want to expose a client method that always return false?
> > It's virtual, so this declares the default implementation as returning
> > false. It's overridden in AccessibilityObject. The only reason I kept it
> > around at all is because ATSPI uses it, but they don't have ITM enabled, so
> > I feel this is appropriate. Same situation for readOnlyValue().
> 
> The right thing would be to change ATSPI to use the AccessibilityObject, not
> the AXCoreObject in these cases and remove them from the AXCoreObject
> interface.
Fixed in latest patch. There are existing casts to AccessibilityObject in ATSPI code, so we can use those.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #10)
> > > > AG: can we do it the other way around? i.e., remove
> > > > AXPropertyName::CanSetValueAttribute and keep AXPropertyName::ReadOnlyValue
> > > > or simply AXPropertyName::ReadOnly.
> > > I don't think we could do it with this patch. The other components of
> > > CanSetValueAttribute besides readOnlyValue are not available off the
> > > main-thread, so caching readOnlyValue wouldn't have any use. We could
> > > consider trying to change that,  but seems like work for a future patch.
> > 
> > Fine to leave for another patch, but I was actually referring to the naming
> > ReadOnly vs. CanSetValue.
> Ahh, yes I misunderstood. Yes let's consider this for a future patch.
> 
> > > > @@ -1053,7 +1049,7 @@ public:
> > > >      virtual String popupValue() const = 0;
> > > >      virtual bool supportsHasPopup() const = 0;
> > > >      virtual bool pressedIsPresent() const = 0;
> > > > -    virtual bool ariaIsMultiline() const = 0;
> > > > +    virtual bool ariaIsMultiline() const { return false; }
> > > > 
> > > > AG: why do we want to expose a client method that always return false?
> > > It's virtual, so this declares the default implementation as returning
> > > false. It's overridden in AccessibilityObject. The only reason I kept it
> > > around at all is because ATSPI uses it, but they don't have ITM enabled, so
> > > I feel this is appropriate. Same situation for readOnlyValue().
> > 
> > The right thing would be to change ATSPI to use the AccessibilityObject, not
> > the AXCoreObject in these cases and remove them from the AXCoreObject
> > interface.
> Fixed in latest patch. There are existing casts to AccessibilityObject in
> ATSPI code, so we can use those.

Great, Thanks!

[Tyler Wilcock]

Created attachment 467806 [details]
Patch

[Tyler Wilcock]

Created attachment 467807 [details]
Patch

[EWS]

Committed 268256@main (3fc18de4dfcd): <https://commits.webkit.org/268256@main>

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