Bug 261791 – AX: AXPropertyName::IsExpanded becomes stale for certain elements with the popovertarget attribute

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Multiple elements can point at a single popover:

<button id="show-popover-btn" popovertarget="mypopover" popovertargetaction="show">Show popover</button>
<button id="hide-popover-btn" popovertarget="mypopover" popovertargetaction="hide">Hide popover</button>
<div id="mypopover" popover>Popover content</div>

Currently, if #show-popover-btn is used to open the popover, its AXPropertyName::IsExpanded will be updated, but not #hide-popover-btn's. This can cause ATs to output stale information.

[Radar WebKit Bug Importer]

<rdar://problem/115758014>

[Tyler Wilcock]

Created attachment 467771 [details]
Patch

[Tyler Wilcock]

Created attachment 467773 [details]
Patch

[Tyler Wilcock]

Created attachment 467775 [details]
Patch

[Tyler Wilcock]

Created attachment 467782 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
index 92625adbcc4a..e7b5c16bd2d7 100644
--- a/Source/WebCore/accessibility/AXLogger.cpp
+++ b/Source/WebCore/accessibility/AXLogger.cpp
@@ -512,6 +512,12 @@ TextStream& operator<<(TextStream& stream, AXRelationType relationType)
     case AXRelationType::OwnerFor:
         stream << "OwnerFor";
         break;
+    case AXRelationType::PopoverTarget:
+        stream << "PopoverTarget";
+        break;
+    case AXRelationType::PopoverTargetedBy:
+        stream << "PopoverTargetedBy";
+        break;

AG: what about PopoverTarget and PopoverTargetFor?

     }

     return stream;
@@ -625,6 +631,9 @@ TextStream& operator<<(TextStream& stream, AXObjectCache::AXNotification notific
     case AXObjectCache::AXNotification::AXPageScrolled:
         stream << "AXPageScrolled";
         break;
+    case AXObjectCache::AXNotification::AXPopoverTargetChanged:
+        stream << "AXPopoverTargetChanged";
+        break;
     case AXObjectCache::AXNotification::AXPositionInSetChanged:
         stream << "AXPositionInSetChanged";
         break;
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index bed612dfe2e1..75fb3b589489 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -75,6 +75,7 @@
 #include "ElementRareData.h"
 #include "FocusController.h"
 #include "HTMLAreaElement.h"
+#include "HTMLButtonElement.h"
 #include "HTMLCanvasElement.h"
 #include "HTMLDialogElement.h"
 #include "HTMLImageElement.h"
@@ -845,8 +846,10 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node* node)

     bool inCanvasSubtree = lineageOfType<HTMLCanvasElement>(*node->parentElement()).first();
     bool insideMeterElement = is<HTMLMeterElement>(*node->parentElement());
-    bool hasDisplayContents = is<Element>(*node) && downcast<Element>(*node).hasDisplayContents();
-    if (!inCanvasSubtree && !insideMeterElement && !hasDisplayContents && !isNodeAriaVisible(node))
+    auto* element = dynamicDowncast<Element>(node);

AG: smart pointer?

+    bool hasDisplayContents = element && element->hasDisplayContents();
+    bool isPopover = element && element->hasAttributeWithoutSynchronization(popoverAttr);
+    if (!inCanvasSubtree && !insideMeterElement && !hasDisplayContents && !isPopover && !isNodeAriaVisible(node))
         return nullptr;

     Ref protectedNode { *node };
@@ -1623,9 +1626,14 @@ void AXObjectCache::onFocusChange(Node* oldNode, Node* newNode)
         handleFocusedUIElementChanged(oldNode, newNode);
 }

-void AXObjectCache::onPopoverTargetToggle(const HTMLFormControlElement& popoverInvokerElement)
+void AXObjectCache::onPopoverToggle(const HTMLElement& popover)
 {
-    postNotification(get(const_cast<HTMLFormControlElement*>(&popoverInvokerElement)), &document(), AXExpandedChanged);
+    RefPtr axPopover = get(const_cast<HTMLElement*>(&popover));
+    if (!axPopover)
+        return;
+    // There may be multiple elements with popovertarget attributes that point at |popover|.
+    for (const auto& invoker : axPopover->popoverTargetedBy())
+        postNotification(dynamicDowncast<AccessibilityObject>(invoker.get()), &document(), AXExpandedChanged);
 }

 void AXObjectCache::deferMenuListValueChange(Element* element)
