Bug 264262 – AX: Move isMatchingPlugin off of the main thread

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

We currently serve isMatchingPlugin on the main thread. We should be caching the right values and serving this on the AX thread instead.

[Radar WebKit Bug Importer]

<rdar://problem/118001876>

[Joshua Hoffman]

rdar://116132871

[Joshua Hoffman]

Created attachment 468525 [details]
Patch

[chris fleizach]

Comment on attachment 468525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468525&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:4888
> +    if (axWidget)

if (auto* axWidget = getOrCreate(widget))

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2999
> +    if (!axObject->isWidgetPluginViewBase())

do we really need to cache this. is this something we could tie to a single property like "ValidWidget" or some appropriate name

[Joshua Hoffman]

Created attachment 468526 [details]
Patch

[Joshua Hoffman]

Comment on attachment 468525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468525&action=review

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2999
>> +    if (!axObject->isWidgetPluginViewBase())
> 
> do we really need to cache this. is this something we could tie to a single property like "ValidWidget" or some appropriate name

Yeah that would be cleaner, I can look at refactoring that.

[Tyler Wilcock]

View in context: https://bugs.webkit.org/attachment.cgi?id=468525&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:498
> +    bool isWidgetVisible() const final { return  boolAttributeValue(AXPropertyName::IsWidgetVisible); }
> +    bool isWidgetPluginViewBase() const final { return  boolAttributeValue(AXPropertyName::IsWidgetPluginViewBase); }

Looks like an extra space between `return` and `boolAttributeValue` on these two lines?

> LayoutTests/accessibility/mac/widget-visibility.html:32
> +        // Ensure explicitely hiding/showing widgets affects searching for "visible only".

I think explicitely is a misspelling of explicitly?

> LayoutTests/accessibility/mac/widget-visibility.html:33
> +		domPdfEmbedElement.style.visibility = "visible";

Extra tab / spaces here?

> LayoutTests/accessibility/mac/widget-visibility.html:41
> +		domPdfEmbedElement.style.visibility = "hidden";
> +		await sleep(2000);

Extra tab / spaces here?

I get that you've added this sleep to allow the bug to occur if our implementation behaves wrong. But is it enough to just have the `await waitFor` block below wait until the search returns nothing? Would that still catch the bug if our implementation becomes wrong?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #7)
> View in context:
> 
> I think explicitely is a misspelling of explicitly?

Yes, thanks for catching that!

> I get that you've added this sleep to allow the bug to occur if our
> implementation behaves wrong. But is it enough to just have the `await
> waitFor` block below wait until the search returns nothing? Would that still
> catch the bug if our implementation becomes wrong?

The latest patch I uploaded above removed that—it was an artifact from debugging. So sorry!

[Joshua Hoffman]

Comment on attachment 468525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468525&action=review

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2999
>>> +    if (!axObject->isWidgetPluginViewBase())
>> 
>> do we really need to cache this. is this something we could tie to a single property like "ValidWidget" or some appropriate name
> 
> Yeah that would be cleaner, I can look at refactoring that.

Upon discussion with Tyler who wrote isMatchingPlugin initially, it is important that we return false here for widgets that are not plugins (Scrollbars, etc). So we cannot combine the two properties here.

[Joshua Hoffman]

Created attachment 468529 [details]
Patch

[Joshua Hoffman]

Created attachment 468535 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468535&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:4917
> +    if (auto* axWidget = getOrCreate(widget))

Is it necessary for this to be `getOrCreate`? Or can it just be `get`?

> Source/WebCore/accessibility/AccessibilityRenderObject.h:109
> +    bool isPlugin() const final { return widget() && is<PluginViewBase>(widget()); }

This can just be is<PluginViewBase>(widget()). is<T> inherently does a null check.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:325
>          setProperty(AXPropertyName::IsWidget, true);
> +        setProperty(AXPropertyName::IsPlugin, object.isPlugin());

