OPTION element: Update OPTION label even if it has invalid markup

https://html.spec.whatwg.org/C/#the-option-element > Content model: > If the element has no label attribute and is not a child of a datalist > element: Text that is not inter-element whitespace. OPTION elements should not have element children, but they can have if web authors add elements by DOM API. For both of drop-down SELECTs and listbox SELECTs, Chrome missed to update OPTION labels. It's a regression since Google Chrome 83 for drop-down SELECT, and the issue exists since the WebKit fork for listbox SELECTs. This CL fixes the issue by using MutationObserver only if an OPTION element has Element children. Bug: 1090806 Change-Id: I69f5aa4646fda0ad2770cc2748016c6b37b3ad06 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2235227 Commit-Queue: Kent Tamura <tkent@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#776436}
marcosholgado · Jun 9, 2020 · 027aefc · 027aefc
1 parent 5e30de3
commit 027aefc
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 0 deletions.
diff --git a/third_party/blink/renderer/core/html/forms/html_option_element.cc b/third_party/blink/renderer/core/html/forms/html_option_element.cc
@@ -26,8 +26,10 @@
 
 #include "third_party/blink/renderer/core/html/forms/html_option_element.h"
 
+#include "third_party/blink/renderer/bindings/core/v8/v8_mutation_observer_init.h"
 #include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
 #include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/dom/mutation_observer.h"
 #include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/dom/node_traversal.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
@@ -44,6 +46,37 @@
 
 namespace blink {
 
+class OptionTextObserver : public MutationObserver::Delegate {
+ public:
+  explicit OptionTextObserver(HTMLOptionElement& option)
+      : option_(option), observer_(MutationObserver::Create(this)) {
+    MutationObserverInit* init = MutationObserverInit::Create();
+    init->setCharacterData(true);
+    init->setChildList(true);
+    init->setSubtree(true);
+    observer_->observe(option_, init, ASSERT_NO_EXCEPTION);
+  }
+
+  ExecutionContext* GetExecutionContext() const override {
+    return option_->GetExecutionContext();
+  }
+
+  void Deliver(const MutationRecordVector& records,
+               MutationObserver&) override {
+    option_->DidChangeTextContent();
+  }
+
+  void Trace(Visitor* visitor) const override {
+    visitor->Trace(option_);
+    visitor->Trace(observer_);
+    MutationObserver::Delegate::Trace(visitor);
+  }
+
+ private:
+  Member<HTMLOptionElement> option_;
+  Member<MutationObserver> observer_;
+};
+
 HTMLOptionElement::HTMLOptionElement(Document& document)
     : HTMLElement(html_names::kOptionTag, document), is_selected_(false) {
   EnsureUserAgentShadowRoot();
@@ -81,6 +114,11 @@ HTMLOptionElement* HTMLOptionElement::CreateForJSConstructor(
   return element;
 }
 
+void HTMLOptionElement::Trace(Visitor* visitor) const {
+  visitor->Trace(text_observer_);
+  HTMLElement::Trace(visitor);
+}
+
 bool HTMLOptionElement::SupportsFocus() const {
   HTMLSelectElement* select = OwnerSelectElement();
   if (select && select->UsesMenuList())
@@ -280,6 +318,15 @@ void HTMLOptionElement::SetDirty(bool value) {
 
 void HTMLOptionElement::ChildrenChanged(const ChildrenChange& change) {
   HTMLElement::ChildrenChanged(change);
+  DidChangeTextContent();
+
+  // If an element is inserted, We need to use MutationObserver to detect
+  // textContent changes.
+  if (change.type == ChildrenChangeType::kElementInserted && !text_observer_)
+    text_observer_ = MakeGarbageCollected<OptionTextObserver>(*this);
+}
+
+void HTMLOptionElement::DidChangeTextContent() {
   if (HTMLDataListElement* data_list = OwnerDataListElement())
     data_list->OptionElementChildrenChanged();
   else if (HTMLSelectElement* select = OwnerSelectElement())

diff --git a/third_party/blink/renderer/core/html/forms/html_option_element.h b/third_party/blink/renderer/core/html/forms/html_option_element.h
@@ -33,6 +33,7 @@ namespace blink {
 class ExceptionState;
 class HTMLDataListElement;
 class HTMLSelectElement;
+class OptionTextObserver;
 
 class CORE_EXPORT HTMLOptionElement final : public HTMLElement {
   DEFINE_WRAPPERTYPEINFO();
@@ -53,6 +54,7 @@ class CORE_EXPORT HTMLOptionElement final : public HTMLElement {
                                                    ExceptionState&);
 
   explicit HTMLOptionElement(Document&);
+  void Trace(Visitor* visitor) const override;
 
   // A text to be shown to users.  The difference from |label()| is |label()|
   // returns an empty string if |label| content attribute is empty.
@@ -101,6 +103,9 @@ class CORE_EXPORT HTMLOptionElement final : public HTMLElement {
   void SetMultiSelectFocusedState(bool);
   bool IsMultiSelectFocused() const;
 
+  // Callback for OptionTextObserver.
+  void DidChangeTextContent();
+
  private:
   ~HTMLOptionElement() override;
 
@@ -117,6 +122,8 @@ class CORE_EXPORT HTMLOptionElement final : public HTMLElement {
 
   void UpdateLabel();
 
+  Member<OptionTextObserver> text_observer_;
+
   // Represents 'selectedness'.
   // https://html.spec.whatwg.org/C/#concept-option-selectedness
   bool is_selected_;

diff --git a/...nal/wpt/html/semantics/forms/the-option-element/dynamic-content-change-rendering-ref.html b/...nal/wpt/html/semantics/forms/the-option-element/dynamic-content-change-rendering-ref.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+
+<select>
+<option>foo</option>
+</select>
+
+<select multiple>
+<option>bar</option>
+</select>
+
+</bod>
+</html>
diff --git a/...xternal/wpt/html/semantics/forms/the-option-element/dynamic-content-change-rendering.html b/...xternal/wpt/html/semantics/forms/the-option-element/dynamic-content-change-rendering.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<title>Invalidation test on resetting &lt;select></title>
+<link rel="help" href="https://html.spec.whatwg.org/C/#concept-option-label">
+<link rel="help" href="http://crbug.com/1090806">
+<link rel="match" href="dynamic-content-change-rendering-ref.html">
+<body>
+
+<select id="dropdown">
+<option></option>
+</select>
+
+<select id="listbox" multiple>
+<option></option>
+</select>
+
+<script>
+const selects = document.querySelectorAll('select');
+
+const span0 = document.createElement('span');
+selects[0].options[0].appendChild(span0);
+
+const span1 = document.createElement('span');
+selects[1].options[0].appendChild(span1);
+
+document.documentElement.addEventListener('TestRendered', e => {
+  span0.textContent = 'foo';
+  span1.textContent = 'bar';
+  e.target.removeAttribute('class');
+});
+</script>
+</body>
+</html>