Skip to content

Commit

Permalink
Set empty string for reflection of IDREF attributes
Browse files Browse the repository at this point in the history 
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3934330
Reviewed-by: Mason Freed <[email protected]>
Reviewed-by: Joanmarie Diggs <[email protected]>
Commit-Queue: Manuel Rego <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1056288}
  • Loading branch information
@mrego
mrego authored and Chromium LUCI CQ committed Oct 7, 2022
1 parent bbe9a2b commit f3d72d8
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 136 deletions.
47 changes: 3 additions & 44 deletions third_party/blink/renderer/core/dom/element.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,22 +861,7 @@ void Element::SetElementAttribute(const QualifiedName& name, Element* element) {
return;
}

const AtomicString id = element->GetIdAttribute();

// In order to sprout a non-empty content attribute from an explicitly set
// attr-element, |element| must:
// 1) have a valid ID attribute, and
// 2) be the first element in tree order with this ID.
// Otherwise the content attribute will reflect the empty string.
//
// Note that the explicitly set attr-element is still set. See the spec for
// more details:
// https://whatpr.org/html/3917/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes
if (id.IsNull() || GetTreeScope() != element->GetTreeScope() ||
GetTreeScope().getElementById(id) != element)
setAttribute(name, g_empty_atom);
else
setAttribute(name, id);
setAttribute(name, g_empty_atom);