@@ -2436,7 +2444,8 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     } else if (attrName == colspanAttr) {
         postNotification(element, AXColumnSpanChanged);
         recomputeParentTableProperties(element, TableProperty::CellSlots);
-    }
+    } else if (attrName == popovertargetAttr)
+        postNotification(element, AXPopoverTargetChanged);

     if (!attrName.localName().string().startsWith("aria-"_s))
         return;
@@ -4125,6 +4134,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXPositionInSetChanged:
             tree->updateNodeProperties(*notification.first, { AXPropertyName::PosInSet, AXPropertyName::SupportsPosInSet });
             break;
+        case AXPopoverTargetChanged:
+            tree->updateNodeProperties(*notification.first, { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded });
+            break;
         case AXSortDirectionChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::SortDirection);
             break;
@@ -4406,6 +4418,7 @@ Vector<QualifiedName>& AXObjectCache::relationAttributes()
         aria_labeledbyAttr,
         aria_ownsAttr,
         headersAttr,
+        popovertargetAttr,
     };
     return relationAttributes;
 }
@@ -4449,6 +4462,10 @@ AXRelationType AXObjectCache::symmetricRelation(AXRelationType relationType)
         return AXRelationType::OwnerFor;
     case AXRelationType::OwnerFor:
         return AXRelationType::OwnedBy;
+    case AXRelationType::PopoverTarget:
+        return AXRelationType::PopoverTargetedBy;
+    case AXRelationType::PopoverTargetedBy:
+        return AXRelationType::PopoverTarget;
     case AXRelationType::None:
         return AXRelationType::None;
     }
@@ -4475,6 +4492,8 @@ AXRelationType AXObjectCache::attributeToRelationType(const QualifiedName& attri
         return AXRelationType::OwnerFor;
     if (attribute == headersAttr)
         return AXRelationType::Headers;
+    if (attribute == popovertargetAttr)
+        return AXRelationType::PopoverTarget;
     return AXRelationType::None;
 }

@@ -4684,6 +4703,10 @@ void AXObjectCache::updateRelations(Element& origin, const QualifiedName& attrib
         return;
     }

+    // `popovertarget` is only valid for input and button elements.
+    if (UNLIKELY(attribute == popovertargetAttr) && !is<HTMLInputElement>(origin) && !is<HTMLButtonElement>(origin))
+        return;
+
     removeRelations(origin, relationType);
     addRelations(origin, attribute);

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index ee4c3d9bac6f..7a0af9a45e92 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -195,7 +195,7 @@ public:
     void childrenChanged(RenderObject*, RenderObject* newChild = nullptr);
     void childrenChanged(AccessibilityObject*);
     void onFocusChange(Node* oldFocusedNode, Node* newFocusedNode);
-    void onPopoverTargetToggle(const HTMLFormControlElement&);
+    void onPopoverToggle(const HTMLElement&);
     void onScrollbarFrameRectChange(const Scrollbar&);
     void onSelectedChanged(Node*);
     void onTextSecurityChanged(HTMLInputElement&);
@@ -360,6 +360,7 @@ public:
         AXNewDocumentLoadComplete,
         AXPageScrolled,
         AXPlaceholderChanged,
+        AXPopoverTargetChanged,
         AXPositionInSetChanged,
         AXRoleChanged,
         AXRoleDescriptionChanged,
@@ -754,7 +755,7 @@ inline void AXObjectCache::onTextCompositionChange(Node&, CompositionState, bool
 inline void AXObjectCache::valueChanged(Element*) { }
 inline void AXObjectCache::onFocusChange(Node*, Node*) { }
 inline void AXObjectCache::onPageActivityStateChange(OptionSet<ActivityState>) { }
-inline void AXObjectCache::onPopoverTargetToggle(const HTMLFormControlElement&) { }
+inline void AXObjectCache::onPopoverToggle(const HTMLElement&) { }
 inline void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar&) { }
 inline void AXObjectCache::deferRecomputeIsIgnoredIfNeeded(Element*) { }
 inline void AXObjectCache::deferRecomputeIsIgnored(Element*) { }
