Attachment 450117 Details for Bug 235700 – Patch

[patch] Patch

wcore-atspi-children-changed.diff (text/plain), 14.97 KB, created by Carlos Garcia Campos on 2022-01-27 02:06:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Carlos Garcia Campos

Created: 2022-01-27 02:06:36 PST

Size: 14.97 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8277505593d9..367d7c0e7622 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,45 @@
>+2022-01-27  Carlos Garcia Campos  <cgarcia@igalia.com>
>+
>+        [GTK][a11y] Do not set the children count when serializing objects for the ATSPI cache
>+        https://bugs.webkit.org/show_bug.cgi?id=235700
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        It's confusing the cache if things doesn't happen in the expected order. We handle the children by emitting the
>+        children-changed signals. When the children count is set in the cache by AddAccessible signal or as a response
>+        to GetItems, atspi sets the accessible children vector size to the given value. If children-changed:add is
>+        emitted after that, it's considered a new child and the children vector grows. So, we end up with the double of
>+        children, but half of them are just null.
>+
>+        * accessibility/atspi/AccessibilityAtspi.cpp:
>+        (WebCore::AccessibilityAtspi::registerObject): Add the registered object to the update cache list.
>+        (WebCore::AccessibilityAtspi::unregisterObject): Remove the object from the cache and emit RemoveAccessible
>+        signal if needed.
>+        (WebCore::AccessibilityAtspi::parentChanged): Stop registering the objects here, they will be registered when
>+        actually needed. Also don't emit the signal if the object is pending, because AddAccessible will update the cache.
>+        (WebCore::AccessibilityAtspi::childrenChanged): Do not add the pending objects to the cache, we can just emit
>+        the signal now and cache will be updated later.
>+        (WebCore::AccessibilityAtspi::stateChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::textChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::textAttributesChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::textCaretMoved): Ditto.
>+        (WebCore::AccessibilityAtspi::textSelectionChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::valueChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::selectionChanged): Ditto.
>+        (WebCore::AccessibilityAtspi::loadEvent): Ditto.
>+        (WebCore::AccessibilityAtspi::cacheUpdateTimerFired): Copy the list because addToCacheIfNeeded() might add new
>+        items to the that we don't want to process in this iteration.
>+        (WebCore::AccessibilityAtspi::addToCacheIfPending): Deleted.
>+        (WebCore::AccessibilityAtspi::addAccessible): Deleted.
>+        (WebCore::AccessibilityAtspi::removeAccessible): Deleted.
>+        * accessibility/atspi/AccessibilityAtspi.h:
>+        * accessibility/atspi/AccessibilityObjectAtspi.cpp:
>+        (WebCore::AccessibilityObjectAtspi::registerObject): Remove the call to addAccessible, it's now done in AccessibilityAtspi::registerObject.
>+        (WebCore::AccessibilityObjectAtspi::serialize const): Pass -1 as children count to ensure the children vector is
>+        not modified.
>+        * accessibility/atspi/AccessibilityRootAtspi.cpp:
>+        (WebCore::AccessibilityRootAtspi::serialize const): Ditto.
>+
> 2022-01-27  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [GTK][a11y] Keep a reference to the parent wrapper with ATSPI
>diff --git a/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp b/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp
>index 7e14a746a938..95872f1015a8 100644
>--- a/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp
>+++ b/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp
>@@ -329,6 +329,10 @@ String AccessibilityAtspi::registerObject(AccessibilityObjectAtspi& atspiObject,
>     }
>     m_atspiObjects.add(&atspiObject, WTFMove(registeredObjects));
> 
>+    m_cacheUpdateList.add(&atspiObject);
>+    if (!m_cacheUpdateTimer.isActive())
>+        m_cacheUpdateTimer.startOneShot(0_s);
>+
>     return path;
> }
> 
>@@ -344,10 +348,17 @@ void AccessibilityAtspi::unregisterObject(AccessibilityObjectAtspi& atspiObject)
>             g_dbus_connection_unregister_object(m_connection.get(), id);
>     }
> 
>-    g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "StateChanged",
>+    const auto& path = atspiObject.path();
>+    g_dbus_connection_emit_signal(m_connection.get(), nullptr, path.utf8().data(), "org.a11y.atspi.Event.Object", "StateChanged",
>         g_variant_new("(siiva{sv})", "defunct", TRUE, 0, g_variant_new_string("0"), nullptr), nullptr);
> 
>-    removeAccessible(atspiObject);
>+    if (!m_cacheUpdateList.remove(&atspiObject) && m_cache.remove(path)) {
>+        g_dbus_connection_emit_signal(m_connection.get(), nullptr, "/org/a11y/atspi/cache", "org.a11y.atspi.Cache", "RemoveAccessible",
>+            g_variant_new("((so))", uniqueName(), path.utf8().data()), nullptr);
>+    }
>+
>+    if (m_cacheUpdateList.isEmpty())
>+        m_cacheUpdateTimer.stop();
> 
>     auto registeredObjects = m_atspiObjects.take(&atspiObject);
>     for (auto id : registeredObjects)
>@@ -380,13 +391,10 @@ void AccessibilityAtspi::parentChanged(AccessibilityObjectAtspi& atspiObject)
>     if (!m_clients.isEmpty())
>         return;
> 
>-    // Emit parentChanged only if the object is already registered, otherwise register the object,
>-    // without emitting the signal, because org.a11y.atspi.Cache.AddAccessible() will update the cache.
>-    if (atspiObject.registerObject())
>+    // Do not emit parent-changed for pending objects, since AddAccessible will update the cache.
>+    if (m_cacheUpdateList.contains(&atspiObject))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "PropertyChange",
>         g_variant_new("(siiva{sv})", "accessible-parent", 0, 0, atspiObject.parentReference(), nullptr), nullptr);
> }
>@@ -417,8 +425,6 @@ void AccessibilityAtspi::childrenChanged(AccessibilityObjectAtspi& atspiObject,
>     if (m_clients.isEmpty())
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-    addToCacheIfPending(child);
> 
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "ChildrenChanged",
>         g_variant_new("(siiv(so))", change == ChildrenChanged::Added ? "add" : "remove", child.indexInParentForChildrenChanged(change),
>@@ -434,7 +440,6 @@ void AccessibilityAtspi::childrenChanged(AccessibilityRootAtspi& rootObject, Acc
>     if (m_clients.isEmpty())
>         return;
> 
>-    addToCacheIfPending(child);
> 
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, rootObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "ChildrenChanged",
>         g_variant_new("(siiv(so))", change == ChildrenChanged::Added ? "add" : "remove", 0,
>@@ -453,8 +458,6 @@ void AccessibilityAtspi::stateChanged(AccessibilityObjectAtspi& atspiObject, con
>     if (!shouldEmitSignal("Object", "StateChanged", name))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "StateChanged",
>         g_variant_new("(siiva{sv})", name, value, 0, g_variant_new_string("0"), nullptr), nullptr);
> }
>@@ -471,8 +474,6 @@ void AccessibilityAtspi::textChanged(AccessibilityObjectAtspi& atspiObject, cons
>     if (!shouldEmitSignal("Object", "TextChanged", changeType))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "TextChanged",
>         g_variant_new("(siiva{sv})", changeType, offset, length, g_variant_new_string(text.data()), nullptr), nullptr);
> }
>@@ -485,8 +486,6 @@ void AccessibilityAtspi::textAttributesChanged(AccessibilityObjectAtspi& atspiOb
>     if (!shouldEmitSignal("Object", "TextAttributesChanged"))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "TextAttributesChanged",
>         g_variant_new("(siiva{sv})", "", 0, 0, g_variant_new_string(""), nullptr), nullptr);
> }
>@@ -503,8 +502,6 @@ void AccessibilityAtspi::textCaretMoved(AccessibilityObjectAtspi& atspiObject, u
>     if (!shouldEmitSignal("Object", "TextCaretMoved"))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "TextCaretMoved",
>         g_variant_new("(siiva{sv})", "", caretOffset, 0, g_variant_new_string(""), nullptr), nullptr);
> }
>@@ -517,8 +514,6 @@ void AccessibilityAtspi::textSelectionChanged(AccessibilityObjectAtspi& atspiObj
>     if (!shouldEmitSignal("Object", "TextSelectionChanged"))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "TextSelectionChanged",
>         g_variant_new("(siiva{sv})", "", 0, 0, g_variant_new_string(""), nullptr), nullptr);
> }
>@@ -535,8 +530,6 @@ void AccessibilityAtspi::valueChanged(AccessibilityObjectAtspi& atspiObject, dou
>     if (!shouldEmitSignal("Object", "PropertyChange", "accessible-value"))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "PropertyChange",
>         g_variant_new("(siiva{sv})", "accessible-value", 0, 0, g_variant_new_double(value), nullptr), nullptr);
> }
>@@ -553,8 +546,6 @@ void AccessibilityAtspi::selectionChanged(AccessibilityObjectAtspi& atspiObject)
>     if (!shouldEmitSignal("Object", "SelectionChanged"))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Object", "SelectionChanged",
>         g_variant_new("(siiva{sv})", "", 0, 0, g_variant_new_string(""), nullptr), nullptr);
> }
>@@ -571,8 +562,6 @@ void AccessibilityAtspi::loadEvent(AccessibilityObjectAtspi& atspiObject, CStrin
>     if (!shouldEmitSignal("Document", event.data()))
>         return;
> 
>-    addToCacheIfPending(atspiObject);
>-
>     g_dbus_connection_emit_signal(m_connection.get(), nullptr, atspiObject.path().utf8().data(), "org.a11y.atspi.Event.Document", event.data(),
>         g_variant_new("(siiva{sv})", "", 0, 0, g_variant_new_string(""), nullptr), nullptr);
> }
>@@ -785,43 +774,11 @@ void AccessibilityAtspi::addToCacheIfNeeded(AccessibilityObjectAtspi& atspiObjec
>         g_variant_new("(@(" ITEM_SIGNATURE "))", g_variant_builder_end(&builder)), nullptr);
> }
> 
>-void AccessibilityAtspi::addToCacheIfPending(AccessibilityObjectAtspi& atspiObject)
>-{
>-    if (!m_cacheUpdateList.remove(&atspiObject))
>-        return;
>-
>-    addToCacheIfNeeded(atspiObject);
>-    if (m_cacheUpdateList.isEmpty())
>-        m_cacheUpdateTimer.stop();
>-}
>-
> void AccessibilityAtspi::cacheUpdateTimerFired()
> {
>-    while (!m_cacheUpdateList.isEmpty())
>-        addToCacheIfNeeded(*m_cacheUpdateList.takeFirst());
>-}
>-
>-void AccessibilityAtspi::addAccessible(AccessibilityObjectAtspi& atspiObject)
>-{
>-    if (!m_connection)
>-        return;
>-
>-    m_cacheUpdateList.add(&atspiObject);
>-    if (!m_cacheUpdateTimer.isActive())
>-        m_cacheUpdateTimer.startOneShot(0_s);
>-}
>-
>-void AccessibilityAtspi::removeAccessible(AccessibilityObjectAtspi& atspiObject)
>-{
>-    if (m_cacheUpdateList.remove(&atspiObject))
>-        return;
>-
>-    const auto& path = atspiObject.path();
>-    if (!m_cache.remove(path))
>-        return;
>-
>-    g_dbus_connection_emit_signal(m_connection.get(), nullptr, "/org/a11y/atspi/cache", "org.a11y.atspi.Cache", "RemoveAccessible",
>-        g_variant_new("((so))", uniqueName(), path.utf8().data()), nullptr);
>+    auto cacheUpdateList = std::exchange(m_cacheUpdateList, { });
>+    for (auto& atspiObject : cacheUpdateList)
>+        addToCacheIfNeeded(*atspiObject);
> }
> 
> void AccessibilityAtspi::cacheClearTimerFired()
>diff --git a/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h b/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h
>index e9f252a1e93b..2104a4023239 100644
>--- a/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h
>+++ b/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h
>@@ -80,8 +80,6 @@ public:
> 
>     static const char* localizedRoleName(AccessibilityRole);
> 
>-    void addAccessible(AccessibilityObjectAtspi&);
>-
> #if ENABLE(DEVELOPER_MODE)
>     using NotificationObserverParameter = std::variant<std::nullptr_t, String, bool, unsigned, Ref<AccessibilityObjectAtspi>>;
>     using NotificationObserver = Function<void(AccessibilityObjectAtspi&, const char*, NotificationObserverParameter)>;
>@@ -107,8 +105,6 @@ private:
> 
>     void ensureCache();
>     void addToCacheIfNeeded(AccessibilityObjectAtspi&);
>-    void addToCacheIfPending(AccessibilityObjectAtspi&);
>-    void removeAccessible(AccessibilityObjectAtspi&);
>     void cacheUpdateTimerFired();
>     void cacheClearTimerFired();
> 
>diff --git a/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp b/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
>index 0b02b0aace2a..ebd95530ffdc 100644
>--- a/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
>+++ b/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
>@@ -509,8 +509,6 @@ bool AccessibilityObjectAtspi::registerObject()
>         interfaces.append({ const_cast<GDBusInterfaceInfo*>(&webkit_table_cell_interface), &s_tableCellFunctions });
> 
>     m_path = AccessibilityAtspi::singleton().registerObject(*this, WTFMove(interfaces));
>-    AccessibilityAtspi::singleton().addAccessible(*this);
>-
>     return true;
> }
> 
>@@ -1174,7 +1172,8 @@ void AccessibilityObjectAtspi::serialize(GVariantBuilder* builder) const
>     g_variant_builder_add(builder, "@(so)", parentReference());
> 
>     g_variant_builder_add(builder, "i", indexInParent());
>-    g_variant_builder_add(builder, "i", childCount());
>+    // Do not set the children count in cache, because children are handled by children-changed signals.
>+    g_variant_builder_add(builder, "i", -1);
> 
>     GVariantBuilder interfaces = G_VARIANT_BUILDER_INIT(G_VARIANT_TYPE("as"));
>     buildInterfaces(&interfaces);
>diff --git a/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp b/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp
>index f9ba62731f11..72c23ef085cb 100644
>--- a/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp
>+++ b/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp
>@@ -234,7 +234,8 @@ void AccessibilityRootAtspi::serialize(GVariantBuilder* builder) const
>     g_variant_builder_add(builder, "@(so)", parentReference());
> 
>     g_variant_builder_add(builder, "i", 0);
>-    g_variant_builder_add(builder, "i", child() ? 1 : 0);
>+    // Do not set the children count in cache, because child is handled by children-changed signals.
>+    g_variant_builder_add(builder, "i", -1);
> 
>     GVariantBuilder interfaces = G_VARIANT_BUILDER_INIT(G_VARIANT_TYPE("as"));
>     g_variant_builder_add(&interfaces, "s", webkit_accessible_interface.name);

Flags:

aperez: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 235700: 450117