Documents must keep same AXObjectCache/AXTreeSerializer [chromium/src : main]
0 views
Skip to first unread message
Aaron Leventhal (Gerrit)
Mar 19, 2023, 1:54:25 PM3/19/23
to David Tseng, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org
Attention is currently required from: David Tseng.
Aaron Leventhal would like David Tseng to review this change.
Documents must keep same AXObjectCache/AXTreeSerializer
A clever trick that was used to batch major document changes into a
single operation was to destroy the existing AXObjectCache. For
example, this was used for large-scale inertness changes, e.g. caused
by a dialog being toggled, or by a fullscreen mode toggle.
The unintended consequence of this was that a new AXTreeSerializer was
also created, which has logic to assume the first node it sees is
the root node. This wreaks havoc when the tree is deserialized, because
there is already a root node. The Unserialize() function assumes the
new root is correct and destroys the old root's subtree.
Follow-up: when we land tree consistency check, we could pass in a boolean value that is true when the serializer is new. Then on the
browser side, for new serializers, we can check that there aren't
any nodes already present.
Bug: 1421550
Change-Id: If36641435884445b692f249a7933c4af8ed94b48
---
M third_party/blink/renderer/core/accessibility/ax_object_cache.h
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/frame/local_frame.cc
M third_party/blink/renderer/core/fullscreen/fullscreen.cc
M third_party/blink/renderer/core/html/html_dialog_element.cc
M third_party/blink/renderer/core/page/page.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
M ui/accessibility/ax_tree_serializer.h
10 files changed, 59 insertions(+), 31 deletions(-)
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 19, 2023, 1:54:28 PM3/19/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: David Tseng.
Patch set 4:Auto-Submit +1
Aaron Leventhal (Gerrit)
Mar 20, 2023, 12:21:53 PM3/20/23
to Chris Harrelson, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng
Attention is currently required from: Chris Harrelson, David Tseng.
Aaron Leventhal would like Chris Harrelson to review this change.
M ui/accessibility/ax_tree.cc
M ui/accessibility/ax_tree_serializer.h
11 files changed, 62 insertions(+), 32 deletions(-)
Aaron Leventhal (Gerrit)
Mar 20, 2023, 12:21:58 PM3/20/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Chris Harrelson, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Chris Harrelson, David Tseng.
Patch set 5:Auto-Submit +1
David Tseng (Gerrit)
Mar 20, 2023, 3:58:46 PM3/20/23
to Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Chris Harrelson, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
lgtm
Patch set 5:Code-Review +1
4 comments:
File third_party/blink/renderer/core/dom/document.cc:
Patch Set #5, Line 3135:
g_ax_object_cache_count--;I wish we could get rid of g_ax_object_cache_count; it's quite confusing as used. e.g. move it as a static member of AXObjectCache.
File third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc:
Patch Set #5, Line 3670:
// This method is useful when something that potentially affects most of theUsually comments like this go into the header.
Patch Set #5, Line 3678:
// Tell the serializer that everything will need to be serialized.nit: insert blank line above.
Patch Set #5, Line 3680:
// Send the serialization at the next available opportunity.nit: insert blank line above.
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 20, 2023, 4:59:49 PM3/20/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng, Chris Harrelson, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Chris Harrelson, David Tseng.
1 comment:
File third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc:
Patch Set #5, Line 3670:
// This method is useful when something that potentially affects most of theUsually comments like this go into the header.
I have a follow-up that improves this more. I'll address these nits there.
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 20, 2023, 5:02:54 PM3/20/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng, Chris Harrelson, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Chris Harrelson, David Tseng.
3 comments:
File third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc:
Patch Set #5, Line 3670:
// This method is useful when something that potentially affects most of theUsually comments like this go into the header.
Done
Patch Set #5, Line 3678:
// Tell the serializer that everything will need to be serialized.nit: insert blank line above.
Done
Patch Set #5, Line 3680:
// Send the serialization at the next available opportunity.nit: insert blank line above.
Done
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 21, 2023, 12:18:10 PM3/21/23
to Mason Freed, Chris Harrelson, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng
Attention is currently required from: David Tseng, Mason Freed.
Aaron Leventhal would like Mason Freed to review this change.
Aaron Leventhal removed Chris Harrelson from this change.
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 21, 2023, 12:18:15 PM3/21/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mason Freed, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: David Tseng, Mason Freed.
1 comment:
Patchset:
=> Mason, for DOM stuff
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Mar 21, 2023, 12:26:37 PM3/21/23
to Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Aaron Leventhal, David Tseng.
Patch set 5:Code-Review +1
2 comments:
Patchset:
LGTM, but can you please do the refactor thing I asked about?
File third_party/blink/renderer/core/frame/local_frame.cc:
if (AXObjectCache* cache = GetDocument()->ExistingAXObjectCache()) {
cache->MarkDocumentDirty();
}Since these three lines appear many times in this CL, does it make sense to refactor them into a new `Document::MarkAxObjectCacheDirty()` function? That would clean up the code and avoid having to include `ax_object_cache.h` everywhere.
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 21, 2023, 12:27:47 PM3/21/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mason Freed, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: David Tseng.
1 comment:
File third_party/blink/renderer/core/frame/local_frame.cc:
if (AXObjectCache* cache = GetDocument()->ExistingAXObjectCache()) {
cache->MarkDocumentDirty();
}
Since these three lines appear many times in this CL, does it make sense to refactor them into a new […]
np
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Mar 21, 2023, 12:39:48 PM3/21/23
to abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mason Freed, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: David Tseng.
Patch set 6:Commit-Queue +2
Mason Freed (Gerrit)
Mar 21, 2023, 1:48:00 PM3/21/23
to Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mason Freed, David Tseng, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Attention is currently required from: Aaron Leventhal, David Tseng.
1 comment:
File third_party/blink/renderer/core/page/page.cc:
Patch Set #6, Line 719:
DeprecatedLocalMainFrame()->GetDocument()->RefreshAccessibilityTree();So much better. Thanks!
To view, visit change 4351150. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Mar 21, 2023, 1:49:15 PM3/21/23
to Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mason Freed, Mason Freed, David Tseng, Akihiro Ota, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios
Chromium LUCI CQ submitted this change.
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/dom/document.cc
Insertions: 6, Deletions: 0.
@@ -3151,6 +3151,12 @@
return cache_owner.ax_object_cache_.Get();
}
+void Document::RefreshAccessibilityTree() const {
+ if (AXObjectCache* cache = ExistingAXObjectCache()) {
+ cache->MarkDocumentDirty();
+ }
+}
+
CanvasFontCache* Document::GetCanvasFontCache() {
if (!canvas_font_cache_)
canvas_font_cache_ = MakeGarbageCollected<CanvasFontCache>(*this);
```
```
The name of the file: third_party/blink/renderer/core/html/html_dialog_element.cc
Insertions: 2, Deletions: 7.
@@ -26,7 +26,6 @@
#include "third_party/blink/renderer/core/html/html_dialog_element.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_focus_options.h"
-#include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
#include "third_party/blink/renderer/core/css/style_change_reason.h"
#include "third_party/blink/renderer/core/dom/events/event.h"
#include "third_party/blink/renderer/core/dom/events/native_event_listener.h"
@@ -136,9 +135,7 @@
// tree can change inertness which means they must be added or removed from
// the tree. The most foolproof way is to clear the entire tree and rebuild
// it, though a more clever way is probably possible.
- if (AXObjectCache* cache = document.ExistingAXObjectCache()) {
- cache->MarkDocumentDirty();
- }
+ document.RefreshAccessibilityTree();
}
HTMLDialogElement::HTMLDialogElement(Document& document)
@@ -284,9 +281,7 @@
SetIsModal(true);
- // Throw away the AX cache first, so the subsequent steps don't have a chance
- // of queuing up AX events on objects that would be invalidated when the cache
- // is thrown away.
+ // Refresh the AX cache first, because most of it is changing.
InertSubtreesChanged(document, old_modal_dialog);
document.UpdateStyleAndLayout(DocumentUpdateReason::kJavaScript);
```
```
The name of the file: third_party/blink/renderer/core/fullscreen/fullscreen.cc
Insertions: 1, Deletions: 4.
@@ -34,7 +34,6 @@
#include "third_party/blink/public/platform/task_type.h"
#include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_fullscreen_options.h"
-#include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
#include "third_party/blink/renderer/core/css/style_change_reason.h"
#include "third_party/blink/renderer/core/css/style_engine.h"
#include "third_party/blink/renderer/core/dom/document.h"
@@ -120,9 +119,7 @@
// Any element not contained by the fullscreen element is inert (see
// |Node::IsInert()|), so changing the fullscreen element will typically
// change the inertness of most elements. Reserialize the entire document.
- if (AXObjectCache* cache = document.ExistingAXObjectCache()) {
- cache->MarkDocumentDirty();
- }
+ document.RefreshAccessibilityTree();
if (LocalFrame* frame = document.GetFrame()) {
// TODO(foolip): Synchronize hover state changes with animation frames.
```
```
The name of the file: third_party/blink/renderer/core/frame/local_frame.cc
Insertions: 2, Deletions: 7.
@@ -76,7 +76,6 @@
#include "third_party/blink/public/web/web_local_frame_client.h"
#include "third_party/blink/renderer/bindings/core/v8/script_controller.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
-#include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
#include "third_party/blink/renderer/core/clipboard/system_clipboard.h"
#include "third_party/blink/renderer/core/content_capture/content_capture_manager.h"
#include "third_party/blink/renderer/core/core_export.h"
@@ -1032,9 +1031,7 @@
// Nodes all over the accessibility tree can change inertness which means they
// must be added or removed from the tree.
if (GetDocument()) {
- if (AXObjectCache* cache = GetDocument()->ExistingAXObjectCache()) {
- cache->MarkDocumentDirty();
- }
+ GetDocument()->RefreshAccessibilityTree();
}
}
@@ -1579,9 +1576,7 @@
void LocalFrame::PortalStateChanged() {
if (GetDocument()) {
- if (AXObjectCache* cache = GetDocument()->ExistingAXObjectCache()) {
- cache->MarkDocumentDirty();
- }
+ GetDocument()->RefreshAccessibilityTree();
}
if (IsOutermostMainFrame()) {
```
```
The name of the file: third_party/blink/renderer/core/dom/document.h
Insertions: 4, Deletions: 0.
@@ -756,6 +756,10 @@
// AXContexts are deleted, the AXObjectCache will be removed.
AXObjectCache* ExistingAXObjectCache() const;
Document& AXObjectCacheOwner() const;
+ // If there is an accessibility tree, recompute it and re-serialize it all.
+ // This method is useful when something that potentially affects most of the
+ // page occurs, such as an inertness change or a fullscreen toggle.
+ void RefreshAccessibilityTree() const;
// to get visually ordered hebrew and arabic pages right
bool VisuallyOrdered() const { return visually_ordered_; }
```
```
The name of the file: third_party/blink/renderer/core/page/page.cc
Insertions: 1, Deletions: 6.
@@ -28,7 +28,6 @@
#include "third_party/blink/public/platform/platform.h"
#include "third_party/blink/public/web/blink.h"
#include "third_party/blink/renderer/bindings/core/v8/script_controller.h"
-#include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/css/media_feature_overrides.h"
#include "third_party/blink/renderer/core/css/style_change_reason.h"
@@ -717,11 +716,7 @@
if (!MainFrame() || !MainFrame()->IsLocalFrame()) {
break;
}
- if (AXObjectCache* cache = DeprecatedLocalMainFrame()
- ->GetDocument()
- ->ExistingAXObjectCache()) {
- cache->MarkDocumentDirty();
- }
+ DeprecatedLocalMainFrame()->GetDocument()->RefreshAccessibilityTree();
break;
case ChangeType::kViewportStyle: {
auto* main_local_frame = DynamicTo<LocalFrame>(MainFrame());
```
Approvals:
Aaron Leventhal: Send CL to CQ automatically after approval; Commit
David Tseng: Looks good to me
Mason Freed: Looks good to me
Documents must keep same AXObjectCache/AXTreeSerializer
A clever trick that was used to batch major document changes into a
single operation was to destroy the existing AXObjectCache. For
example, this was used for large-scale inertness changes, e.g. caused
by a dialog being toggled, or by a fullscreen mode toggle.
The unintended consequence of this was that a new AXTreeSerializer was
also created, which has logic to assume the first node it sees is
the root node. This wreaks havoc when the tree is deserialized, because
there is already a root node. The Unserialize() function assumes the
new root is correct and destroys the old root's subtree.
Follow-up: when we land tree consistency check, we could pass in a boolean value that is true when the serializer is new. Then on the
browser side, for new serializers, we can check that there aren't
any nodes already present.
Bug: 1421550
Change-Id: If36641435884445b692f249a7933c4af8ed94b48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4351150
Reviewed-by: Mason Freed <mas...@chromium.org>
Commit-Queue: Aaron Leventhal <aleve...@chromium.org>
Reviewed-by: David Tseng <dts...@chromium.org>
Auto-Submit: Aaron Leventhal <aleve...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1120044}
---
M third_party/blink/renderer/core/accessibility/ax_object_cache.h
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/frame/local_frame.cc
M third_party/blink/renderer/core/fullscreen/fullscreen.cc
M third_party/blink/renderer/core/html/html_dialog_element.cc
M third_party/blink/renderer/core/page/page.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
M ui/accessibility/ax_tree.cc
M ui/accessibility/ax_tree_serializer.h
11 files changed, 58 insertions(+), 36 deletions(-)
Reply all
Reply to author
Forward
0 new messages