diff --git a/Source/WebCore/accessibility/AccessibilityObjectInterface.h b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
index e727148f9bbb..9f1f48aee466 100644
--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -794,6 +794,8 @@ enum class AXRelationType : uint8_t {
     LabelFor,
     OwnedBy,
     OwnerFor,
+    PopoverTarget,
+    PopoverTargetedBy,

AG: can we use the ControllerFor and ControlledBy relations to represent this?

 };
 using AXRelations = HashMap<AXRelationType, Vector<AXID>, DefaultHash<uint8_t>, WTF::UnsignedWithZeroKeyHashTraits<uint8_t>>;

@@ -1047,6 +1049,7 @@ public:
     AccessibilityChildrenVector labelForObjects() const { return relatedObjects(AXRelationType::LabelFor); }
     AccessibilityChildrenVector ownedObjects() const { return relatedObjects(AXRelationType::OwnerFor); }
     AccessibilityChildrenVector owners() const { return relatedObjects(AXRelationType::OwnedBy); }
+    AccessibilityChildrenVector popoverTargetedBy() const { return relatedObjects(AXRelationType::PopoverTargetedBy); }
     virtual AccessibilityChildrenVector relatedObjects(AXRelationType) const = 0;

     bool hasPopup() const;

[Tyler Wilcock]

Created attachment 467803 [details]
Patch

[Tyler Wilcock]

> @@ -512,6 +512,12 @@ TextStream& operator<<(TextStream& stream,
> AXRelationType relationType)
>      case AXRelationType::OwnerFor:
>          stream << "OwnerFor";
>          break;
> +    case AXRelationType::PopoverTarget:
> +        stream << "PopoverTarget";
> +        break;
> +    case AXRelationType::PopoverTargetedBy:
> +        stream << "PopoverTargetedBy";
> +        break;
> 
> AG: what about PopoverTarget and PopoverTargetFor?
Yes, that's much better. Fixed.

>      bool inCanvasSubtree =
> lineageOfType<HTMLCanvasElement>(*node->parentElement()).first();
>      bool insideMeterElement = is<HTMLMeterElement>(*node->parentElement());
> -    bool hasDisplayContents = is<Element>(*node) &&
> downcast<Element>(*node).hasDisplayContents();
> -    if (!inCanvasSubtree && !insideMeterElement && !hasDisplayContents &&
> !isNodeAriaVisible(node))
> +    auto* element = dynamicDowncast<Element>(node);
> 
> AG: smart pointer?
I think this instance may be OK without. This pointer is only used to call hasDisplayContents() and hasAttributeWithoutSynchronization(), and looking at the implementations of those I don't think there is a risk of them of causing the pointer to be destroyed out from under us. But I also doubt this codepath is very hot, so should be fine to add it if you want to be extra safe.

> +    PopoverTarget,
> +    PopoverTargetedBy,
> 
> AG: can we use the ControllerFor and ControlledBy relations to represent
> this?
Hmm, I'm conflicted. I agree that those terms accurately model how the popover attributes work, so hooking into ControllerFor and ControlledBy would be simpler in some ways. But it might be unexpected that these two are conflated. For example, the one existing caller of controllers() explicitly needs to do work only for aria-controls style relationships, so returning popover controllers would be unexpected and wasteful. So for that reason I lean towards separating these relationships, but interested to hear what you think.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> > @@ -512,6 +512,12 @@ TextStream& operator<<(TextStream& stream,
> > AXRelationType relationType)
> >      case AXRelationType::OwnerFor:
> >          stream << "OwnerFor";
> >          break;
> > +    case AXRelationType::PopoverTarget:
> > +        stream << "PopoverTarget";
> > +        break;
> > +    case AXRelationType::PopoverTargetedBy:
> > +        stream << "PopoverTargetedBy";
> > +        break;
> > 
> > AG: what about PopoverTarget and PopoverTargetFor?
> Yes, that's much better. Fixed.
> 
> >      bool inCanvasSubtree =
> > lineageOfType<HTMLCanvasElement>(*node->parentElement()).first();
> >      bool insideMeterElement = is<HTMLMeterElement>(*node->parentElement());
> > -    bool hasDisplayContents = is<Element>(*node) &&
> > downcast<Element>(*node).hasDisplayContents();
> > -    if (!inCanvasSubtree && !insideMeterElement && !hasDisplayContents &&
> > !isNodeAriaVisible(node))
> > +    auto* element = dynamicDowncast<Element>(node);
> > 
> > AG: smart pointer?
> I think this instance may be OK without. This pointer is only used to call
> hasDisplayContents() and hasAttributeWithoutSynchronization(), and looking
> at the implementations of those I don't think there is a risk of them of
> causing the pointer to be destroyed out from under us. But I also doubt this
> codepath is very hot, so should be fine to add it if you want to be extra
> safe.