We could probably avoid storing one property in the map if we took advantage of the fact that "is widget" inherently includes "is plugin". Concretely:

if (object.isWidget()) {
    // Plugins are a type of widget, so we only need to incur the memory cost of storing one of these. AXIsolatedObject::isWidget checks for both.
    if (object.isPlugin())
        setProperty(AXPropertyName::IsPlugin, true)
    else
         setProperty(AXPropertyName::IsWidget, true)

    setProperty(AXPropertyName::IsWidgetVisible, object.isWidgetVisible());
}

And then:

// Plugins are a type of widget.
bool isWidget() const final { return boolAttributeValue(AXPropertyName::IsWidget) || boolAttributeValue(AXPropertyName::IsPlugin); }

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3000
> +    if (!axObject->isPlugin())
> +        return false;

I think with this, we can remove the isWidget() check above? Does that sound correct?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #12)
> Comment on attachment 468535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468535&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4917
> > +    if (auto* axWidget = getOrCreate(widget))
> 
> Is it necessary for this to be `getOrCreate`? Or can it just be `get`?

We should be able to use get—I will update that.

[Joshua Hoffman]

Created attachment 468578 [details]
Patch

[Tyler Wilcock]

> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3000
> > +    if (!axObject->isPlugin())
> > +        return false;
> 
> I think with this, we can remove the isWidget() check above? Does that sound
> correct?
Were you able to check this idea out? Or do we need to keep the isWidget() check?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #15)
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3000
> > > +    if (!axObject->isPlugin())
> > > +        return false;
> > 
> > I think with this, we can remove the isWidget() check above? Does that sound
> > correct?
> Were you able to check this idea out? Or do we need to keep the isWidget()
> check?

Yes we can do this—I read this comment as related to the attribute one so forgot to include it, but will add it back!

[Joshua Hoffman]

Created attachment 468580 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #17)
> Created attachment 468580 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index 5360bd0d8a9a..2ed0fbf56380 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -1156,6 +1156,8 @@ public:
     virtual Widget* widget() const = 0;
     virtual PlatformWidget platformWidget() const = 0;
     virtual Widget* widgetForAttachmentView() const = 0;
+    virtual bool isWidgetVisible() const = 0;

AG: we already have in the AXCoreObject interface isWidget() and isVisible(). Why do we need to add isWidgetVisible? Can't it be determined from the exissting properties?

