Bug 262351 – AX: AccessibilityRenderObject::updateRoleAfterChildrenCreation causes unnecessary AXRoleChanged notifications for several subclasses

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

AccessibilityRenderObject::updateRoleAfterChildrenCreation is predicated on the assumption that the class uses m_role. However, many subclasses don't, instead hard-coding a value like so:

AccessibilityRole roleValue() const final { return AccessibilityRole::Label; }

This means that m_role (not actually used) may differ from roleValue(), causing updateRoleAfterChildrenCreation to post a spurious AXRoleChanged notification every time the object's children are cleared and re-added, in turn causing lots of wasted work.

[Radar WebKit Bug Importer]

<rdar://problem/116213145>

[Tyler Wilcock]

Created attachment 467968 [details]
Patch

[Tyler Wilcock]

Created attachment 467976 [details]
Patch

[Tyler Wilcock]

Created attachment 467979 [details]
Patch

[Andres Gonzalez]

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

AG: can we just have AccessibilityObject::roleValue() final { return m_role; } and each subclass initializes it on construction? Those classes that can change role, need to override determineRole(). So when the role may have changed, we compare m_role to determineRole() and update m_role.

diff --git a/Source/WebCore/accessibility/AXImage.cpp b/Source/WebCore/accessibility/AXImage.cpp
index 5aca82233177..77f7c79be0af 100644
--- a/Source/WebCore/accessibility/AXImage.cpp
+++ b/Source/WebCore/accessibility/AXImage.cpp
@@ -47,12 +47,10 @@ Ref<AXImage> AXImage::create(RenderImage* renderer)
     return adoptRef(*new AXImage(renderer));
 }

-AccessibilityRole AXImage::roleValue() const
+AccessibilityRole AXImage::determineAccessibilityRole()
 {
-    auto ariaRole = ariaRoleAttribute();
-    if (ariaRole != AccessibilityRole::Unknown)
-        return ariaRole;
-
+    if ((m_ariaRole = determineAriaRoleAttribute()) != AccessibilityRole::Unknown)
+        return m_ariaRole;
     return AccessibilityRole::Image;
 }

diff --git a/Source/WebCore/accessibility/AXImage.h b/Source/WebCore/accessibility/AXImage.h
index 84099b3b2572..2f8ef01e5371 100644
--- a/Source/WebCore/accessibility/AXImage.h
+++ b/Source/WebCore/accessibility/AXImage.h
@@ -41,7 +41,7 @@ public:
 private:
     explicit AXImage(RenderImage*);

-    AccessibilityRole roleValue() const override;
+    AccessibilityRole determineAccessibilityRole() final;
     std::optional<AccessibilityChildrenVector> imageOverlayElements() override;
 };

diff --git a/Source/WebCore/accessibility/AccessibilityAttachment.h b/Source/WebCore/accessibility/AccessibilityAttachment.h
index 633b0e66ee21..b9a52f5b5e3d 100644
--- a/Source/WebCore/accessibility/AccessibilityAttachment.h
+++ b/Source/WebCore/accessibility/AccessibilityAttachment.h
@@ -42,7 +42,8 @@ public:
     bool hasProgress(float* progress = nullptr) const;

 private:
-    AccessibilityRole roleValue() const override { return AccessibilityRole::Button; }
+    AccessibilityRole determineAccessibilityRole() final { return AccessibilityRole::Button; }
+
     bool isAttachmentElement() const override { return true; }

     String roleDescription() const override;
diff --git a/Source/WebCore/accessibility/AccessibilityImageMapLink.h b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
index aff90d2d897d..a2346d7d55c9 100644
--- a/Source/WebCore/accessibility/AccessibilityImageMapLink.h
+++ b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
@@ -47,7 +47,7 @@ public:

     Node* node() const override { return m_areaElement.get(); }

-    AccessibilityRole roleValue() const override;
+    AccessibilityRole roleValue() const final;

AG: shouldn't we change to determineRole() too?

     bool isEnabled() const override { return true; }

     Element* anchorElement() const override;
diff --git a/Source/WebCore/accessibility/AccessibilityLabel.h b/Source/WebCore/accessibility/AccessibilityLabel.h
index 29e953d0b79d..551063ab806e 100644
--- a/Source/WebCore/accessibility/AccessibilityLabel.h
+++ b/Source/WebCore/accessibility/AccessibilityLabel.h
@@ -43,7 +43,9 @@ public:
 private:
     explicit AccessibilityLabel(RenderObject*);
     bool computeAccessibilityIsIgnored() const final;