auto result = explicitly_set_attr_elements_map->insert(name, nullptr);
if (result.is_new_entry) {
Expand Down Expand Up @@ -931,6 +916,8 @@ void Element::SetElementArrayAttribute(
return;
}

setAttribute(name, g_empty_atom);

// Get or create element array, and remove any pre-existing elements.
//
// Note that this code intentionally performs two look ups on |name| within
Expand All @@ -947,39 +934,11 @@ void Element::SetElementArrayAttribute(
} else {
stored_elements->clear();
}
SpaceSplitString value;

for (auto element : *given_elements) {
// If |value| is null and |stored_elements| is non-empty, then a previous
// element must have been invalid wrt. the content attribute string rules,
// and therefore the content attribute string should reflect the empty
// string. This means we can stop trying to compute the content attribute
// string.
if (value.IsNull() && !stored_elements->empty()) {
stored_elements->insert(element);
continue;
}

stored_elements->insert(element);
const AtomicString given_element_id = element->GetIdAttribute();

// We compute the content attribute string as a space separated string of
// the given |element| ids. Every |element| in |given_elements| must have an
// id, must be in the same tree scope and must be the first id in tree order
// with that id, otherwise the content attribute should reflect the empty
// string.
if (given_element_id.IsNull() ||
GetTreeScope() != element->GetTreeScope() ||
GetTreeScope().getElementById(given_element_id) != element) {
value.Clear();
continue;
}

// Whitespace between elements is added when the string is serialized.
value.Add(given_element_id);
}

setAttribute(name, value.SerializeToString());
if (isConnected()) {
if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache())
cache->HandleAttributeChanged(name, this);
Expand Down
2 changes: 1 addition & 1 deletion third_party/blink/web_tests/dom/element/aria-element-reflection-garbagecollection.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@
// Set describedBy based on the order of my fruit preferences.
fruitBowl.ariaDescribedByElements = [pear, apple, banana];
assert_array_equals(fruitBowl.ariaDescribedByElements, [pear, apple, banana]);
assert_equals(fruitBowl.getAttribute("aria-describedby"), "pear apple banana");
assert_equals(fruitBowl.getAttribute("aria-describedby"), "");

// Someone else comes along and eats the apple.
apple.remove();
Expand Down
22 changes: 11 additions & 11 deletions .../blink/web_tests/external/wpt/custom-elements/reactions/AriaMixin-element-attributes.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<div id="parentElement"></div>
<script>

function testElementReflectAttribute(jsAttributeName, contentAttributeName, validValue1, contentValue1, validValue2, contentValue2, name, getParentElement) {
function testElementReflectAttribute(jsAttributeName, contentAttributeName, validValue1, validValue2, name, getParentElement) {
test(function () {
let element = define_new_custom_element([contentAttributeName]);
let instance = document.createElement(element.name);
Expand All @@ -27,7 +27,7 @@
let logEntries = element.takeLog();
assert_array_equals(logEntries.types(), ['attributeChanged']);

assert_attribute_log_entry(logEntries.last(), {name: contentAttributeName, oldValue: null, newValue: contentValue1, namespace: null});
assert_attribute_log_entry(logEntries.last(), {name: contentAttributeName, oldValue: null, newValue: "", namespace: null});
}, name + ' must enqueue an attributeChanged reaction when adding ' + contentAttributeName + ' content attribute');

test(function () {
Expand All @@ -39,7 +39,7 @@
instance[jsAttributeName] = validValue2;
var logEntries = element.takeLog();
assert_array_equals(logEntries.types(), ['attributeChanged']);
assert_attribute_log_entry(logEntries.last(), {name: contentAttributeName, oldValue: contentValue1, newValue: contentValue2, namespace: null});
assert_attribute_log_entry(logEntries.last(), {name: contentAttributeName, oldValue: "", newValue: "", namespace: null});
}, name + ' must enqueue an attributeChanged reaction when replacing an existing attribute');
}

Expand All @@ -51,14 +51,14 @@
dummy2.id = 'dummy2';
document.body.appendChild(dummy2);

testElementReflectAttribute('ariaActiveDescendantElement', 'aria-activedescendant', dummy1, 'dummy1', dummy2, 'dummy2', 'ariaActiveDescendantElement in Element');
testElementReflectAttribute('ariaControlsElements', 'aria-controls', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaControlsElements in Element');
testElementReflectAttribute('ariaDescribedByElements', 'aria-describedby', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaDescribedByElements in Element');
testElementReflectAttribute('ariaDetailsElements', 'aria-details', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaDetailsElements in Element');
testElementReflectAttribute('ariaErrorMessageElement', 'aria-errormessage', dummy1, 'dummy1', dummy2, 'dummy2', 'ariaErrorMessageElement in Element');
testElementReflectAttribute('ariaFlowToElements', 'aria-flowto', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaFlowToElements in Element');
testElementReflectAttribute('ariaLabelledByElements', 'aria-labelledby', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaLabelledByElements in Element')
testElementReflectAttribute('ariaOwnsElements', 'aria-owns', [dummy1], 'dummy1', [dummy2], 'dummy2', 'ariaOwnsElements in Element')
testElementReflectAttribute('ariaActiveDescendantElement', 'aria-activedescendant', dummy1, dummy2, 'ariaActiveDescendantElement in Element');
testElementReflectAttribute('ariaControlsElements', 'aria-controls', [dummy1], [dummy2], 'ariaControlsElements in Element');
testElementReflectAttribute('ariaDescribedByElements', 'aria-describedby', [dummy1], [dummy2], 'ariaDescribedByElements in Element');
testElementReflectAttribute('ariaDetailsElements', 'aria-details', [dummy1], [dummy2], 'ariaDetailsElements in Element');
testElementReflectAttribute('ariaErrorMessageElement', 'aria-errormessage', dummy1, dummy2, 'ariaErrorMessageElement in Element');
testElementReflectAttribute('ariaFlowToElements', 'aria-flowto', [dummy1], [dummy2], 'ariaFlowToElements in Element');
testElementReflectAttribute('ariaLabelledByElements', 'aria-labelledby', [dummy1], [dummy2], 'ariaLabelledByElements in Element')
testElementReflectAttribute('ariaOwnsElements', 'aria-owns', [dummy1], [dummy2], 'ariaOwnsElements in Element')

</script>
</body>
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/web_tests/external/wpt/html/dom/aria-element-reflection-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ PASS Setting the IDL attribute to an element which is not the first element in D
PASS Setting an element reference that crosses into a shadow tree is disallowed, but setting one that is in a shadow inclusive ancestor is allowed.
PASS aria-errormessage
PASS aria-details
PASS Deleting a reflected element should return null for the IDL attribute and cause the content attribute to become stale.
PASS Changing the ID of an element causes the content attribute to become out of sync.
PASS Deleting a reflected element should return null for the IDL attribute and the content attribute will be empty.
PASS Changing the ID of an element doesn't lose the reference.
PASS Reparenting an element into a descendant shadow scope hides the element reference.
PASS Reparenting referenced element cannot cause retargeting of reference.
PASS Element reference set in invalid scope remains intact throughout move to valid scope.
Expand Down
Loading

0 comments on commit f3d72d8

Please sign in to comment.