+    virtual bool isPlugin() const = 0;

     // FIXME: Remove the following methods from the AXCoreObject interface and instead use methods such as axScrollView() if needed.
     virtual Page* page() const = 0;

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #18)
> (In reply to Joshua Hoffman from comment #17)
> > Created attachment 468580 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/AXCoreObject.h
> b/Source/WebCore/accessibility/AXCoreObject.h
> index 5360bd0d8a9a..2ed0fbf56380 100644
> --- a/Source/WebCore/accessibility/AXCoreObject.h
> +++ b/Source/WebCore/accessibility/AXCoreObject.h
> @@ -1156,6 +1156,8 @@ public:
>      virtual Widget* widget() const = 0;
>      virtual PlatformWidget platformWidget() const = 0;
>      virtual Widget* widgetForAttachmentView() const = 0;
> +    virtual bool isWidgetVisible() const = 0;
> 
> AG: we already have in the AXCoreObject interface isWidget() and
> isVisible(). Why do we need to add isWidgetVisible? Can't it be determined
> from the exissting properties?

Widget::isVisible is separate from isVisible, as Widget itself is also not a Node/Element. Widget::isVisible uses the widget's m_selfVisible and m_parentVisible to determine visibility.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #19)
> (In reply to Andres Gonzalez from comment #18)
> > (In reply to Joshua Hoffman from comment #17)
> > > Created attachment 468580 [details]
> > > Patch
> > 
> > diff --git a/Source/WebCore/accessibility/AXCoreObject.h
> > b/Source/WebCore/accessibility/AXCoreObject.h
> > index 5360bd0d8a9a..2ed0fbf56380 100644
> > --- a/Source/WebCore/accessibility/AXCoreObject.h
> > +++ b/Source/WebCore/accessibility/AXCoreObject.h
> > @@ -1156,6 +1156,8 @@ public:
> >      virtual Widget* widget() const = 0;
> >      virtual PlatformWidget platformWidget() const = 0;
> >      virtual Widget* widgetForAttachmentView() const = 0;
> > +    virtual bool isWidgetVisible() const = 0;
> > 
> > AG: we already have in the AXCoreObject interface isWidget() and
> > isVisible(). Why do we need to add isWidgetVisible? Can't it be determined
> > from the exissting properties?
> 
> Widget::isVisible is separate from isVisible, as Widget itself is also not a
> Node/Element. Widget::isVisible uses the widget's m_selfVisible and
> m_parentVisible to determine visibility.

AG: I think AX cares only about the resulting visibility, not whether the embed object is visible separately from the widget or vice versa.

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #20)
> (In reply to Joshua Hoffman from comment #19)
> > (In reply to Andres Gonzalez from comment #18)
> > > (In reply to Joshua Hoffman from comment #17)
> > > > Created attachment 468580 [details]
> > > > Patch
> > > 
> > > 
> > > AG: we already have in the AXCoreObject interface isWidget() and
> > > isVisible(). Why do we need to add isWidgetVisible? Can't it be determined
> > > from the exissting properties?
> > 
> > Widget::isVisible is separate from isVisible, as Widget itself is also not a
> > Node/Element. Widget::isVisible uses the widget's m_selfVisible and
> > m_parentVisible to determine visibility.
> 
> AG: I think AX cares only about the resulting visibility, not whether the
> embed object is visible separately from the widget or vice versa.

Interesting— in the code we are replacing, we check the widget's visibility explicitly:

downcast<PluginViewBase>(widget)->isVisible()

So my instinct would be that this would create a behavior change. But it might be that Widget::isVisible will always be the same state as the embed object's. I can confirm this and make this change if so.

[Joshua Hoffman]

Created attachment 468594 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #22)
> Created attachment 468594 [details]
> Patch

Looking good. A few minor details:

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 23babdad974a..1e0e66564ace 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -59,6 +59,7 @@ class Page;
 class RenderBlock;
 class RenderObject;
 class RenderText;
+class RenderWidget;
 class Scrollbar;
 class ScrollView;
 class VisiblePosition;
@@ -203,6 +204,7 @@ public:
     void onTitleChange(Document&);
     void onValidityChange(Element&);
     void onTextCompositionChange(Node&, CompositionState, bool, const String&, size_t, bool);
+    void onWidgetVisibilityChanged(RenderWidget*);
     void valueChanged(Element*);
     void checkedStateChanged(Node*);
     void autofillTypeChanged(Node*);
@@ -386,6 +388,7 @@ public:
         AXTextCompositionEnded,
         AXURLChanged,
         AXValueChanged,
+        AXWidgetVisibilityChanged,

AG: shouldn't we call this just AXVisibilityChanged? It is now used for Widgets, but it could be use for Nodes or RenderObjects. It will better match the AXPropertyName.

         AXScrolledToAnchor,
         AXLabelCreated,
         AXLiveRegionCreated,
@@ -793,6 +796,7 @@ inline void AXObjectCache::onFocusChange(Node*, Node*) { }
 inline void AXObjectCache::onPageActivityStateChange(OptionSet<ActivityState>) { }
 inline void AXObjectCache::onPopoverToggle(const HTMLElement&) { }
 inline void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar&) { }
+inline void AXObjectCache::onWidgetVisibilityChanged(Widget&) { }
 inline void AXObjectCache::deferRecomputeIsIgnoredIfNeeded(Element*) { }
 inline void AXObjectCache::deferRecomputeIsIgnored(Element*) { }
 inline void AXObjectCache::deferTextChangedIfNeeded(Node*) { }
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index 05948a47b6c6..2c2df7a91686 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -218,7 +218,7 @@ public:
     bool isRequired() const override { return false; }
     bool supportsRequiredAttribute() const override { return false; }
     bool isExpanded() const override;
-    bool isVisible() const override { return true; }
+    bool isVisible() const override { return !isHidden(); }
     virtual bool isCollapsed() const { return false; }
     void setIsExpanded(bool) override { }
     FloatRect unobscuredContentRect() const override;
@@ -460,6 +460,7 @@ public:
     Widget* widget() const override { return nullptr; }
     PlatformWidget platformWidget() const override { return nullptr; }
     Widget* widgetForAttachmentView() const override { return nullptr; }
+    bool isPlugin() const override { return false; }

 #if PLATFORM(COCOA)
     RemoteAXObjectRef remoteParentObject() const override;
diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.h b/Source/WebCore/accessibility/AccessibilityRenderObject.h
index b5ee2c5ec236..77045c052fc9 100644
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.h
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.h
@@ -30,6 +30,7 @@

 #include "AccessibilityNodeObject.h"
 #include "LayoutRect.h"
+#include "PluginViewBase.h"
 #include "RenderObject.h"
 #include <wtf/Forward.h>
 #include <wtf/WeakPtr.h>
@@ -104,6 +105,7 @@ public:
     Widget* widgetForAttachmentView() const override;
     AccessibilityChildrenVector documentLinks() override;
     LocalFrameView* documentFrameView() const override;
+    bool isPlugin() const final { return is<PluginViewBase>(widget()); }

     void setSelectedTextRange(CharacterRange&&) override;
     bool setValue(const String&) override;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 8f9c2eea8e01..82f86406e449 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -320,8 +320,11 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
         }
     }

-    if (object.isWidget())
+    if (object.isWidget()) {
         setProperty(AXPropertyName::IsWidget, true);
+        setProperty(AXPropertyName::IsPlugin, object.isPlugin());
+        setProperty(AXPropertyName::IsVisible, object.isVisible());
+    }

     auto descriptor = object.title();
     if (descriptor.length())
@@ -341,11 +344,14 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     // These properties are only needed on the AXCoreObject interface due to their use in ATSPI,
     // so only cache them for ATSPI.
 #if PLATFORM(ATSPI)
+    // We cache IsVisible on all platforms just for Widgets above. In ATSPI, this should be cached on all objects.
+    if (!object.isWidget())
+        setProperty(AXPropertyName::IsVisible, object.isVisible());
+
     setProperty(AXPropertyName::ActionVerb, object.actionVerb().isolatedCopy());
     setProperty(AXPropertyName::IsFieldset, object.isFieldset());
     setProperty(AXPropertyName::IsPressed, object.isPressed());
     setProperty(AXPropertyName::IsSelectedOptionActive, object.isSelectedOptionActive());
-    setProperty(AXPropertyName::IsVisible, object.isVisible());
     setProperty(AXPropertyName::LocalizedActionVerb, object.localizedActionVerb().isolatedCopy());
 #endif

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 192f347b8b27..c24f843d953b 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -447,6 +447,7 @@ private:
     bool isOnScreen() const final;
     bool isOffScreen() const final;
     bool isPressed() const final;
+    // FIXME: isVisible should be accurate for all objects, not just widgets, on COCOA.
     bool isVisible() const final { return boolAttributeValue(AXPropertyName::IsVisible); }
     bool isSelectedOptionActive() const final;
     bool hasBoldFont() const final { return boolAttributeValue(AXPropertyName::HasBoldFont); }
@@ -494,6 +495,7 @@ private:
     Widget* widget() const final;
     PlatformWidget platformWidget() const final;
     Widget* widgetForAttachmentView() const final;
+    bool isPlugin() const final { return boolAttributeValue(AXPropertyName::IsPlugin); }

     HashMap<String, AXEditingStyleValueVariant> resolvedEditingStyles() const final;
 #if PLATFORM(COCOA)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 7d52c3c0c2bc..7ffb3a98f016 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -616,6 +616,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::IsRowHeader:
             propertyMap.set(AXPropertyName::IsRowHeader, axObject.isRowHeader());
             break;
+        case AXPropertyName::IsVisible:
+            propertyMap.set(AXPropertyName::IsVisible, axObject.isVisible());
+            break;
         case AXPropertyName::MaxValueForRange:
             propertyMap.set(AXPropertyName::MaxValueForRange, axObject.maxValueForRange());
             break;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index ea9feb9317ab..4459f20496c3 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -146,6 +146,7 @@ enum class AXPropertyName : uint16_t {
     IsMathMultiscript,
     IsMathToken,
     IsMultiSelectable,
+    IsPlugin,
     IsPressed,
     IsRequired,
     IsRowHeader,
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index cd3890a50a68..a789cdc0d2cc 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -2993,17 +2993,11 @@ - (AXTextMarkerRef)textMarkerForTextMarker:(AXTextMarkerRef)textMarkerRef atUnit

 static bool isMatchingPlugin(AXCoreObject* axObject, const AccessibilitySearchCriteria& criteria)
 {
-    if (!axObject->isWidget())
+    if (!axObject->isPlugin())
         return false;

-    return Accessibility::retrieveValueFromMainThread<bool>([&axObject, &criteria] () -> bool {
-        auto* widget = axObject->widget();
-        if (!is<PluginViewBase>(widget))
-            return false;
-
-        return criteria.searchKeys.contains(AccessibilitySearchKey::AnyType)
-            && (!criteria.visibleOnly || downcast<PluginViewBase>(widget)->isVisible());
-    });
+    return criteria.searchKeys.contains(AccessibilitySearchKey::AnyType)
+        && (!criteria.visibleOnly || axObject->isVisible());
 }

AG: while you're at it, can we change the signature to take a AXCoreObject& or const& since the function assumes it is a not-null object?

 ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN
diff --git a/Source/WebCore/rendering/RenderWidget.cpp b/Source/WebCore/rendering/RenderWidget.cpp
index cbd3eb86b01f..733dff2d6986 100644
--- a/Source/WebCore/rendering/RenderWidget.cpp
+++ b/Source/WebCore/rendering/RenderWidget.cpp
@@ -201,6 +201,8 @@ void RenderWidget::setWidget(RefPtr<Widget>&& widget)
                 m_widget->show();
                 repaint();
             }
+            if (auto* cache = document().existingAXObjectCache())
+                cache->onWidgetVisibilityChanged(this);
         }
         moveWidgetToParentSoon(*m_widget, &view().frameView());
     }
@@ -225,6 +227,9 @@ void RenderWidget::styleDidChange(StyleDifference diff, const RenderStyle* oldSt
             m_widget->hide();
         else
             m_widget->show();
+
+        if (auto* cache = document().existingAXObjectCache())
+            cache->onWidgetVisibilityChanged(this);
     }
 }

