Filter out empty ID in attribute changed callback

Per spec [1], empty ID attributes must be treated identically as
nonexistent. This CL implements these steps faithfully, and results in
DocumentFragment::getElementById() always returning nullptr when the
empty string is given, thus fixing bug 1068580.

Additionally, previously Document::getElementById() explicitly returned
nullptr when the empty string or the null string is given. The empty
string handling is no longer necessary after this CL, and thus removed.
(The null string handling is still necessary for internal C++ code that
relies on the short-circuiting behavior).

[1]: https://dom.spec.whatwg.org/#concept-id

Change-Id: Ia65024efef9e5ef54d9db233590a17b372282ed1
Fixed: 1068580
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140054Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757310}

third_party/blink/renderer/core/dom/element.cc

@@ -2432,6 +2432,11 @@ void Element::AttributeChanged(const AttributeModificationParams& params) {
     AtomicString old_id = GetElementData()->IdForStyleResolution();
     AtomicString new_id = MakeIdForStyleResolution(
         params.new_value, GetDocument().InQuirksMode());
+    // Treat empty IDs the same way as nonexistent ID, per
+    // https://dom.spec.whatwg.org/#concept-id.
+    if (new_id.IsEmpty()) {
+      new_id = g_null_atom;
+    }
     if (new_id != old_id) {
       GetElementData()->SetIdForStyleResolution(new_id);
       GetDocument().GetStyleEngine().IdChangedForElement(old_id, new_id, *this);

third_party/blink/renderer/core/dom/element.h

@@ -1299,7 +1299,17 @@ inline const AtomicString& Element::IdForStyleResolution() const {
 }
 
 inline const AtomicString& Element::GetIdAttribute() const {
-  return HasID() ? FastGetAttribute(html_names::kIdAttr) : g_null_atom;
+  // Note: HasID() can return false even if the id attribute exists. This
+  // happens when the attribute value is empty, which per spec is equivalent to
+  // not having an id attribute at all:
+  // https://dom.spec.whatwg.org/#concept-id. On the other hand, if HasID()
+  // returns true then there must be a non-empty id attribute.
+  if (HasID()) {
+    const AtomicString& attr_value = FastGetAttribute(html_names::kIdAttr);
+    DCHECK(!attr_value.IsEmpty());
+    return attr_value;
+  }
+  return g_null_atom;
 }
 
 inline const AtomicString& Element::GetNameAttribute() const {

third_party/blink/renderer/core/dom/tree_scope.cc

@@ -110,7 +110,9 @@ void TreeScope::ClearScopedStyleResolver() {
 }
 
 Element* TreeScope::getElementById(const AtomicString& element_id) const {
-  if (element_id.IsEmpty())
+  // |element_id| can never be null for calls that originated from JavaScript,
+  // but sometimes other C++ code can call this function with g_null_atom.
+  if (element_id.IsNull())
     return nullptr;
   if (!elements_by_id_)
     return nullptr;

third_party/blink/web_tests/external/wpt/dom/nodes/DocumentFragment-getElementById-expected.txt

-This is a testharness.js-based test.
-PASS The method must exist
-PASS It must return null when there are no matches
-PASS It must return the first element when there are matches
-FAIL Empty string ID values assert_equals: Even if there is an element with an empty-string ID attribute, it must not be returned expected null but got Element node <div id=""></div>
-PASS It must return the first element when there are matches, using a template
-Harness: the test ran to completion.
-