Skip to content

GitLab

T
tangled
Commit 9eb8c1ae authored by Dominic Mazzoni's avatar Dominic Mazzoni Committed by Commit Bot
Browse files
Options

Re-land: Use PaintInvalidator to optimize AX bounding boxes 

Original patch: http://crrev.com/c/2319411
Revert: http://crrev.com/c/2326560

See diff between first and last patchsets.

The revert was due to the new DCHECK added in
render_accessibility_impl.cc. I carefully debugged all
of the failures and there was only one real failure
(slider thumbs) which are now fixed with a few lines of
code to explicitly invalidate them. The other
failures were all false positives - in every case,
the bounds of a LayoutObject had changed but the
paint invalidation simply hadn't come yet - but it
eventually came and all bounding boxes became correct.

So I'm just removing the DCHECK. The performance win is
great, and we're not seeing any cases where bounds aren't
invalidated where they should be. We can revisit if we
see failures in the wild.

Original description:

Previously, any time there was a layout, the accessibility
serialization code would scan every AXObject to see if any
needed to update their bounding box.

Instead, replace this with a mechanism that uses
PaintInvalidator to mark AXObjects that might have
dirty bounding boxes.

Existing tests already provide some coverage; if you
comment out the code in paint_invalidator.cc, a
handful of browser tests fail.

To provide even more coverage, a debug-only check
walks the entire tree and ensures that no nodes
have incorrect bounding boxes. If you try commenting
out the code in paint_invalidator.cc now, hundreds
of browser tests fail. This provides some good
confidence that it's working correctly.

Finally, add a new blink perf test demonstrating a
simple scenario where this results in a dramatic
speedup, just by focusing links in a document with a
few thousand nodes.

