Skip to content

Commit

Permalink
Merge M115: Revert "Reland "Improve slot a11y invalidation/cleanup""
Browse files Browse the repository at this point in the history
This reverts commit f748591.

Reason for revert: performance regression https://crbug.com/1447357

Original change's description:
> Reland "Improve slot a11y invalidation/cleanup"
>
> This is a reland of commit dd9058a
>
> Reason for revert: broke Fuchsia test expectations.
> https://ci.chromium.org/p/chromium/builders/ci/fuchsia-arm64-cast-receiver-rel/3859. Fuchsia should not actually be picking up ignored nodes at all, and therefore it should not have regressed. I've filed a bug on Fuchsia to change that behavior: crbug.com/1420272
>
> Patchset 1 contains the originally-landed code.
>
> Original change's description:
> > Improve slot a11y invalidation/cleanup
> >
> > Remove obscure logic related to determining if a slot is relevant, and just treat them all as relevant. I believe this was required to pass some PDF accessibility tests in the pass, but no longer seems to be. This causes a couple of changes to Blink expectations, but because the irrelevant slots are still hidden/ignored, the platform expectations are not changing, which is what's really important.
> >
> > As a result, we no longer leave extra objects in AXObjectCache or serializer when an there are changes related to slots. This is verified by a follow-up, CL:4185240, and this CL is necessary to pass the checks added there.
> >
> > Bug: None
> >
> > Change-Id: If787fcfc79a3568ffacc262d29f7cc4a5d89dbba
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4295193
> > Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
> > Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1110744}
>
> Bug: None
> Change-Id: Ic323ad86a57c6254659660979311636ff3bd213f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4294848
> Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
> Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1111125}

(cherry picked from commit 71b3160)

