Skip to content

Commit

Permalink
Reland "Improve slot a11y invalidation/cleanup"
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
aleventhal authored and Chromium LUCI CQ committed Feb 28, 2023
1 parent b0d8951 commit f748591
Show file tree
Hide file tree
Showing 10 changed files with 25 additions and 85 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@ rootWebArea htmlTag='#document'
++++++genericContainer ignored invisible htmlTag='div'
++++++++genericContainer ignored invisible htmlTag='my-element'
++++++++++genericContainer ignored invisible htmlTag='div'
++++++button htmlTag='button' name='Done'
++++++++++++genericContainer ignored invisible htmlTag='slot'
++++++++++++++genericContainer ignored invisible htmlTag='span'
++++++++++++++++staticText ignored invisible name='Slot contents'
++++++button htmlTag='button' name='Done'
2 changes: 2 additions & 0 deletions content/test/data/accessibility/html/meter-expected-blink.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ rootWebArea
++++++++++genericContainer
++++++++++++genericContainer ignored
++++++++genericContainer ignored invisible
++++++++++genericContainer ignored invisible
++++++++++++staticText ignored invisible name='2 out of 10'
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ UNKNOWN focusable has_input_focus
++++++++UNKNOWN
++++++++++UNKNOWN
++++++++++++UNKNOWN hidden
++++++++UNKNOWN hidden
++++++++UNKNOWN hidden
++++++++++UNKNOWN hidden
++++++++++++STATIC_TEXT hidden label='2 out of 10'
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ rootWebArea name=''
++++++++++genericContainer
++++++++++++genericContainer ignored
++++++++genericContainer ignored invisible
++++++++++genericContainer ignored invisible
15 changes: 3 additions & 12 deletions third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3177,18 +3177,9 @@ String AXNodeObject::GetValueForControl() const {
if (auto* select_menu = HTMLSelectMenuElement::OwnerSelectMenu(node)) {
DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
if (HTMLOptionElement* selected = select_menu->selectedOption()) {
// 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();
if (selected->firstChild()) {
return selected->textContent();
}
}
return String();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,6 @@ 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 @@ -1132,57 +1126,6 @@ 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 @@ -3672,18 +3615,6 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,6 @@ 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 f748591

Please sign in to comment.