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.
<rdar://problem/116213145>
Created attachment 467968 [details] Patch
Created attachment 467976 [details] Patch
Created attachment 467979 [details] Patch
(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;
Created attachment 467997 [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. 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.
(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.
Created attachment 468033 [details] Patch
> > > 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.
Created attachment 468036 [details] Patch
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].