-    AccessibilityRole roleValue() const final { return AccessibilityRole::Label; }
+
+    AccessibilityRole determineAccessibilityRole() final { return AccessibilityRole::Label; }
+
     bool isLabel() const final { return true; }
     String stringValue() const final;
     void updateChildrenIfNecessary() final;
diff --git a/Source/WebCore/accessibility/AccessibilityList.h b/Source/WebCore/accessibility/AccessibilityList.h
index 0e4080bfa4a5..631b8188bb0c 100644
--- a/Source/WebCore/accessibility/AccessibilityList.h
+++ b/Source/WebCore/accessibility/AccessibilityList.h
@@ -38,7 +38,7 @@ public:
     static Ref<AccessibilityList> create(Node&);
     virtual ~AccessibilityList();

-    AccessibilityRole roleValue() const override;
+    AccessibilityRole roleValue() const final;

AG: determineRole instead ?

 private:
     explicit AccessibilityList(RenderObject*);
     explicit AccessibilityList(Node&);
diff --git a/Source/WebCore/accessibility/AccessibilityListBox.h b/Source/WebCore/accessibility/AccessibilityListBox.h
index f2f9a4f94a52..4b63e64c839e 100644
--- a/Source/WebCore/accessibility/AccessibilityListBox.h
+++ b/Source/WebCore/accessibility/AccessibilityListBox.h
@@ -39,8 +39,9 @@ public:

     bool canSetSelectedChildren() const override;
     WEBCORE_EXPORT void setSelectedChildren(const AccessibilityChildrenVector&) override;
-    AccessibilityRole roleValue() const override { return AccessibilityRole::ListBox; }
-
+
+    AccessibilityRole determineAccessibilityRole() final { return AccessibilityRole::ListBox; }
+
     AccessibilityChildrenVector selectedChildren() final;
     AccessibilityChildrenVector visibleChildren() final;

diff --git a/Source/WebCore/accessibility/AccessibilityMenuList.h b/Source/WebCore/accessibility/AccessibilityMenuList.h
index 89c83dbfb313..539aa215aec9 100644
--- a/Source/WebCore/accessibility/AccessibilityMenuList.h
+++ b/Source/WebCore/accessibility/AccessibilityMenuList.h
@@ -44,7 +44,8 @@ private:
     explicit AccessibilityMenuList(RenderMenuList*);

     bool isMenuList() const final { return true; }
-    AccessibilityRole roleValue() const override { return AccessibilityRole::PopUpButton; }
+    AccessibilityRole determineAccessibilityRole() final { return AccessibilityRole::PopUpButton; }
+
     bool canSetFocusAttribute() const override;

     void addChildren() override;
diff --git a/Source/WebCore/accessibility/AccessibilityMenuListPopup.h b/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
index 3e959c260816..a96244a939a7 100644
--- a/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
+++ b/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
@@ -49,7 +49,7 @@ private:
     bool isMenuListPopup() const final { return true; }

     LayoutRect elementRect() const final { return LayoutRect(); }
-    AccessibilityRole roleValue() const override { return AccessibilityRole::MenuListPopup; }
+    AccessibilityRole roleValue() const final { return AccessibilityRole::MenuListPopup; }

AG: same ?

     bool isVisible() const override;
     bool press() override;
diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.h b/Source/WebCore/accessibility/AccessibilityNodeObject.h
index 458c3cd95662..544877251596 100644
--- a/Source/WebCore/accessibility/AccessibilityNodeObject.h
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.h
@@ -50,6 +50,9 @@ public:
     virtual ~AccessibilityNodeObject();

     void init() override;
+    // FIXME: This should be made final after AccessibilityTable is fixed to use m_role
+    // rather than computing its own roleValue().
+    AccessibilityRole roleValue() const override { return m_role; }

     bool canvasHasFallbackContent() const override;

@@ -149,6 +152,7 @@ protected:
     explicit AccessibilityNodeObject(Node*);
     void detachRemoteParts(AccessibilityDetachmentType) override;

+    AccessibilityRole m_role { AccessibilityRole::Unknown };
     AccessibilityRole m_ariaRole { AccessibilityRole::Unknown };
 #ifndef NDEBUG
     bool m_initialized { false };
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index 7d9a20498021..b2882c4e079d 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -415,7 +415,6 @@ public:
     // Only if isColorWell()
     SRGBA<uint8_t> colorValue() const override;

-    AccessibilityRole roleValue() const override { return m_role; }
     String rolePlatformString() const override;
     String roleDescription() const override;
     String subrolePlatformString() const override;