If I'm getting it right, unless it is a trivial function like a getter, we want a smart pointer. There is a cost associated with that, but sounds like the benefit out-weights it. Another argument is that now you know what those functions are doing, but that can change and we'll probably won't find out until too late.

> 
> > +    PopoverTarget,
> > +    PopoverTargetedBy,
> > 
> > AG: can we use the ControllerFor and ControlledBy relations to represent
> > this?
> Hmm, I'm conflicted. I agree that those terms accurately model how the
> popover attributes work, so hooking into ControllerFor and ControlledBy
> would be simpler in some ways. But it might be unexpected that these two are
> conflated. For example, the one existing caller of controllers() explicitly
> needs to do work only for aria-controls style relationships, so returning
> popover controllers would be unexpected and wasteful. So for that reason I
> lean towards separating these relationships, but interested to hear what you
> think.

I think you are referring to 

void AXObjectCache::handleTabPanelSelected(Node* oldNode, Node* newNode)
{
    auto updateTab = [this] (AccessibilityObject* controlPanel, Node& focusedNode) {
        if (!controlPanel)
            return;

        auto controllers = controlPanel->controllers();
...

But that is done only for tab panel roles. So I don't see a problem. ARIA is a source of relationships but not the only one, so I think we should use existing Relations whether they come from ARIA or any other source. That's why having an abstraction at the API level is worthwhile.

[Tyler Wilcock]

Created attachment 467815 [details]
Patch

[Tyler Wilcock]

> > > +    auto* element = dynamicDowncast<Element>(node);
> > > 
> > > AG: smart pointer?
> > I think this instance may be OK without. This pointer is only used to call
> > hasDisplayContents() and hasAttributeWithoutSynchronization(), and looking
> > at the implementations of those I don't think there is a risk of them of
> > causing the pointer to be destroyed out from under us. But I also doubt this
> > codepath is very hot, so should be fine to add it if you want to be extra
> > safe.
> 
> If I'm getting it right, unless it is a trivial function like a getter, we
> want a smart pointer. There is a cost associated with that, but sounds like
> the benefit out-weights it. Another argument is that now you know what those
> functions are doing, but that can change and we'll probably won't find out
> until too late.
Fixed this. There was already a Ref of the node, so I just moved it higher in the function to protect it for this cast to Element too.

> > > +    PopoverTarget,
> > > +    PopoverTargetedBy,
> > > 
> > > AG: can we use the ControllerFor and ControlledBy relations to represent
> > > this?
> > Hmm, I'm conflicted. I agree that those terms accurately model how the
> > popover attributes work, so hooking into ControllerFor and ControlledBy
> > would be simpler in some ways. But it might be unexpected that these two are
> > conflated. For example, the one existing caller of controllers() explicitly
> > needs to do work only for aria-controls style relationships, so returning
> > popover controllers would be unexpected and wasteful. So for that reason I
> > lean towards separating these relationships, but interested to hear what you
> > think.
> 
> I think you are referring to 
> 
> void AXObjectCache::handleTabPanelSelected(Node* oldNode, Node* newNode)
> {
>     auto updateTab = [this] (AccessibilityObject* controlPanel, Node&
> focusedNode) {
>         if (!controlPanel)
>             return;
> 
>         auto controllers = controlPanel->controllers();
> ...
> 
> But that is done only for tab panel roles. So I don't see a problem. ARIA is
> a source of relationships but not the only one, so I think we should use
> existing Relations whether they come from ARIA or any other source. That's
> why having an abstraction at the API level is worthwhile.
Fixed this. PopoverTarget and PopoverTargetOf relations are removed and we now use ControllerFor and ControllerBy.

[EWS]

Committed 268288@main (6be92f9431b6): <https://commits.webkit.org/268288@main>

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