Bug: 1109081
Change-Id: Ia6bdbbac243c816a613d7b44807c91534b33221e
AX-Relnotes: makes accessibility less sluggish on very large web pages
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333573Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: default avatarNektarios Paisios <nektar@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795148}
parent 7428498c
Hide whitespace changes
Inline Side-by-side
Showing with 150 additions and 16 deletions
content/renderer/accessibility/render_accessibility_impl.cc
View file @ 9eb8c1ae
......@@ -986,15 +986,11 @@ void RenderAccessibilityImpl::SendLocationChanges() {
if (!root.UpdateLayoutAndCheckValidity())
return;
// Do a breadth-first explore of the whole blink AX tree.
base::queue<WebAXObject> objs_to_explore;
objs_to_explore.push(root);
while (objs_to_explore.size()) {
WebAXObject obj = objs_to_explore.front();
objs_to_explore.pop();
blink::WebVector<WebAXObject> changed_bounds_objects;
root.GetAllObjectsWithChangedBounds(changed_bounds_objects);
for (const WebAXObject& obj : changed_bounds_objects) {
// See if we had a previous location. If not, this whole subtree must
// be new, so don't continue to explore this branch.
// be new, so no need to update.
int id = obj.AxID();
if (!tree_source_->HasCachedBoundingBox(id))
continue;
......@@ -1007,12 +1003,6 @@ void RenderAccessibilityImpl::SendLocationChanges() {
// Save the new location.
tree_source_->SetCachedBoundingBox(id, new_location);
// Explore children of this object.
std::vector<WebAXObject> children;
tree_source_->GetChildren(obj, &children);
for (WebAXObject& child : children)
objs_to_explore.push(child);
}
// Ensure that the number of cached bounding boxes doesn't exceed the
......
third_party/blink/perf_tests/accessibility/focus-links.html 0 → 100644
View file @ 9eb8c1ae
<!DOCTYPE html>
<html>
<body>
<script src="../resources/runner.js"></script>
<table id="testElement">
<tr>
<th>Sender</th>
<td>Message</td>
</tr>
</table>
<script>
var isDone = false;
var startTime;
// Before the test starts, add 2000 rows to the table, something like a
// message board with lots of replies on a long thread.
let table = document.getElementById('testElement');
for (let i = 0; i < 2000; i++) {
let tr = document.createElement('tr');
table.appendChild(tr);
let sender = document.createElement('td');
sender.innerHTML = 'user' + Math.floor(10000*Math.random());
tr.appendChild(sender);
let message = document.createElement('td');
message.innerHTML = '<div>Message content ' +
Math.floor(10000*Math.random()) + '</div>';
let link = document.createElement('a');
link.href = '#';
link.id = 'link' + i;
link.innerHTML = 'Reply';
message.appendChild(link);
tr.appendChild(message);
}
function runTest() {
if (startTime) {
PerfTestRunner.measureValueAsync(PerfTestRunner.now() - startTime);
PerfTestRunner.addRunTestEndMarker();
}
if (!isDone) {
PerfTestRunner.addRunTestStartMarker();
startTime = PerfTestRunner.now();
// Iterate over some of the links and focus each one with a different
// delay. Just focusing a link shouldn't incur a large cost,
// even if the page has a lot of elements.
for (let i = 0; i < 100; i++) {
window.setTimeout(() => {
document.getElementById('link' + i).focus();
}, 10 * i);
}
// Wait to allow the asynchronous accessibility code that's
// covered by traceEventsToMeasure to have a chance to run.
setTimeout(runTest, 1500);
}
}
PerfTestRunner.startMeasureValuesAsync({
description: 'Test accessibility performance when appending to a textarea.',
unit: 'ms',
done: function () {
isDone = true;
},
run: function() {
runTest();
},
iterationCount: 6,
tracingCategories: 'accessibility',
traceEventsToMeasure: [
'RenderAccessibilityImpl::SendPendingAccessibilityEvents',
]
});
</script>
</html>
third_party/blink/public/web/web_ax_object.h
View file @ 9eb8c1ae
......@@ -390,6 +390,12 @@ class WebAXObject {
SkMatrix44& container_transform,
bool* clips_children = nullptr) const;
// Retrieves a vector of all WebAXObjects in this document whose
// bounding boxes may have changed since the last query. Can be called
// on any object.
BLINK_EXPORT void GetAllObjectsWithChangedBounds(
WebVector<WebAXObject>& out_changed_bounds_objects) const;
// Blink-internal DOM Node ID. Currently used for PDF exporting.
BLINK_EXPORT int GetDOMNodeId() const;
......
third_party/blink/renderer/core/accessibility/ax_object_cache.h
View file @ 9eb8c1ae
......@@ -145,6 +145,9 @@ class CORE_EXPORT AXObjectCache : public GarbageCollected<AXObjectCache> {
// without producing any layout or other notifications.
virtual void HandleFrameRectsChanged(Document&) = 0;
// Called when a layout object's bounding box may have changed.
virtual void InvalidateBoundingBox(const LayoutObject*) = 0;
virtual const AtomicString& ComputedRoleForNode(Node*) = 0;
virtual String ComputedNameForNode(Node*) = 0;
......
third_party/blink/renderer/core/paint/paint_invalidator.cc
View file @ 9eb8c1ae
......@@ -284,6 +284,9 @@ bool PaintInvalidator::InvalidatePaint(
PaintInvalidatorContext::kSubtreeInvalidationChecking;
}
if (AXObjectCache* cache = object.GetDocument().ExistingAXObjectCache())
cache->InvalidateBoundingBox(&object);
return reason != PaintInvalidationReason::kNone;
}
......
third_party/blink/renderer/modules/accessibility/ax_node_object.cc
View file @ 9eb8c1ae
......@@ -198,8 +198,7 @@ void AXNodeObject::AlterSliderOrSpinButtonValue(bool increase) {
if (IsDetached())
return;
AXObjectCache().PostNotification(GetNode(),
ax::mojom::blink::Event::kValueChanged);
AXObjectCache().HandleValueChanged(GetNode());
return;
}
......
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
View file @ 9eb8c1ae
......@@ -2004,6 +2004,18 @@ AXObject* AXObjectCacheImpl::GetActiveAriaModalDialog() const {
return active_aria_modal_dialog_;
}
HeapVector<Member<AXObject>>
AXObjectCacheImpl::GetAllObjectsWithChangedBounds() {
VectorOf<AXObject> changed_bounds_objects;
changed_bounds_objects.ReserveCapacity(changed_bounds_ids_.size());
for (AXID changed_bounds_id : changed_bounds_ids_) {
if (AXObject* obj = ObjectFromAXID(changed_bounds_id))
changed_bounds_objects.push_back(obj);
}
changed_bounds_ids_.clear();
return changed_bounds_objects;
}
void AXObjectCacheImpl::HandleInitialFocus() {
PostNotification(document_, ax::mojom::Event::kFocus);
}
......@@ -2051,6 +2063,15 @@ void AXObjectCacheImpl::HandleTextMarkerDataAdded(Node* start, Node* end) {
void AXObjectCacheImpl::HandleValueChanged(Node* node) {
PostNotification(node, ax::mojom::Event::kValueChanged);
// If it's a slider, invalidate the thumb's bounding box.
AXObject* ax_object = Get(node);
if (ax_object && ax_object->RoleValue() == ax::mojom::blink::Role::kSlider &&
ax_object->HasChildren() && !ax_object->NeedsToUpdateChildren() &&
ax_object->ChildCountIncludingIgnored() == 1) {
changed_bounds_ids_.insert(
ax_object->ChildAtIncludingIgnored(0)->AXObjectID());
}
}
void AXObjectCacheImpl::HandleUpdateActiveMenuOption(LayoutObject* menu_list,
......@@ -2126,6 +2147,12 @@ void AXObjectCacheImpl::HandleFrameRectsChanged(Document& document) {
MarkAXObjectDirty(Get(&document), false);
}
void AXObjectCacheImpl::InvalidateBoundingBox(
const LayoutObject* layout_object) {
if (AXObject* obj = Get(const_cast<LayoutObject*>(layout_object)))
changed_bounds_ids_.insert(obj->AXObjectID());
}
void AXObjectCacheImpl::HandleScrollPositionChanged(
LocalFrameView* frame_view) {
SCOPED_DISALLOW_LIFECYCLE_TRANSITION(*frame_view->GetFrame().GetDocument());
......
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
View file @ 9eb8c1ae
......@@ -163,6 +163,10 @@ class MODULES_EXPORT AXObjectCacheImpl
// without producing any layout or other notifications.
void HandleFrameRectsChanged(Document&) override;
// Invalidates the bounding box, which can be later retrieved by
// GetAllObjectsWithChangedBounds.
void InvalidateBoundingBox(const LayoutObject*) override;
const AtomicString& ComputedRoleForNode(Node*) override;
String ComputedNameForNode(Node*) override;
......@@ -284,6 +288,11 @@ class MODULES_EXPORT AXObjectCacheImpl
bool UseAXMenuList() { return use_ax_menu_list_; }
// Retrieves a vector of all AXObjects whose bounding boxes may have changed
// since the last query. Clears the vector so that the next time it's
// called, it will only retrieve objects that have changed since now.
HeapVector<Member<AXObject>> GetAllObjectsWithChangedBounds();
protected:
void PostPlatformNotification(
AXObject* obj,
......@@ -492,6 +501,10 @@ class MODULES_EXPORT AXObjectCacheImpl
// Maps ids to their object's autofill state.
HashMap<AXID, WebAXAutofillState> autofill_state_map_;
// The set of node IDs whose bounds has changed since the last time
// GetAllObjectsWithChangedBounds was called.
HashSet<AXID> changed_bounds_ids_;
// The source of the event that is currently being handled.
ax::mojom::blink::EventFrom active_event_from_ =
ax::mojom::blink::EventFrom::kNone;
......
third_party/blink/renderer/modules/exported/web_ax_object.cc
View file @ 9eb8c1ae
......@@ -1502,6 +1502,20 @@ void WebAXObject::GetRelativeBounds(WebAXObject& offset_container,
bounds_in_container = WebFloatRect(bounds);
}
void WebAXObject::GetAllObjectsWithChangedBounds(
WebVector<WebAXObject>& out_changed_bounds_objects) const {
if (IsDetached())
return;
HeapVector<Member<AXObject>> changed_bounds_objects =
private_->AXObjectCache().GetAllObjectsWithChangedBounds();
out_changed_bounds_objects.reserve(changed_bounds_objects.size());
out_changed_bounds_objects.resize(changed_bounds_objects.size());
std::copy(changed_bounds_objects.begin(), changed_bounds_objects.end(),
out_changed_bounds_objects.begin());
}
bool WebAXObject::ScrollToMakeVisible() const {
if (IsDetached())
return false;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment