Bug 236750

<rdar://problem/89066221>

Created attachment 452311 [details]
Patch

patch looks good. is there any way to test this?

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

--- a/Source/WebCore/accessibility/AccessibilityScrollView.cpp
+++ a/Source/WebCore/accessibility/AccessibilityScrollView.cpp

+AXObjectCache* AccessibilityScrollView::currentAxObjectCache() const
+{
+    if (auto* cache = axObjectCache())
+        return cache;
+
+    // It's possible that the document associated with `this` was destroyed (and thus axObjectCache() will return null),
+    // but also that `this` still represents a valid frame owner (e.g. iframe). Return the cache of the iframe in this case.
+    return m_frameOwnerElement ? m_frameOwnerElement->document().axObjectCache() : nullptr;
+}

Can we just override AXCoreObject::axObjectCache instead of adding a new method? 

The comment here only makes sense for the reader who knows the implementation of AccessibilityObject::axObjectCache. this could be written more succinctly and clearer like:

    auto* document = this->document();
    return document ? document->axObjectCache() :
        m_frameOwnerElement ? m_frameOwnerElement->document().axObjectCache() : nullptr;

Or perhaps better yet to override document(), and then you wouldn't need to override axObjectCache().

+Document* AccessibilityScrollView::contentDocument() const
+{
+    if (m_frameOwnerElement)
+        return m_frameOwnerElement->contentDocument();
+    return nullptr;
+}

do we need this method separate from document()? Or we can just override document()?

If we need this, it could be written in two lines with the ? :, which I think it is better.

+FrameView* AccessibilityScrollView::contentDocumentFrameView() const
+{
+    if (auto* document = contentDocument())
+        return document->view();
+    return nullptr;
+}

Can we override documentFrameView() instead?

@@ -157,7 +194,7 @@ AccessibilityScrollbar* AccessibilityScrollView::addChildScrollbar(Scrollbar* sc
     if (!scrollbar)
         return nullptr;

-    AXObjectCache* cache = axObjectCache();
+    AXObjectCache* cache = currentAxObjectCache();

No need for this if you override axObjectCache().


 AccessibilityObject* AccessibilityScrollView::webAreaObject() const
 {
-    if (!is<FrameView>(m_scrollView))
-        return nullptr;
+    auto* document = contentDocument();
+    if (is<FrameView>(m_scrollView))
+        document = downcast<FrameView>(*m_scrollView).frame().document();

-    Document* document = downcast<FrameView>(*m_scrollView).frame().document();
     if (!document || !document->hasLivingRenderTree())
         return nullptr;

-    if (AXObjectCache* cache = axObjectCache())
+    if (auto* cache = currentAxObjectCache())
         return cache->getOrCreate(document);
I think this can be simplified with the right overrides.

 AccessibilityObject* AccessibilityScrollView::parentObject() const
 {
-    if (!is<FrameView>(m_scrollView))
+    if (!is<FrameView>(m_scrollView)) {
+        if (m_frameOwnerElement) {
+            if (auto* cache = m_frameOwnerElement->document().axObjectCache())
+                return cache->getOrCreate(m_frameOwnerElement.get());
+        }
         return nullptr;
+    }

     AXObjectCache* cache = axObjectCache();
     if (!cache)

Can this be simplified with the right overrides? i.e., no need to get the cache in two different places.

 ScrollableArea* AccessibilityScrollView::getScrollableAreaIfScrollable() const
 {
-    return m_scrollView.get();
+    return currentScrollView();
 }

Can we inline this in the header like scrollView() for consistency?

--- a/Source/WebCore/accessibility/AccessibilityScrollView.h
+++ a/Source/WebCore/accessibility/AccessibilityScrollView.h

+    // Returns the document for m_frameOwnerElement's current `contentFrame` (if present).
+    Document* contentDocument() const;
+    FrameView* contentDocumentFrameView() const;
...
+    AXObjectCache* currentAxObjectCache() const;

Do we need these, or we can just override document() and documentFrameView()?

Created attachment 452400 [details]
Patch

Fixed all of your comments. Still trying to get a minimal test case reproducing the issue.

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

 bool AccessibilityScrollView::isAttachment() const
 {
-    return m_scrollView && m_scrollView->platformWidget();
+    if (auto* scrollView = currentScrollView())
+        return scrollView->platformWidget();
+    return false;

I personally prefer the two line version:

auto* scrollView = currentScrollView();
return scrollView ? scrollView->platformWidget() : false;

But not big deal.

 PlatformWidget AccessibilityScrollView::platformWidget() const
 {
-    return m_scrollView ? m_scrollView->platformWidget() : nullptr;
+    if (auto* scrollView = currentScrollView())
+        return scrollView->platformWidget();
+    return nullptr;
 }

Same here:

auto* scrollView = currentScrollView();
return scrollView ? scrollView->platformWidget() : nullptr;

Created attachment 452465 [details]
Minimal reproduction of issue

Committed r290130 (247474@main): <https://commits.webkit.org/247474@main>

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