Skip to content

GitLab

T
tangled
Commit 5a85445c authored by David Tseng's avatar David Tseng Committed by Commit Bot
Browse files
Options

Always defer focus events after clean layout 

In the attached bug (which has recently been re-introduced),
1. focus is requested on a node
2. in response, the page triggers some type of re-flow which causes the layout object of the node to be invalidated
3. some amount of non-determinism occurs where we might call AXObject CacheImpl::GetOrCreate on a node (keyed by layout object). This sometimes results in us creating a brand new AXLayoutObject or AXNodeObject for a Node that hasn't been destroyed.
4. when it arrives in the browser or in ChromeVox, the target of the focus looks like it was destroyed, so the focus event never gets fired.

Test: browser_tests --gtest_filter=Automation*.ForceLayout
Bug: 885244
Change-Id: I2f7747a98d15a6ae5b3ed75638a93d1078526015
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872422
Commit-Queue: David Tseng <dtseng@chromium.org>
Reviewed-by: default avatarAlice Boxhall <aboxhall@chromium.org>
Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709303}
parent 7e987473
Hide whitespace changes
Inline Side-by-side
Showing with 77 additions and 21 deletions
chrome/browser/extensions/api/automation/automation_apitest.cc
View file @ 5a85445c
......@@ -420,6 +420,12 @@ IN_PROC_BROWSER_TEST_F(AutomationApiTest, IgnoredNodesNotReturned) {
<< message_;
}
IN_PROC_BROWSER_TEST_F(AutomationApiTest, ForceLayout) {
StartEmbeddedTestServer();
ASSERT_TRUE(RunExtensionSubtest("automation/tests/tabs", "force_layout.html"))
<< message_;
}
#if defined(OS_CHROMEOS)
class AutomationApiTestWithDeviceScaleFactor : public AutomationApiTest {
......
chrome/test/data/extensions/api_test/automation/sites/force_layout.html 0 → 100644
View file @ 5a85445c
y<!--
* Copyright 2019 The Chromium Authors. All rights reserved. Use of this
* source code is governed by a BSD-style license that can be found in the
* LICENSE file.
-->
<html>
<head>
<title>Automation Tests - Force Layout</title>
</head>
<body>
<p>Test</p>
<button></button>
<script>
let button = document.body.querySelector('button');
button.addEventListener('focus', () => {
button.style.display = 'none';
button.style.width = 200;
button.style.display = 'block';
});
</script>
</body>
</html>
chrome/test/data/extensions/api_test/automation/tests/tabs/force_layout.html 0 → 100644
View file @ 5a85445c
<!--
* Copyright 2019 The Chromium Authors. All rights reserved. Use of this
* source code is governed by a BSD-style license that can be found in the
* LICENSE file.
-->
<script src="common.js"></script>
<script src="force_layout.js"></script>
chrome/test/data/extensions/api_test/automation/tests/tabs/force_layout.js 0 → 100644
View file @ 5a85445c
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
var allTests = [
function testForceLayoutFiresFocus() {
var node = rootNode.find({ role: 'button'});
assertEq('button', node.role);
rootNode.addEventListener('focus', (evt) => {
// The underlying DOM button has not changed, but its layout has. This
// listener ensures we at least get a focus event on a new valid
// accessibility object. Once display none nodes are in the tree, it's
// likely that |node| == |evt.target|.
assertFalse(evt.target == node);
assertEq(undefined, node.role);
assertEq('button', evt.target.role);
chrome.test.succeed();
});
node.focus();
}
];
setUpAndRunTests(allTests, 'force_layout.html');
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
View file @ 5a85445c
......@@ -773,8 +773,8 @@ AXObject::InOrderTraversalIterator AXObjectCacheImpl::InOrderTraversalEnd() {
void AXObjectCacheImpl::DeferTreeUpdateInternal(Node* node,
base::OnceClosure callback) {
tree_update_callback_queue_.push_back(
MakeGarbageCollected<TreeUpdateParams>(node, std::move(callback)));
tree_update_callback_queue_.push_back(MakeGarbageCollected<TreeUpdateParams>(
node, ComputeEventFrom(), std::move(callback)));
}
void AXObjectCacheImpl::DeferTreeUpdate(
......@@ -990,10 +990,15 @@ void AXObjectCacheImpl::ProcessUpdatesAfterLayout(Document& document) {
base::OnceClosure& callback = tree_update->callback;
if (node->GetDocument() != document) {
tree_update_callback_queue_.push_back(
MakeGarbageCollected<TreeUpdateParams>(node, std::move(callback)));
MakeGarbageCollected<TreeUpdateParams>(node, tree_update->event_from,
std::move(callback)));
continue;
}
bool saved_is_handling_action = is_handling_action_;
if (tree_update->event_from == ax::mojom::EventFrom::kAction)
is_handling_action_ = true;
std::move(callback).Run();
is_handling_action_ = saved_is_handling_action;
}
}
......@@ -1637,23 +1642,13 @@ void AXObjectCacheImpl::HandleFocusedUIElementChanged(
if (!page)
return;
if (RuntimeEnabledFeatures::AccessibilityExposeDisplayNoneEnabled()) {
if (old_focused_element) {
DeferTreeUpdate(&AXObjectCacheImpl::HandleNodeLostFocusWithCleanLayout,
old_focused_element);
}
DeferTreeUpdate(&AXObjectCacheImpl::HandleNodeGainedFocusWithCleanLayout,
this->FocusedElement());
} else {
AXObject* focused_object = this->FocusedObject();
if (!focused_object)
return;
AXObject* old_focused_object = Get(old_focused_element);
PostNotification(old_focused_object, ax::mojom::Event::kBlur);
PostNotification(focused_object, ax::mojom::Event::kFocus);
if (old_focused_element) {
DeferTreeUpdate(&AXObjectCacheImpl::HandleNodeLostFocusWithCleanLayout,
old_focused_element);
}
DeferTreeUpdate(&AXObjectCacheImpl::HandleNodeGainedFocusWithCleanLayout,
this->FocusedElement());
}
void AXObjectCacheImpl::HandleInitialFocus() {
......
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
View file @ 5a85445c
......@@ -288,9 +288,12 @@ class MODULES_EXPORT AXObjectCacheImpl
};
struct TreeUpdateParams final : public GarbageCollected<TreeUpdateParams> {
TreeUpdateParams(Node* node, base::OnceClosure callback)
: node(node), callback(std::move(callback)) {}
TreeUpdateParams(Node* node,
ax::mojom::EventFrom event_from,
base::OnceClosure callback)
: node(node), event_from(event_from), callback(std::move(callback)) {}
WeakMember<Node> node;
ax::mojom::EventFrom event_from;
base::OnceClosure callback;
void Trace(Visitor* visitor) { visitor->Trace(node); }
......
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