diff --git a/LayoutTests/accessibility/mac/widget-visibility-expected.txt b/LayoutTests/accessibility/mac/widget-visibility-expected.txt
new file mode 100644
index 000000000000..4362b93af7d9
--- /dev/null
+++ b/LayoutTests/accessibility/mac/widget-visibility-expected.txt
@@ -0,0 +1,9 @@
+This test ensures that widget visibility status is correctly reported from accessibility via searching.
+
+PASS: searchResultElement.stringAttributeValue('AXSubrole') === 'AXPDFPluginSubrole'
+PASS: searchResultElement === null
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/mac/widget-visibility.html b/LayoutTests/accessibility/mac/widget-visibility.html
new file mode 100644
index 000000000000..e3d84bd4b647
--- /dev/null
+++ b/LayoutTests/accessibility/mac/widget-visibility.html
@@ -0,0 +1,54 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test.js"></script>
+<script src="../../resources/accessibility-helper.js"></script>
+</head>
+<body>
+
+<div id="embedContainer">
+    <embed id="pdfEmbed" width="75%" height="75%" name="plugin" src="../resources/simple-webkit-pages.pdf" type="application/pdf">
+</div>
+
+<script>
+let output = "This test ensures that widget visibility status is correctly reported from accessibility via searching.\n\n";
+
+if (window.accessibilityController) {
+    window.jsTestIsAsync = true;
+
+    let pdfAxObject, pdfEmbedElement;
+    setTimeout(async () => {
+        await waitFor(() => {
+            embedContainer = accessibilityController.accessibleElementById("embedContainer");
+            return embedContainer && embedContainer.childrenCount >= 1;

AG: don't need >= 1.

+        });
+        await waitFor(() => {
+            pdfEmbedElement = accessibilityController.accessibleElementById("pdfEmbed");
+            return pdfEmbedElement && pdfEmbedElement.children.length >= 1;

AG: Can't we merge these two waitFor into one? use either childrenCount or children.length? and no need >= 1.

+        });
+
+        var domPdfEmbedElement = document.getElementById("pdfEmbed");
+
+        // Ensure explicitly hiding/showing widgets affects searching for "visible only".
+        domPdfEmbedElement.style.visibility = "visible";
+        await waitFor(() => {
+            searchResultElement = pdfEmbedElement.uiElementForSearchPredicate(null, true, "AXAnyTypeSearchKey", "", true);
+            return searchResultElement;
+        });
+        output += expect("searchResultElement.stringAttributeValue('AXSubrole')", "'AXPDFPluginSubrole'");
+
+        domPdfEmbedElement.style.visibility = "hidden";
+        await waitFor(() => {
+            searchResultElement = pdfEmbedElement.uiElementForSearchPredicate(null, true, "AXAnyTypeSearchKey", "", true);
+            return !searchResultElement;
+        });
+        output += expect("searchResultElement", "null");
+
+        debug(output);
+        finishJSTest();
+    });
+}
+</script>
+</body>
+</html>
+

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #23)
> (In reply to Joshua Hoffman from comment #22)
> > Created attachment 468594 [details]
> > Patch
> AG: shouldn't we call this just AXVisibilityChanged? It is now used for
> Widgets, but it could be use for Nodes or RenderObjects. It will better
> match the AXPropertyName.

Good call—that will set us up well for future isVisible work too. I'll rename that. 

>  static bool isMatchingPlugin(AXCoreObject* axObject, const
> AccessibilitySearchCriteria& criteria)
>  {
> -    if (!axObject->isWidget())
> +    if (!axObject->isPlugin())
>          return false;
> 
> -    return Accessibility::retrieveValueFromMainThread<bool>([&axObject,
> &criteria] () -> bool {
> -        auto* widget = axObject->widget();
> -        if (!is<PluginViewBase>(widget))
> -            return false;
> -
> -        return criteria.searchKeys.contains(AccessibilitySearchKey::AnyType)
> -            && (!criteria.visibleOnly ||
> downcast<PluginViewBase>(widget)->isVisible());
> -    });
> +    return criteria.searchKeys.contains(AccessibilitySearchKey::AnyType)
> +        && (!criteria.visibleOnly || axObject->isVisible());
>  }
> 
> AG: while you're at it, can we change the signature to take a AXCoreObject&
> or const& since the function assumes it is a not-null object?

Yes!

> 
> +        });
> +        await waitFor(() => {
> +            pdfEmbedElement =
> accessibilityController.accessibleElementById("pdfEmbed");
> +            return pdfEmbedElement && pdfEmbedElement.children.length >= 1;
> 
> AG: Can't we merge these two waitFor into one? use either childrenCount or
> children.length? and no need >= 1.

Yeah, I will merge them.

[Joshua Hoffman]

Created attachment 468603 [details]
Patch

[EWS]

Committed 270785@main (d084dfdc80b6): <https://commits.webkit.org/270785@main>

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