Fixed: 1447357
Change-Id: Ia8d5463baa90f831f30d41e4bfcf8d314ec2953b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4566753
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149384}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4573789
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5790@{#147}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
aleventhal authored and Chromium LUCI CQ committed May 30, 2023
1 parent e5a47cd commit 1228548
Show file tree
Hide file tree
Showing 10 changed files with 85 additions and 25 deletions.
Expand Up @@ -5,7 +5,4 @@ rootWebArea htmlTag='#document'
++++++genericContainer ignored invisible htmlTag='div'
++++++++genericContainer ignored invisible htmlTag='my-element'
++++++++++genericContainer ignored invisible htmlTag='div'
++++++++++++genericContainer ignored invisible htmlTag='slot'
++++++++++++++genericContainer ignored invisible htmlTag='span'
++++++++++++++++staticText ignored invisible name='Slot contents'
++++++button htmlTag='button' name='Done'
++++++button htmlTag='button' name='Done'
2 changes: 0 additions & 2 deletions content/test/data/accessibility/html/meter-expected-blink.txt
Expand Up @@ -6,5 +6,3 @@ rootWebArea
++++++++++genericContainer
++++++++++++genericContainer ignored
++++++++genericContainer ignored invisible
++++++++++genericContainer ignored invisible
++++++++++++staticText ignored invisible name='2 out of 10'
Expand Up @@ -5,6 +5,4 @@ UNKNOWN focusable has_input_focus
++++++++UNKNOWN
++++++++++UNKNOWN
++++++++++++UNKNOWN hidden
++++++++UNKNOWN hidden
++++++++++UNKNOWN hidden
++++++++++++STATIC_TEXT hidden label='2 out of 10'
++++++++UNKNOWN hidden
Expand Up @@ -10,9 +10,6 @@ rootWebArea
++++++++genericContainer ignored invisible
++++++++++staticText ignored invisible name='Listbox role on ul'
++++++details ignored invisible
++++++++genericContainer ignored invisible
++++++++++disclosureTriangle ignored invisible
++++++++++++staticText ignored invisible name='summary/details popover'
++++++button collapsed name='Button pointing to hidden popover'
++++++++staticText name='Button pointing to hidden popover'
++++++++++inlineTextBox name='Button pointing to hidden popover'
Expand Down
Expand Up @@ -12,14 +12,10 @@ rootWebArea focusable
++++++++++++++genericContainer ignored
++++++++genericContainer ignored
++++++++++listBox ignored invisible
++++++++++++genericContainer ignored invisible
++++++++++++++listBoxOption focusable ignored invisible
++++++++++++++listBoxOption focusable ignored invisible
++++++++++++++listBoxOption focusable ignored invisible
++++++genericContainer
++++++++genericContainer ignored
++++++++++comboBoxMenuButton collapsed value='Option 1' haspopup=listbox
++++++++++++staticText name='Custom selectmenu button'
++++++++++++++inlineTextBox name='Custom selectmenu button'
++++++++genericContainer ignored
++++++++++listBox ignored invisible
++++++++++listBox ignored invisible
Expand Up @@ -12,10 +12,6 @@ UNKNOWN focusable has_input_focus
++++++++++++++UNKNOWN hidden
++++++++UNKNOWN hidden
++++++++++UNKNOWN hidden
++++++++++++UNKNOWN hidden
++++++++++++++UNKNOWN hidden focusable actions='{DEFAULT}'
++++++++++++++UNKNOWN hidden focusable actions='{DEFAULT}'
++++++++++++++UNKNOWN hidden focusable actions='{DEFAULT}'
++++++UNKNOWN
++++++++UNKNOWN hidden
++++++++++UNKNOWN actions='{DEFAULT}' value='Option 1'
Expand Down
Expand Up @@ -6,4 +6,3 @@ rootWebArea name=''
++++++++++genericContainer
++++++++++++genericContainer ignored
++++++++genericContainer ignored invisible
++++++++++genericContainer ignored invisible
15 changes: 12 additions & 3 deletions third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Expand Up @@ -3212,9 +3212,18 @@ String AXNodeObject::GetValueForControl() const {
if (auto* select_menu = HTMLSelectMenuElement::OwnerSelectMenu(node)) {
DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
if (HTMLOptionElement* selected = select_menu->selectedOption()) {
if (selected->firstChild()) {
return selected->textContent();
}
// TODO(accessibility) Because these <option> elements can contain
// anything, we need to create an AXObject for the selected option, and
// use ax_selected_option->ComputedName(). However, for now, the
// AXObject is not created because AXObject::IsRelevantSlotElement()
// returns false for the invisible slot parent. Also, strangely,
// selected->innerText()/GetInnerTextWithoutUpdate() are returning "".
// See the following content_browsertest:
// All/DumpAccessibilityTreeTest.AccessibilitySelectMenu/blink.
// TODO(crbug.com/1401767): DCHECK fails with synchronous serialization.
DCHECK(selected->firstChild())
<< "There is a selected option but it has no DOM children.";
return selected->textContent();
}
return String();
}
Expand Down
Expand Up @@ -515,6 +515,12 @@ bool IsSubtreePrunedForAccessibility(const Element* node) {
return true;
}

if (const HTMLSlotElement* slot =
ToHTMLSlotElementIfSupportsAssignmentOrNull(node)) {
if (!AXObjectCacheImpl::IsRelevantSlotElement(*slot))
return true;
}

// <optgroup> is irrelevant inside of a <select> menulist.
if (auto* opt_group = DynamicTo<HTMLOptGroupElement>(node)) {
if (auto* select = opt_group->OwnerSelectElement()) {
Expand Down Expand Up @@ -1130,6 +1136,57 @@ bool AXObjectCacheImpl::ShouldCreateAXMenuListFor(LayoutObject* layout_object) {
return false;
}

// static
bool AXObjectCacheImpl::IsRelevantSlotElement(const HTMLSlotElement& slot) {
// A <slot> descendant of a node that is still in the DOM but no longer
// rendered will return true for Node::isConnected() and false for
// AXObject::IsDetached(). But from the perspective of platform ATs, this
// subtree is not connected and is detached unless it is canvas fallback
// content. In order to detect this condition, we look to the first non-slot
// parent. If it has a layout object, the <slot>'s contents are rendered.
// If it doesn't, but it's in the canvas subtree, those contents should be
// treated as canvas fallback content.
//
// The alternative way to determine whether the <slot> is still relevant for
// rendering is to iterate FlatTreeTraversal::Parent until you get to the last
// parent, and see if it's a document. If it is not a document, then it is not
// relevant. This seems much slower than just checking GetLayoutObject() as it
// needs to iterate the parent chain. However, checking GetLayoutObject()
// could produce null in the case of something that is
// content-visibility:auto. This means that any slotted content inside
// content-visibility:auto may be removed from the AX tree depending on
// whether it was recently rendered.
//
// TODO(accessibility) This fails for the web test
// detach-locked-slot-children-crash.html with --force-renderer-accessibility.
// See web_tests/FlagExpectations/force-renderer-accessibility.
// There should be a better way to accomplish this.
// Could a new function be added to the slot element?
const Node* parent = LayoutTreeBuilderTraversal::Parent(slot);
if (const HTMLSlotElement* parent_slot =
ToHTMLSlotElementIfSupportsAssignmentOrNull(parent)) {
return AXObjectCacheImpl::IsRelevantSlotElement(*parent_slot);
}

if (parent && parent->GetLayoutObject())
return true;

const Element* parent_element = DynamicTo<Element>(parent);
if (!parent_element)
return false;

// Authors can include elements as "Fallback content" inside a <canvas> in
// order to provide an alternative means to interact with the canvas using
// a screen reader. Those should always be included.
if (parent_element->IsInCanvasSubtree())
return true;

// LayoutObject::CreateObject() will not create an object for elements
// with display:contents. If we do not include a <slot> for that reason,
// any descendants will be not be included in the accessibility tree.
return parent_element->HasDisplayContentsStyle();
}

// static
bool AXObjectCacheImpl::IsRelevantPseudoElement(const Node& node) {
DCHECK(node.IsPseudoElement());
Expand Down Expand Up @@ -3826,6 +3883,18 @@ AXObject* AXObjectCacheImpl::GetSerializationTarget(AXObject* obj) {
return nullptr;
}

// A <slot> descendant of a node that is still in the DOM but no longer
// rendered will return true for Node::isConnected() and false for
// AXObject::IsDetached(). But from the perspective of platform ATs, this
// subtree is not connected and is detached.
// TODO(accessibility): The relevance check probably applies to all nodes
// not just slot elements.
if (const HTMLSlotElement* slot =
ToHTMLSlotElementIfSupportsAssignmentOrNull(obj->GetNode())) {
if (!AXObjectCacheImpl::IsRelevantSlotElement(*slot))
return nullptr;
}

// Ensure still in tree.
if (obj->IsMissingParent()) {
// TODO(accessibility) Only needed because of <select> size changes.
Expand Down
Expand Up @@ -440,6 +440,7 @@ class MODULES_EXPORT AXObjectCacheImpl
static bool IsRelevantPseudoElement(const Node& node);
static bool IsRelevantPseudoElementDescendant(
const LayoutObject& layout_object);
static bool IsRelevantSlotElement(const HTMLSlotElement& slot);

bool HasBeenDisposed() { return has_been_disposed_; }

Expand Down

0 comments on commit 1228548

Please sign in to comment.