@@ -830,7 +829,6 @@ private:
 protected: // FIXME: Make the data members private.
     AccessibilityChildrenVector m_children;
     mutable bool m_childrenInitialized { false };
-    AccessibilityRole m_role { AccessibilityRole::Unknown };
 private:
     OptionSet<AXAncestorFlag> m_ancestorFlags;
     AccessibilityObjectInclusion m_lastKnownIsIgnoredValue { AccessibilityObjectInclusion::DefaultBehavior };
diff --git a/Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp b/Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp
index cc625467f684..d7d66ed717b7 100644
--- a/Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp
+++ b/Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp
@@ -118,7 +118,7 @@ float AccessibilityProgressIndicator::minValueForRange() const
     return 0.0;
 }

-AccessibilityRole AccessibilityProgressIndicator::roleValue() const
+AccessibilityRole AccessibilityProgressIndicator::determineAccessibilityRole()
 {
     if (meterElement())
         return AccessibilityRole::Meter;
diff --git a/Source/WebCore/accessibility/AccessibilityProgressIndicator.h b/Source/WebCore/accessibility/AccessibilityProgressIndicator.h
index b79ff53d18a7..89e17c349c9e 100644
--- a/Source/WebCore/accessibility/AccessibilityProgressIndicator.h
+++ b/Source/WebCore/accessibility/AccessibilityProgressIndicator.h
@@ -37,7 +37,7 @@ public:
     bool isIndeterminate() const final;

 private:
-    AccessibilityRole roleValue() const override;
+    AccessibilityRole determineAccessibilityRole() final;

     String valueDescription() const override;
     String gaugeRegionValueDescription() const;
diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
index aad9b2903d99..99228415aaa2 100644
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -2038,11 +2038,6 @@ AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()
     if (m_renderer->isImage()) {
         if (is<HTMLInputElement>(node))
             return hasPopup() ? AccessibilityRole::PopUpButton : AccessibilityRole::Button;
-
-        if (auto* svgRoot = remoteSVGRootElement(Create)) {
-            if (svgRoot->hasAccessibleContent())
-                return AccessibilityRole::SVGRoot;
-        }
         return AccessibilityRole::Image;
     }

@@ -2397,8 +2392,6 @@ void AccessibilityRenderObject::updateRoleAfterChildrenCreation()
         if (!menuItemCount)
             m_role = AccessibilityRole::Group;
     }
-    if (role == AccessibilityRole::SVGRoot && !children().size())
-        m_role = AccessibilityRole::Image;

     if (role != m_role) {
         if (auto* cache = axObjectCache())
diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.h b/Source/WebCore/accessibility/AccessibilityRenderObject.h
index 54aca6fa8ae2..d5c03c55a03a 100644
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.h
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.h
@@ -189,8 +189,8 @@ private:
 #endif
     String expandedTextValue() const override;
     bool supportsExpandedTextValue() const override;
-    void updateRoleAfterChildrenCreation();
-
+    virtual void updateRoleAfterChildrenCreation();
+
     bool inheritsPresentationalRole() const override;

     bool shouldGetTextFromNode(AccessibilityTextUnderElementMode) const;
diff --git a/Source/WebCore/accessibility/AccessibilitySVGElement.h b/Source/WebCore/accessibility/AccessibilitySVGElement.h
index 3e9988f9c2d2..65ad7705a123 100644
--- a/Source/WebCore/accessibility/AccessibilitySVGElement.h
+++ b/Source/WebCore/accessibility/AccessibilitySVGElement.h
@@ -39,13 +39,13 @@ public:
 protected:
     explicit AccessibilitySVGElement(RenderObject*, AXObjectCache*);
     AXObjectCache* axObjectCache() const override { return m_axObjectCache.get(); }
+    AccessibilityRole determineAriaRoleAttribute() const final;

 private:
     String description() const final;
     String helpText() const final;
     void accessibilityText(Vector<AccessibilityText>&) const final;
-    AccessibilityRole determineAccessibilityRole() final;
-    AccessibilityRole determineAriaRoleAttribute() const final;
+    AccessibilityRole determineAccessibilityRole() override;
     bool inheritsPresentationalRole() const final;
     bool computeAccessibilityIsIgnored() const final;

diff --git a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
index e32cb858e0ce..5a999eb4b3c5 100644
--- a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
+++ b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
@@ -66,52 +66,11 @@ AccessibilityObject* AccessibilitySVGRoot::parentObject() const
     return AccessibilitySVGElement::parentObject();
 }

