Log Message
[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 Adrian Perez de Castro. It's confusing the cache if things don'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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (288725 => 288726)
--- trunk/Source/WebCore/ChangeLog 2022-01-28 06:59:31 UTC (rev 288725)
+++ trunk/Source/WebCore/ChangeLog 2022-01-28 07:00:55 UTC (rev 288726)
@@ -1,5 +1,47 @@
2022-01-27 Carlos Garcia Campos <[email protected]>
+ [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 Adrian Perez de Castro.
+
+ It's confusing the cache if things don'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 <[email protected]>
+
[GTK][a11y] Keep a reference to the parent wrapper with ATSPI
https://bugs.webkit.org/show_bug.cgi?id=235699
Modified: trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp (288725 => 288726)
--- trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp 2022-01-28 06:59:31 UTC (rev 288725)
+++ trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp 2022-01-28 07:00:55 UTC (rev 288726)
@@ -329,6 +329,10 @@
}
m_atspiObjects.add(&atspiObject, WTFMove(registeredObjects));
+ m_cacheUpdateList.add(&atspiObject);
+ if (!m_cacheUpdateTimer.isActive())
+ m_cacheUpdateTimer.startOneShot(0_s);
+
return path;
}
@@ -344,11 +348,18 @@
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)
g_dbus_connection_unregister_object(m_connection.get(), id);
@@ -380,13 +391,10 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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,45 +774,13 @@
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());
+ auto cacheUpdateList = std::exchange(m_cacheUpdateList, { });
+ for (auto& atspiObject : cacheUpdateList)
+ addToCacheIfNeeded(*atspiObject);
}
-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);
-}
-
void AccessibilityAtspi::cacheClearTimerFired()
{
for (const auto& registeredObjects : m_atspiHyperlinks.values()) {
Modified: trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h (288725 => 288726)
--- trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h 2022-01-28 06:59:31 UTC (rev 288725)
+++ trunk/Source/WebCore/accessibility/atspi/AccessibilityAtspi.h 2022-01-28 07:00:55 UTC (rev 288726)
@@ -80,8 +80,6 @@
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 @@
void ensureCache();
void addToCacheIfNeeded(AccessibilityObjectAtspi&);
- void addToCacheIfPending(AccessibilityObjectAtspi&);
- void removeAccessible(AccessibilityObjectAtspi&);
void cacheUpdateTimerFired();
void cacheClearTimerFired();
Modified: trunk/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp (288725 => 288726)
--- trunk/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp 2022-01-28 06:59:31 UTC (rev 288725)
+++ trunk/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp 2022-01-28 07:00:55 UTC (rev 288726)
@@ -509,8 +509,6 @@
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 @@
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);
Modified: trunk/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp (288725 => 288726)
--- trunk/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp 2022-01-28 06:59:31 UTC (rev 288725)
+++ trunk/Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp 2022-01-28 07:00:55 UTC (rev 288726)
@@ -234,7 +234,8 @@
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);
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
