Skip to content

Commit

Permalink
Revert "[A11y] Reland targeted cached property invalidation"
Browse files Browse the repository at this point in the history
This reverts commit 3438f2a.

Reason for revert: Suspect causing blink_wpt_tests and blink_web_tests failure on Linux Tests (dbg)(1) bot.

Failed tests:
blink_wpt_tests failed because of:
external/wpt/css/css-contain/content-visibility/detach-locked-slot-children-crash.html
external/wpt/css/css-contain/content-visibility/element-reassigned-to-skipped-slot.html
external/wpt/css/css-contain/content-visibility/element-reassigned-to-slot-in-skipped-subtree.html
external/wpt/html/semantics/forms/the-input-element/focus-dynamic-type-change-on-blur.html
...7 more failure(s) (11 total)...

blink_web_tests failed because of:
accessibility/details-summary-crash.html
fast/events/drag-on-removed-slider-does-not-crash.html
fast/forms/range/range-type-change-onchange-2.html
html/details_summary/details-add-summary.html
...5 more failure(s) (9 total)...

First build failure:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/114032/overview

Sample log:
---
STDERR: #6 0x7fc01bd0361d logging::CheckError::~CheckError()
STDERR: #7 0x7fbffd21d08a blink::FlatTreeTraversal::AssertPrecondition()
STDERR: #8 0x7fbffd21cfd5 blink::FlatTreeTraversal::Parent()
STDERR: #9 0x7fbffd3c2a45 blink::FlatTreeTraversal::AncestorsOf()
STDERR: #10 0x7fbffd3d410e blink::(anonymous namespace)::NearestLockedExclusiveAncestor()
STDERR: #11 0x7fbffd3d3d17 blink::DisplayLockUtilities::IsInUnlockedOrActivatableSubtree()
STDERR: #12 0x7fbfedfabc4b blink::DisplayLockUtilities::ShouldIgnoreNodeDueToDisplayLock()
STDERR: #13 0x7fbfedf8b61c blink::AXObject::ToString()
STDERR: #14 0x7fbfedf8eccc blink::AXObject::Detach()
STDERR: #15 0x7fbfedf512df blink::AXNodeObject::Detach()
STDERR: #16 0x7fbfedf2d6a5 blink::AXLayoutObject::Detach()
STDERR: #17 0x7fbfedfce51a blink::AXObjectCacheImpl::Remove()
STDERR: #18 0x7fbfedfcd998 blink::AXObjectCacheImpl::Remove()
STDERR: #19 0x7fbfedfcdd74 blink::AXObjectCacheImpl::Remove()
STDERR: #20 0x7fbfedfce992 blink::AXObjectCacheImpl::Remove()
STDERR: #21 0x7fbffe6d16be blink::LayoutObject::WillBeDestroyed()
---

Original change's description:
> [A11y] Reland targeted cached property invalidation
>
> Relands the following CLS:
> * Enhance performance by targeting value updates to specific nodes, commit 704633e.
> * Don't queue anything for irrelevant attribute changes, commit 2c66a62.
> * Add comment explaining call to UpdateStyleAndLayoutTreeForNode(), commit 73b9eed.
> * Ensure cached values not invalidated during the computation of them, commit 4d167a6.
> * Run a test with --force-renderer-accessibility that used to fail, commit cab7ecd.
> * Simplify code to update cached focusable state, commit 79cb184.
> * Simplify code that invalidates cached values on an AXObject, commit 6df79a5.
>
> Fixed: 1446864, 1446550, 1434555, 1362758
> Change-Id: I16855bdcb746cb41387b69e1e97ab72ffc47e342
> Cq-Do-Not-Cancel-Tryjobs: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4545510
> Commit-Queue: Aaron Leventhal <[email protected]>
> Reviewed-by: Jacques Newman <[email protected]>
> Reviewed-by: Chris Harrelson <[email protected]>
> Reviewed-by: Philip Rogers <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1157878}

Change-Id: Iefe59fe1933747346eda8827fd683f310d6cddb3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4615927
Reviewed-by: Takuto Ikuta <[email protected]>
Owners-Override: Takuto Ikuta <[email protected]>
Commit-Queue: Takuto Ikuta <[email protected]>
Auto-Submit: Takashi Sakamoto <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1157993}
  • Loading branch information
tasak authored and Chromium LUCI CQ committed Jun 15, 2023
1 parent c2dc249 commit 01bdd68
Show file tree
Hide file tree
Showing 29 changed files with 396 additions and 547 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,8 @@ IN_PROC_BROWSER_TEST_F(CrossPlatformAccessibilityBrowserTest,
// Select controls behave differently on Mac/Android, this test doesn't apply.
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC)
IN_PROC_BROWSER_TEST_F(CrossPlatformAccessibilityBrowserTest,
SelectWithOptgroupActiveDescendant) {
// TODO(crbug.com/1446550): Re-enable this test
DISABLED_SelectWithOptgroupActiveDescendant) {
LoadInitialAccessibilityTreeFromHtml(R"HTML(
<!DOCTYPE html>
<html>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2656,12 +2656,14 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
}

IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
AccessibilityModalDialogClosed) {
// TODO(crbug.com/1446550): Re-enable this test
DISABLED_AccessibilityModalDialogClosed) {
RunHtmlTest(FILE_PATH_LITERAL("modal-dialog-closed.html"));
}

IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
AccessibilityModalDialogOpened) {
// TODO(crbug.com/1446550): Re-enable this test
DISABLED_AccessibilityModalDialogOpened) {
RunHtmlTest(FILE_PATH_LITERAL("modal-dialog-opened.html"));
}

Expand All @@ -2685,7 +2687,8 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTestWithIgnoredNodes,
}

IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
AccessibilityModalDialogStack) {
// TODO(crbug.com/1446550): Re-enable this test
DISABLED_AccessibilityModalDialogStack) {
RunHtmlTest(FILE_PATH_LITERAL("modal-dialog-stack.html"));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ bool RenderAccessibilityImpl::IsImmediateProcessingRequiredForEvent(
case ax::mojom::Event::kTreeChanged:
return serialize_post_lifecycle_;

case ax::mojom::Event::kAriaAttributeChanged:
case ax::mojom::Event::kDocumentTitleChanged:
case ax::mojom::Event::kExpandedChanged:
case ax::mojom::Event::kHide:
Expand All @@ -637,7 +638,6 @@ bool RenderAccessibilityImpl::IsImmediateProcessingRequiredForEvent(
// These events are not fired from Blink.
// This list is duplicated in WebFrameTestProxy::PostAccessibilityEvent().
case ax::mojom::Event::kAlert:
case ax::mojom::Event::kAriaAttributeChanged:
case ax::mojom::Event::kAutocorrectionOccured:
case ax::mojom::Event::kChildrenChanged:
case ax::mojom::Event::kControlsChanged:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,4 @@ rootWebArea name='done'
++++++heading ignored invisible
++++++++staticText ignored invisible name='Stay hidden'
++++++heading ignored invisible
++++++++group ignored invisible
++++++++++mark ignored invisible
++++++++++++staticText ignored invisible name='xyz'
++++++++staticText ignored invisible name='To hidden'
++++++heading name='From hidden'
++++++++group
++++++++++mark
++++++++++++staticText name='xyz'
++++++++++++++inlineTextBox name='xyz'
++++++++staticText name='From hidden'
++++++++++inlineTextBox name='From hidden'
++++++++staticText ignored invisible name='To hidden'
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
@WAIT-FOR:done
-->
<h4 id="stay-hidden" aria-hidden="true">Stay hidden</h4>
<h4 id="to-hidden"><div role=group><mark>xyz</mark></div>To hidden</h4>
<h4 id="from-hidden"><div role=group><mark>xyz</mark></div>From hidden</h4>
<h4 id="to-hidden">To hidden</h4>
<script>
document.addEventListener('DOMContentLoaded', () => {
setTimeout(() => {
document.getElementById('to-hidden').setAttribute('aria-hidden', 'true');
document.getElementById('from-hidden').setAttribute('aria-hidden', 'false');
document.title = 'done';
}, 100);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,3 @@ rootWebArea
++++++++genericContainer
++++++++++staticText name='done'
++++++++++++inlineTextBox name='done'
++++++group
++++++++mark
++++++++++staticText name='text'
++++++++++++inlineTextBox name='text'
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
</span>
<div><code>pending</code></div>
</div>
<div role="group" id="collapsed2" style='visibility: visible'><mark>text</mark></div>

<script>
setTimeout(()=> {
document.getElementById('collapsed').style.visibility = 'collapsed';
document.getElementById('collapsed2').style.visibility = 'collapsed';
document.querySelector('code').innerText = 'done';
}, 200);
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,3 @@ rootWebArea
++++++++genericContainer
++++++++++staticText name='done'
++++++++++++inlineTextBox name='done'
++++++group ignored invisible
++++++++mark ignored invisible
++++++++++staticText ignored invisible name='text'
2 changes: 0 additions & 2 deletions content/test/data/accessibility/css/visibility-to-hidden.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
</span>
<div><code>pending</code></div>
</div>
<div role="group" id="hidden2" style='visibility: visible'><mark>text</mark></div>

<script>
setTimeout(()=> {
document.getElementById('hidden').style.visibility = 'hidden';
document.getElementById('hidden2').style.visibility = 'hidden';
document.querySelector('code').innerText = 'done';
}, 200);
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,3 @@ rootWebArea
++++++++genericContainer
++++++++++staticText name='done'
++++++++++++inlineTextBox name='done'
++++++group
++++++++mark
++++++++++staticText name='text'
++++++++++++inlineTextBox name='text'
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
</span>
<div><code>pending</code></div>
</div>
<div role="group" id="visible2" style='visibility: hidden'><mark>text</mark></div>

<script>
setTimeout(()=> {
document.getElementById('visible').style.visibility = 'visible';
document.getElementById('visible2').style.visibility = 'visible';
document.querySelector('code').innerText = 'done';
}, 200);
</script>
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
STATE-CHANGE:EXPANDED:TRUE role=ROLE_LINK name='Toggle' ENABLED,EXPANDABLE,EXPANDED,FOCUSABLE,SENSITIVE,SHOWING,VISIBLE
=== Start Continuation ===
NAME-CHANGED:(null) role=ROLE_LINK name='(null)' ENABLED,SENSITIVE
PARENT-CHANGED PARENT:(role=ROLE_DOCUMENT_WEB name='(null)') role=ROLE_LINK name='Toggle' ENABLED,EXPANDABLE,FOCUSABLE,SENSITIVE,SHOWING,VISIBLE
STATE-CHANGE:EXPANDED:FALSE role=ROLE_LINK name='Toggle' ENABLED,EXPANDABLE,FOCUSABLE,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-ATTRIBUTES-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ class CORE_EXPORT AXObjectCache : public GarbageCollected<AXObjectCache> {
virtual const Element* RootAXEditableElement(const Node*) = 0;

// Called when aspects of the style (e.g. color, alignment) change.
virtual void StyleChanged(const LayoutObject*,
bool visibility_or_inertness_changed = false) = 0;
virtual void StyleChanged(const LayoutObject*) = 0;

// Called by a node when text or a text equivalent (e.g. alt) attribute is
// changed.
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/layout/layout_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2790,7 +2790,8 @@ void LayoutObject::StyleWillChange(StyleDifference diff,

if (visibility_changed || style_->IsInert() != new_style.IsInert()) {
if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache()) {
cache->StyleChanged(this, /*visibility_or_inertness_changed*/ true);
cache->ChildrenChanged(Parent());
cache->ChildrenChanged(this);
}
}

Expand Down
15 changes: 0 additions & 15 deletions third_party/blink/renderer/core/layout/layout_text.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,6 @@ bool LayoutText::IsWordBreak() const {
return false;
}

void LayoutText::StyleWillChange(StyleDifference diff,
const ComputedStyle& new_style) {
NOT_DESTROYED();

if (const ComputedStyle* current_style = Style()) {
// Process accessibility for style changes that affect text.
if (current_style->Visibility() != new_style.Visibility() ||
current_style->IsInert() != new_style.IsInert()) {
if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache()) {
cache->StyleChanged(this, /*visibility_or_inertness_changed*/ true);
}
}
}
}

void LayoutText::StyleDidChange(StyleDifference diff,
const ComputedStyle* old_style) {
NOT_DESTROYED();
Expand Down
5 changes: 3 additions & 2 deletions third_party/blink/renderer/core/layout/layout_text.h
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,9 @@ class CORE_EXPORT LayoutText : public LayoutObject {
protected:
void WillBeDestroyed() override;

void StyleWillChange(StyleDifference, const ComputedStyle&) final;

void StyleWillChange(StyleDifference, const ComputedStyle&) final {
NOT_DESTROYED();
}
void StyleDidChange(StyleDifference, const ComputedStyle* old_style) override;

void InLayoutNGInlineFormattingContextWillChange(bool) final;
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/renderer/core/svg/svg_element.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ class CORE_EXPORT SVGElement : public Element {
static void SynchronizeListOfSVGAttributes(
const base::span<SVGAnimatedPropertyBase*> attributes);

bool HasFocusEventListeners() const;

protected:
SVGElement(const QualifiedName&,
Document&,
Expand Down Expand Up @@ -282,6 +280,8 @@ class CORE_EXPORT SVGElement : public Element {
void ReportAttributeParsingError(SVGParsingError,
const QualifiedName&,
const AtomicString&);
bool HasFocusEventListeners() const;

void AddedEventListener(const AtomicString& event_type,
RegisteredEventListener&) override;
void RemovedEventListener(const AtomicString& event_type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,8 @@ AXRestriction AXMenuListPopup::Restriction() const {
bool AXMenuListPopup::ComputeAccessibilityIsIgnored(
IgnoredReasons* ignored_reasons) const {
// Base whether the menupopup is ignored on the containing <select>.
if (parent_) {
parent_->UpdateCachedAttributeValuesIfNeeded();
if (parent_)
return parent_->ComputeAccessibilityIsIgnored(ignored_reasons);
}

return kIgnoreObject;
}
Expand All @@ -73,7 +71,6 @@ AXMenuListOption* AXMenuListPopup::MenuListOptionAXObject(
DCHECK(IsA<HTMLOptionElement>(*element));

AXObject* ax_object = AXObjectCache().GetOrCreate(element, this);
ax_object->SetParent(this);

return DynamicTo<AXMenuListOption>(ax_object);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4740,6 +4740,10 @@ bool AXNodeObject::OnNativeBlurAction() {
}

bool AXNodeObject::OnNativeFocusAction() {
// Checking if node is focusable in a native focus action requires that we
// have updated style and layout tree, since the focus check relies on the
// existence of layout objects to determine the result. However, these layout
// objects may have been deferred by display-locking.
Document* document = GetDocument();
Node* node = GetNode();
if (!document || !node)
Expand Down
Loading

0 comments on commit 01bdd68

Please sign in to comment.