-AccessibilityRole AccessibilitySVGRoot::roleValue() const
+AccessibilityRole AccessibilitySVGRoot::determineAccessibilityRole()
 {
-    AccessibilityRole ariaRole = ariaRoleAttribute();
-    if (ariaRole != AccessibilityRole::Unknown)
-        return ariaRole;
-
-    return AccessibilityRole::Group;
-}
-
-bool AccessibilitySVGRoot::hasAccessibleContent() const

AG: why are we removing this method and its use?

-{
-    auto* rootElement = this->element();
-    if (!rootElement)
-        return false;
-
-    auto isAccessibleSVGElement = [] (const Element& element) -> bool {
-        if (!is<SVGElement>(element))
-            return false;
-
-        // The presence of an SVGTitle or SVGDesc element is enough to deem the SVG hierarchy as accessible.
-        if (is<SVGTitleElement>(element)
-            || is<SVGDescElement>(element))
-            return true;
-
-        // Text content is accessible.
-        if (downcast<SVGElement>(element).isTextContent())
-            return true;
-
-        // If the role or aria-label attributes are specified, this is accessible.
-        if (!element.attributeWithoutSynchronization(HTMLNames::roleAttr).isEmpty()
-            || !element.attributeWithoutSynchronization(HTMLNames::aria_labelAttr).isEmpty())
-            return true;
-
-        return false;
-    };
-
-    if (isAccessibleSVGElement(*rootElement))
-        return true;
-
-    // This SVG hierarchy is accessible if any of its descendants is accessible.
-    for (const auto& descendant : descendantsOfType<SVGElement>(*rootElement)) {
-        if (isAccessibleSVGElement(descendant))
-            return true;
-    }
-
-    return false;
+    if ((m_ariaRole = determineAriaRoleAttribute()) != AccessibilityRole::Unknown)
+        return m_ariaRole;
+    return AccessibilityRole::SVGRoot;
 }

 } // namespace WebCore
diff --git a/Source/WebCore/accessibility/AccessibilitySVGRoot.h b/Source/WebCore/accessibility/AccessibilitySVGRoot.h
index a7cb397e83b2..85a9d9054937 100644
--- a/Source/WebCore/accessibility/AccessibilitySVGRoot.h
+++ b/Source/WebCore/accessibility/AccessibilitySVGRoot.h
@@ -39,13 +39,13 @@ public:
     virtual ~AccessibilitySVGRoot();

     void setParent(AccessibilityRenderObject*);
-    bool hasAccessibleContent() const;
 private:
     explicit AccessibilitySVGRoot(RenderObject*, AXObjectCache*);

     AccessibilityObject* parentObject() const override;
     bool isAccessibilitySVGRoot() const override { return true; }
-    AccessibilityRole roleValue() const override;
+
+    AccessibilityRole determineAccessibilityRole() final;

     WeakPtr<AccessibilityRenderObject> m_parent;
 };
