From 85eb5fce7ed6b9e3d0f5a64f6b182972b434d68c Mon Sep 17 00:00:00 2001 From: Chris Hall Date: Wed, 14 Apr 2021 08:22:40 +0000 Subject: [PATCH] [Element Reflection] Null vs Empty-list differentiation. If the content-attribute is not present, then we return null. If the content attribute is present, and there are no explicitly set attr-elements, and the content attribute doesn't yield any valid elements, then we return an empty array. This makes the interface more consistent, as a caller could already to the attribute to null to clear. This brings the implementation in-line with the update here https://github.com/whatwg/html/pull/3917#discussion_r607685136 This also updates/clarifies code comments and adds descriptive strings to existing tests. This is a subset of the work Alice did for https://crrev.com/c/2796854 Change-Id: I3dcc8492fdb5467f82468bbc3d4b7e4a87c5d48e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2825017 Reviewed-by: Alice Boxhall Reviewed-by: Kent Tamura Commit-Queue: Chris Hall Cr-Commit-Position: refs/heads/master@{#872329} GitOrigin-RevId: 5bc9faabddc88997f080101649cd38a1c42875f8 --- blink/renderer/core/dom/document.h | 4 ++-- blink/renderer/core/dom/element.cc | 5 +++-- .../modules/accessibility/ax_relation_cache.cc | 4 ++-- .../accessibility/ax_sparse_attribute_setter.cc | 2 +- .../aria-owns-dynamic-changes.html | 6 +++--- .../aria-element-reflection.tentative.html | 17 ++++++++--------- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/blink/renderer/core/dom/document.h b/blink/renderer/core/dom/document.h index 98124a5a976..3fc43047fc7 100644 --- a/blink/renderer/core/dom/document.h +++ b/blink/renderer/core/dom/document.h @@ -246,8 +246,8 @@ enum ShadowCascadeOrder { kShadowCascadeNone, kShadowCascade }; using DocumentClassFlags = unsigned char; -// A map of IDL attribute name to Element value, for one particular element. -// For example, +// A map of IDL attribute name to Element list value, for one particular +// element. For example, // el1.ariaActiveDescendant = el2 // would add the following pair to the ExplicitlySetAttrElementMap for el1: // ("ariaActiveDescendant", el2) diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc index b4732163a85..6706e3ea74c 100644 --- a/blink/renderer/core/dom/element.cc +++ b/blink/renderer/core/dom/element.cc @@ -941,6 +941,9 @@ base::Optional>> Element::GetElementArrayAttribute( : html_names::kAriaLabelledbyAttr; } + if (!hasAttribute(attr)) + return base::nullopt; + String attribute_value = getAttribute(attr).GetString(); Vector tokens; attribute_value = attribute_value.SimplifyWhiteSpace(); @@ -954,8 +957,6 @@ base::Optional>> Element::GetElementArrayAttribute( if (candidate) result_elements.push_back(candidate); } - if (result_elements.IsEmpty()) - return base::nullopt; return result_elements; } diff --git a/blink/renderer/modules/accessibility/ax_relation_cache.cc b/blink/renderer/modules/accessibility/ax_relation_cache.cc index 5b5be567e89..c60608976b5 100644 --- a/blink/renderer/modules/accessibility/ax_relation_cache.cc +++ b/blink/renderer/modules/accessibility/ax_relation_cache.cc @@ -264,8 +264,8 @@ void AXRelationCache::UpdateAriaOwnsWithCleanLayout(AXObject* owner) { UpdateReverseRelations(owner, owned_id_vector); // We first check if the element has an explicitly set aria-owns association. - // Explicitly set elements are validated on setting time (that they are in a - // valid scope etc). The content attribute can contain ids that are not + // Explicitly set elements are validated when they are read (that they are in + // a valid scope etc). The content attribute can contain ids that are not // legally ownable. HeapVector> owned_children; if (element && element->HasExplicitlySetAttrAssociatedElements( diff --git a/blink/renderer/modules/accessibility/ax_sparse_attribute_setter.cc b/blink/renderer/modules/accessibility/ax_sparse_attribute_setter.cc index 4fa047eea78..38c0d105796 100644 --- a/blink/renderer/modules/accessibility/ax_sparse_attribute_setter.cc +++ b/blink/renderer/modules/accessibility/ax_sparse_attribute_setter.cc @@ -70,7 +70,7 @@ void SetIntListAttribute(ax::mojom::blink::IntListAttribute attribute, return; base::Optional>> attr_associated_elements = element->GetElementArrayAttribute(qualified_name); - if (!attr_associated_elements) + if (!attr_associated_elements || attr_associated_elements.value().IsEmpty()) return; std::vector ax_ids; diff --git a/blink/web_tests/accessibility/aria-owns-dynamic-changes.html b/blink/web_tests/accessibility/aria-owns-dynamic-changes.html index 1d9e0031d45..f7b34777df1 100644 --- a/blink/web_tests/accessibility/aria-owns-dynamic-changes.html +++ b/blink/web_tests/accessibility/aria-owns-dynamic-changes.html @@ -21,7 +21,7 @@ child.id = ""; assert_equals(axFutureParent.childrenCount, 0, "after clearing id"); - assert_equals(futureParent.ariaOwnsElements, null); + assert_array_equals(futureParent.ariaOwnsElements, []); child.id = "future_child"; assert_equals(axFutureParent.childrenCount, 1, "after setting id"); @@ -29,7 +29,7 @@ futureParent.setAttribute("aria-owns", ""); assert_equals(axFutureParent.childrenCount, 0, "after clearing aria-owns"); - assert_equals(futureParent.ariaOwnsElements, null); + assert_array_equals(futureParent.ariaOwnsElements, []); futureParent.setAttribute("aria-owns", "future_child"); assert_equals(axFutureParent.childrenCount, 1, "after setting aria-oens"); @@ -46,7 +46,7 @@ child.remove(); assert_equals(axFutureParent.childrenCount, 0); - assert_equals(futureParent.ariaOwnsElements, null); + assert_array_equals(futureParent.ariaOwnsElements, []); }, "Aria-owns is dynamically recomputed."); diff --git a/blink/web_tests/external/wpt/dom/nodes/aria-element-reflection.tentative.html b/blink/web_tests/external/wpt/dom/nodes/aria-element-reflection.tentative.html index 44fc68acfd6..776fff31432 100644 --- a/blink/web_tests/external/wpt/dom/nodes/aria-element-reflection.tentative.html +++ b/blink/web_tests/external/wpt/dom/nodes/aria-element-reflection.tentative.html @@ -147,7 +147,7 @@