diff --git a/Source/WebCore/accessibility/AccessibilityScrollView.h b/Source/WebCore/accessibility/AccessibilityScrollView.h
index 248f707763bb..fd00691a697b 100644
--- a/Source/WebCore/accessibility/AccessibilityScrollView.h
+++ b/Source/WebCore/accessibility/AccessibilityScrollView.h
@@ -37,7 +37,7 @@ class ScrollView;
 class AccessibilityScrollView final : public AccessibilityObject {
 public:
     static Ref<AccessibilityScrollView> create(ScrollView*);
-    AccessibilityRole roleValue() const override { return AccessibilityRole::ScrollArea; }
+    AccessibilityRole roleValue() const final { return AccessibilityRole::ScrollArea; }

AG: same ?

     ScrollView* scrollView() const override { return currentScrollView(); }

     virtual ~AccessibilityScrollView();
diff --git a/Source/WebCore/accessibility/AccessibilityScrollbar.h b/Source/WebCore/accessibility/AccessibilityScrollbar.h
index 1b92a9995100..8ffc46923c6b 100644
--- a/Source/WebCore/accessibility/AccessibilityScrollbar.h
+++ b/Source/WebCore/accessibility/AccessibilityScrollbar.h
@@ -48,7 +48,7 @@ private:
     bool isAccessibilityScrollbar() const override { return true; }
     LayoutRect elementRect() const override;

-    AccessibilityRole roleValue() const override { return AccessibilityRole::ScrollBar; }
+    AccessibilityRole roleValue() const final { return AccessibilityRole::ScrollBar; }

AG: same ?

     AccessibilityOrientation orientation() const override;
     Document* document() const override;
     bool isEnabled() const override;
diff --git a/Source/WebCore/accessibility/AccessibilitySlider.h b/Source/WebCore/accessibility/AccessibilitySlider.h
index 2c145e35de31..24dc6378b525 100644
--- a/Source/WebCore/accessibility/AccessibilitySlider.h
+++ b/Source/WebCore/accessibility/AccessibilitySlider.h
@@ -47,7 +47,8 @@ private:
     HTMLInputElement* inputElement() const;
     AXCoreObject* elementAccessibilityHitTest(const IntPoint&) const override;

-    AccessibilityRole roleValue() const override { return AccessibilityRole::Slider; }
+    AccessibilityRole determineAccessibilityRole() final { return AccessibilityRole::Slider; }
+
     bool isControl() const override { return true; }

     void addChildren() override;
@@ -67,7 +68,7 @@ public:
     static Ref<AccessibilitySliderThumb> create();
     virtual ~AccessibilitySliderThumb() = default;

-    AccessibilityRole roleValue() const override { return AccessibilityRole::SliderThumb; }
+    AccessibilityRole roleValue() const final { return AccessibilityRole::SliderThumb; }

AG: same ?

     LayoutRect elementRect() const override;

 private:
diff --git a/Source/WebCore/accessibility/AccessibilityTable.h b/Source/WebCore/accessibility/AccessibilityTable.h
index c96bc6c9eeea..470a96b56afd 100644
--- a/Source/WebCore/accessibility/AccessibilityTable.h
+++ b/Source/WebCore/accessibility/AccessibilityTable.h
@@ -45,6 +45,10 @@ public:
     void init() final;

     AccessibilityRole roleValue() const final;
+    // Override updateRoleAfterChildrenCreation and updateRole because this class does not use m_role, and those functions
+    // assume it does. We should fix this so behavior is unified with other AccessibilityNodeObject subclasses.

AG: add FIXME to the above comment.

+    void updateRole() final { }
+    void updateRoleAfterChildrenCreation() final { }
     virtual bool isAriaTable() const { return false; }

     void addChildren() final;

[Tyler Wilcock]

Created attachment 467997 [details]
Patch

[Tyler Wilcock]

> AG: can we just have AccessibilityObject::roleValue() final { return m_role;
> } and each subclass initializes it on construction? Those classes that can
> change role, need to override determineRole(). So when the role may have
> changed, we compare m_role to determineRole() and update m_role.
I think that's effectively what happens now, just not in the constructor. After the object is created in the AXObjectCache, one of the first things that happens is AccessibilityObject::init, which is overridden in AccessibilityNodeObject to initialize m_role.

> diff --git a/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> index aff90d2d897d..a2346d7d55c9 100644
> --- a/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> +++ b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> @@ -47,7 +47,7 @@ public:
> 
>      Node* node() const override { return m_areaElement.get(); }
> 
> -    AccessibilityRole roleValue() const override;
> +    AccessibilityRole roleValue() const final;
> 
> AG: shouldn't we change to determineRole() too?
No need on this one. In practice, m_role was only used by AccessibilityNodeObject subclasses, so this patch makes that more clear by moving it to AccessibilityNodeObject, also saving memory for other non-AccessibilityNodeObject-subclasses. AccessibilityImageMapLink falls into this group: is it a descendant of AccessibilityMockObject, not AccessibilityNodeObject.

> diff --git a/Source/WebCore/accessibility/AccessibilityList.h
> b/Source/WebCore/accessibility/AccessibilityList.h
> index 0e4080bfa4a5..631b8188bb0c 100644
> --- a/Source/WebCore/accessibility/AccessibilityList.h
> +++ b/Source/WebCore/accessibility/AccessibilityList.h
> @@ -38,7 +38,7 @@ public:
>      static Ref<AccessibilityList> create(Node&);
>      virtual ~AccessibilityList();
> 
> -    AccessibilityRole roleValue() const override;
> +    AccessibilityRole roleValue() const final;
> 
> AG: determineRole instead ?
AccessibilityList's override of roleValue() just adds an assert:

AccessibilityRole AccessibilityList::roleValue() const
{
    ASSERT(m_role != AccessibilityRole::Unknown);
    return m_role;
}

AccessibilityList also already overrides determineRole.

> --- a/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
> +++ b/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
> @@ -49,7 +49,7 @@ private:
>      bool isMenuListPopup() const final { return true; }
> 
>      LayoutRect elementRect() const final { return LayoutRect(); }
> -    AccessibilityRole roleValue() const override { return
> AccessibilityRole::MenuListPopup; }
> +    AccessibilityRole roleValue() const final { return
> AccessibilityRole::MenuListPopup; }
> 
> AG: same ?
Rationale same as AccessibilityImageMapLink above (this is a mock object descendant, not a node object one).

> diff --git a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> index e32cb858e0ce..5a999eb4b3c5 100644
> --- a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> +++ b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> @@ -66,52 +66,11 @@ AccessibilityObject*
> AccessibilitySVGRoot::parentObject() const
>      return AccessibilitySVGElement::parentObject();
>  }
> 
> -AccessibilityRole AccessibilitySVGRoot::roleValue() const
> +AccessibilityRole AccessibilitySVGRoot::determineAccessibilityRole()
>  {
> -    AccessibilityRole ariaRole = ariaRoleAttribute();
> -    if (ariaRole != AccessibilityRole::Unknown)
> -        return ariaRole;
> -
> -    return AccessibilityRole::Group;
> -}
> -
> -bool AccessibilitySVGRoot::hasAccessibleContent() const
> 
> AG: why are we removing this method and its use?
Because in today's main, it does nothing. It served only to set up m_role for AccessibilitySVGRoot, but AccessibilitySVGRoot's roleValue() implementation has never used that result because it doesn't use m_role (until after my patch). I decided to remove it since it has been broken since introduced, and removing it causes no tests to fail.

> diff --git a/Source/WebCore/accessibility/AccessibilityScrollView.h
> b/Source/WebCore/accessibility/AccessibilityScrollView.h
> index 248f707763bb..fd00691a697b 100644
> --- a/Source/WebCore/accessibility/AccessibilityScrollView.h
> +++ b/Source/WebCore/accessibility/AccessibilityScrollView.h
> @@ -37,7 +37,7 @@ class ScrollView;
>  class AccessibilityScrollView final : public AccessibilityObject {
>  public:
>      static Ref<AccessibilityScrollView> create(ScrollView*);
> -    AccessibilityRole roleValue() const override { return
> AccessibilityRole::ScrollArea; }
> +    AccessibilityRole roleValue() const final { return
> AccessibilityRole::ScrollArea; }
> 
> AG: same ?
Rationale same as AccessibilityImageMapLink above (this is an AccessibilityObject descendant, not a node object one).

> diff --git a/Source/WebCore/accessibility/AccessibilityScrollbar.h
> b/Source/WebCore/accessibility/AccessibilityScrollbar.h
> index 1b92a9995100..8ffc46923c6b 100644
> --- a/Source/WebCore/accessibility/AccessibilityScrollbar.h
> +++ b/Source/WebCore/accessibility/AccessibilityScrollbar.h
> @@ -48,7 +48,7 @@ private:
>      bool isAccessibilityScrollbar() const override { return true; }
>      LayoutRect elementRect() const override;
> 
> -    AccessibilityRole roleValue() const override { return
> AccessibilityRole::ScrollBar; }
> +    AccessibilityRole roleValue() const final { return
> AccessibilityRole::ScrollBar; }
> 
> AG: same ?
Rationale same as AccessibilityImageMapLink above (this is a mock object descendant, not a node object one).

> @@ -67,7 +68,7 @@ public:
>      static Ref<AccessibilitySliderThumb> create();
>      virtual ~AccessibilitySliderThumb() = default;
> 
> -    AccessibilityRole roleValue() const override { return
> AccessibilityRole::SliderThumb; }
> +    AccessibilityRole roleValue() const final { return
> AccessibilityRole::SliderThumb; }
> 
> AG: same ?
Rationale same as AccessibilityImageMapLink above (this is a mock object descendant, not a node object one).

> --- a/Source/WebCore/accessibility/AccessibilityTable.h
> +++ b/Source/WebCore/accessibility/AccessibilityTable.h
> @@ -45,6 +45,10 @@ public:
>      void init() final;
> 
>      AccessibilityRole roleValue() const final;
> +    // Override updateRoleAfterChildrenCreation and updateRole because this
> class does not use m_role, and those functions
> +    // assume it does. We should fix this so behavior is unified with other
> AccessibilityNodeObject subclasses.
> 
> AG: add FIXME to the above comment.
Added in latest patch.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> > AG: can we just have AccessibilityObject::roleValue() final { return m_role;
> > } and each subclass initializes it on construction? Those classes that can
> > change role, need to override determineRole(). So when the role may have
> > changed, we compare m_role to determineRole() and update m_role.
> I think that's effectively what happens now, just not in the constructor.
> After the object is created in the AXObjectCache, one of the first things
> that happens is AccessibilityObject::init, which is overridden in
> AccessibilityNodeObject to initialize m_role.
> 
> > diff --git a/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> > b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> > index aff90d2d897d..a2346d7d55c9 100644
> > --- a/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> > +++ b/Source/WebCore/accessibility/AccessibilityImageMapLink.h
> > @@ -47,7 +47,7 @@ public:
> > 
> >      Node* node() const override { return m_areaElement.get(); }
> > 
> > -    AccessibilityRole roleValue() const override;
> > +    AccessibilityRole roleValue() const final;
> > 
> > AG: shouldn't we change to determineRole() too?
> No need on this one. In practice, m_role was only used by
> AccessibilityNodeObject subclasses, so this patch makes that more clear by
> moving it to AccessibilityNodeObject, also saving memory for other
> non-AccessibilityNodeObject-subclasses. AccessibilityImageMapLink falls into
> this group: is it a descendant of AccessibilityMockObject, not
> AccessibilityNodeObject.

We are not saving any memory since m_role is a member of AccessibilityObject. It just makes it more complicated that some classes override roleValue and others don't.

> 
> > diff --git a/Source/WebCore/accessibility/AccessibilityList.h
> > b/Source/WebCore/accessibility/AccessibilityList.h
> > index 0e4080bfa4a5..631b8188bb0c 100644
> > --- a/Source/WebCore/accessibility/AccessibilityList.h
> > +++ b/Source/WebCore/accessibility/AccessibilityList.h
> > @@ -38,7 +38,7 @@ public:
> >      static Ref<AccessibilityList> create(Node&);
> >      virtual ~AccessibilityList();
> > 
> > -    AccessibilityRole roleValue() const override;
> > +    AccessibilityRole roleValue() const final;
> > 
> > AG: determineRole instead ?
> AccessibilityList's override of roleValue() just adds an assert:
> 
> AccessibilityRole AccessibilityList::roleValue() const
> {
>     ASSERT(m_role != AccessibilityRole::Unknown);
>     return m_role;
> }
> 
> AccessibilityList also already overrides determineRole.

AG: should we then move the assert to determineRole() and get rid of the roleValue() override?

> 
> > --- a/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
> > +++ b/Source/WebCore/accessibility/AccessibilityMenuListPopup.h
> > @@ -49,7 +49,7 @@ private:
> >      bool isMenuListPopup() const final { return true; }
> > 
> >      LayoutRect elementRect() const final { return LayoutRect(); }
> > -    AccessibilityRole roleValue() const override { return
> > AccessibilityRole::MenuListPopup; }
> > +    AccessibilityRole roleValue() const final { return
> > AccessibilityRole::MenuListPopup; }
> > 
> > AG: same ?
> Rationale same as AccessibilityImageMapLink above (this is a mock object
> descendant, not a node object one).
> 
> > diff --git a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > index e32cb858e0ce..5a999eb4b3c5 100644
> > --- a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > +++ b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > @@ -66,52 +66,11 @@ AccessibilityObject*
> > AccessibilitySVGRoot::parentObject() const
> >      return AccessibilitySVGElement::parentObject();
> >  }
> > 
> > -AccessibilityRole AccessibilitySVGRoot::roleValue() const
> > +AccessibilityRole AccessibilitySVGRoot::determineAccessibilityRole()
> >  {
> > -    AccessibilityRole ariaRole = ariaRoleAttribute();
> > -    if (ariaRole != AccessibilityRole::Unknown)
> > -        return ariaRole;
> > -
> > -    return AccessibilityRole::Group;
> > -}
> > -
> > -bool AccessibilitySVGRoot::hasAccessibleContent() const
> > 
> > AG: why are we removing this method and its use?
> Because in today's main, it does nothing. It served only to set up m_role
> for AccessibilitySVGRoot, but AccessibilitySVGRoot's roleValue()
> implementation has never used that result because it doesn't use m_role
> (until after my patch). I decided to remove it since it has been broken
> since introduced, and removing it causes no tests to fail.

I remember adding this and testing it, although may have not added a test. This was a fix to a bug where we want to speak the SVG element as an image if there is no accessible content inside it, but otherwise expose that hierarchy of accessible content. I will dig further to see if that is broken again and add test if there is none.

> 
> > diff --git a/Source/WebCore/accessibility/AccessibilityScrollView.h
> > b/Source/WebCore/accessibility/AccessibilityScrollView.h
> > index 248f707763bb..fd00691a697b 100644
> > --- a/Source/WebCore/accessibility/AccessibilityScrollView.h
> > +++ b/Source/WebCore/accessibility/AccessibilityScrollView.h
> > @@ -37,7 +37,7 @@ class ScrollView;
> >  class AccessibilityScrollView final : public AccessibilityObject {
> >  public:
> >      static Ref<AccessibilityScrollView> create(ScrollView*);
> > -    AccessibilityRole roleValue() const override { return
> > AccessibilityRole::ScrollArea; }
> > +    AccessibilityRole roleValue() const final { return
> > AccessibilityRole::ScrollArea; }
> > 
> > AG: same ?
> Rationale same as AccessibilityImageMapLink above (this is an
> AccessibilityObject descendant, not a node object one).
> 
> > diff --git a/Source/WebCore/accessibility/AccessibilityScrollbar.h
> > b/Source/WebCore/accessibility/AccessibilityScrollbar.h
> > index 1b92a9995100..8ffc46923c6b 100644
> > --- a/Source/WebCore/accessibility/AccessibilityScrollbar.h
> > +++ b/Source/WebCore/accessibility/AccessibilityScrollbar.h
> > @@ -48,7 +48,7 @@ private:
> >      bool isAccessibilityScrollbar() const override { return true; }
> >      LayoutRect elementRect() const override;
> > 
> > -    AccessibilityRole roleValue() const override { return
> > AccessibilityRole::ScrollBar; }
> > +    AccessibilityRole roleValue() const final { return
> > AccessibilityRole::ScrollBar; }
> > 
> > AG: same ?
> Rationale same as AccessibilityImageMapLink above (this is a mock object
> descendant, not a node object one).
> 
> > @@ -67,7 +68,7 @@ public:
> >      static Ref<AccessibilitySliderThumb> create();
> >      virtual ~AccessibilitySliderThumb() = default;
> > 
> > -    AccessibilityRole roleValue() const override { return
> > AccessibilityRole::SliderThumb; }
> > +    AccessibilityRole roleValue() const final { return
> > AccessibilityRole::SliderThumb; }
> > 
> > AG: same ?
> Rationale same as AccessibilityImageMapLink above (this is a mock object
> descendant, not a node object one).
> 
> > --- a/Source/WebCore/accessibility/AccessibilityTable.h
> > +++ b/Source/WebCore/accessibility/AccessibilityTable.h
> > @@ -45,6 +45,10 @@ public:
> >      void init() final;
> > 
> >      AccessibilityRole roleValue() const final;
> > +    // Override updateRoleAfterChildrenCreation and updateRole because this
> > class does not use m_role, and those functions
> > +    // assume it does. We should fix this so behavior is unified with other
> > AccessibilityNodeObject subclasses.
> > 
> > AG: add FIXME to the above comment.
> Added in latest patch.

[Tyler Wilcock]

Created attachment 468033 [details]
Patch

[Tyler Wilcock]

> > > diff --git a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > > b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > > index e32cb858e0ce..5a999eb4b3c5 100644
> > > --- a/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > > +++ b/Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
> > > @@ -66,52 +66,11 @@ AccessibilityObject*
> > > AccessibilitySVGRoot::parentObject() const
> > >      return AccessibilitySVGElement::parentObject();
> > >  }
> > > 
> > > -AccessibilityRole AccessibilitySVGRoot::roleValue() const
> > > +AccessibilityRole AccessibilitySVGRoot::determineAccessibilityRole()
> > >  {
> > > -    AccessibilityRole ariaRole = ariaRoleAttribute();
> > > -    if (ariaRole != AccessibilityRole::Unknown)
> > > -        return ariaRole;
> > > -
> > > -    return AccessibilityRole::Group;
> > > -}
> > > -
> > > -bool AccessibilitySVGRoot::hasAccessibleContent() const
> > > 
> > > AG: why are we removing this method and its use?
> > Because in today's main, it does nothing. It served only to set up m_role
> > for AccessibilitySVGRoot, but AccessibilitySVGRoot's roleValue()
> > implementation has never used that result because it doesn't use m_role
> > (until after my patch). I decided to remove it since it has been broken
> > since introduced, and removing it causes no tests to fail.
> 
> I remember adding this and testing it, although may have not added a test.
> This was a fix to a bug where we want to speak the SVG element as an image
> if there is no accessible content inside it, but otherwise expose that
> hierarchy of accessible content. I will dig further to see if that is broken
> again and add test if there is none.
I backed out these changes so that I can move forward past this patch.

All of your requested changes have been applied in latest patch.

[Tyler Wilcock]

Created attachment 468036 [details]
Patch

[EWS]

Committed 268789@main (41d4dd496795): <https://commits.webkit.org/268789@main>

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