Android: Should not dispatch unnecessary 'change' event

<select multiple> dispatched unnecessary 'change' event if a user
changed nothing on a chooser dialog. This CL fixes it.

HTMLSelectElement::SelectMultipleOptionsByPopup() used
UpdateSelectedState() to update selection state and prepare for 'change'
event. However, it didn't work well mainly because SaveLastSelection()
doesn't support menulist <select multiple>.

This CL fixes this bug by not using UpdateSelectedState().
SelectMultipleOptionsByPopup() updates selection state and dispatches
'change' event by its own logic.

* Add SetDelegatesMenuListRenderingForTesting() to LayoutTheme for ease
  of testing.

This CL affect only Android, but the test is workable on all platforms.

Bug: 1060039
Change-Id: I7c9c5ee9bb51ec4aa7b2da21329c9a8d4c0778da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2096064
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748932}

third_party/blink/renderer/core/html/forms/html_select_element.cc

@@ -182,14 +182,33 @@ void HTMLSelectElement::SelectMultipleOptionsByPopup(
     const Vector<int>& list_indices) {
   DCHECK(UsesMenuList());
   DCHECK(IsMultiple());
-  for (wtf_size_t i = 0; i < list_indices.size(); ++i) {
-    bool add_selection_if_not_first = i > 0;
-    if (HTMLOptionElement* option = OptionAtListIndex(list_indices[i]))
-      UpdateSelectedState(option, add_selection_if_not_first, false);
+
+  HeapHashSet<Member<HTMLOptionElement>> old_selection;
+  for (auto* option : GetOptionList()) {
+    if (option->Selected()) {
+      old_selection.insert(option);
+      option->SetSelectedState(false);
+    }
+  }
+
+  bool has_new_selection = false;
+  for (int list_index : list_indices) {
+    if (auto* option = OptionAtListIndex(list_index)) {
+      option->SetSelectedState(true);
+      option->SetDirty(true);
+      auto iter = old_selection.find(option);
+      if (iter != old_selection.end())
+        old_selection.erase(iter);
+      else
+        has_new_selection = true;
+    }
   }
+
   SetNeedsValidityCheck();
-  // TODO(tkent): Using listBoxOnChange() is very confusing.
-  ListBoxOnChange();
+  if (has_new_selection || !old_selection.IsEmpty()) {
+    DispatchInputEvent();
+    DispatchChangeEvent();
+  }
 }
 
 unsigned HTMLSelectElement::ListBoxSize() const {
@@ -576,7 +595,7 @@ void HTMLSelectElement::UpdateListBoxSelection(bool deselect_other_options,
 }
 
 void HTMLSelectElement::ListBoxOnChange() {
-  DCHECK(!UsesMenuList() || is_multiple_);
+  DCHECK(!UsesMenuList());
 
   const ListItems& items = GetListItems();
 
@@ -1177,6 +1196,7 @@ bool HTMLSelectElement::PopupIsVisible() const {
 void HTMLSelectElement::UpdateSelectedState(HTMLOptionElement* clicked_option,
                                             bool multi,
                                             bool shift) {
+  DCHECK(!UsesMenuList());
   DCHECK(clicked_option);
   // Save the selection so it can be compared to the new selection when
   // dispatching change events during mouseup, or after autoscroll finishes.

third_party/blink/renderer/core/html/forms/html_select_element_test.cc

@@ -9,9 +9,11 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
+#include "third_party/blink/renderer/core/frame/settings.h"
 #include "third_party/blink/renderer/core/html/forms/form_controller.h"
 #include "third_party/blink/renderer/core/html/forms/html_form_element.h"
 #include "third_party/blink/renderer/core/html/forms/select_type.h"
+#include "third_party/blink/renderer/core/layout/layout_theme.h"
 #include "third_party/blink/renderer/core/testing/page_test_base.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 
@@ -20,6 +22,7 @@ namespace blink {
 class HTMLSelectElementTest : public PageTestBase {
  protected:
   void SetUp() override;
+  void TearDown() override;
 
   SelectType& GetSelectType(const HTMLSelectElement& select) {
     return *select.select_type_;
@@ -39,11 +42,29 @@ class HTMLSelectElementTest : public PageTestBase {
                                               HTMLOptionElement* option) {
     return GetSelectType(select).PreviousSelectableOption(option);
   }
+
+  bool FirstSelectIsConnectedAfterSelectMultiple(const Vector<int>& indices) {
+    auto* select = To<HTMLSelectElement>(GetDocument().body()->firstChild());
+    select->focus();
+    select->SelectMultipleOptionsByPopup(indices);
+    return select->isConnected();
+  }
+
+ private:
+  bool original_delegates_flag_;
 };
 
 void HTMLSelectElementTest::SetUp() {
   PageTestBase::SetUp();
   GetDocument().SetMimeType("text/html");
+  original_delegates_flag_ =
+      LayoutTheme::GetTheme().DelegatesMenuListRendering();
+}
+
+void HTMLSelectElementTest::TearDown() {
+  LayoutTheme::GetTheme().SetDelegatesMenuListRenderingForTesting(
+      original_delegates_flag_);
+  PageTestBase::TearDown();
 }
 
 TEST_F(HTMLSelectElementTest, SaveRestoreSelectSingleFormControlState) {
@@ -470,4 +491,62 @@ TEST_F(HTMLSelectElementTest, SlotAssignmentRecalcDuringOptionRemoval) {
   option->remove();
 }
 
+// crbug.com/1060039
+TEST_F(HTMLSelectElementTest, SelectMultipleOptionsByPopup) {
+  GetDocument().GetSettings()->SetScriptEnabled(true);
+  LayoutTheme::GetTheme().SetDelegatesMenuListRenderingForTesting(true);
+
+  // Select the same set of options.
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option>o0</option><option>o1</option></select>");
+    EXPECT_TRUE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{}))
+        << "Onchange handler should not be executed.";
+  }
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option>o0</option><option selected>o1</option></select>");
+    EXPECT_TRUE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{1}))
+        << "Onchange handler should not be executed.";
+  }
+
+  // 0 old selected options -> 1+ selected options
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option>o0</option><option>o1</option></select>");
+    EXPECT_FALSE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{0}))
+        << "Onchange handler should be executed.";
+  }
+
+  // 1+ old selected options -> more selected options
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option>o0</option><option selected>o1</option></select>");
+    EXPECT_FALSE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{0, 1}))
+        << "Onchange handler should be executed.";
+  }
+
+  // 1+ old selected options -> 0 selected options
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option>o0</option><option selected>o1</option></select>");
+    EXPECT_FALSE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{}))
+        << "Onchange handler should be executed.";
+  }
+
+  // Multiple old selected options -> less selected options
+  {
+    SetHtmlInnerHTML(
+        "<select multiple onchange='this.remove();'>"
+        "<option selected>o0</option><option selected>o1</option></select>");
+    EXPECT_FALSE(FirstSelectIsConnectedAfterSelectMultiple(Vector<int>{1}))
+        << "Onchange handler should be executed.";
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/core/layout/layout_theme.cc

@@ -855,6 +855,14 @@ Color LayoutTheme::FocusRingColor() const {
                                       : GetTheme().PlatformFocusRingColor();
 }
 
+bool LayoutTheme::DelegatesMenuListRendering() const {
+  return delegates_menu_list_rendering_;
+}
+
+void LayoutTheme::SetDelegatesMenuListRenderingForTesting(bool flag) {
+  delegates_menu_list_rendering_ = flag;
+}
+
 String LayoutTheme::DisplayNameForFile(const File& file) const {
   return file.name();
 }

third_party/blink/renderer/core/layout/layout_theme.h

@@ -232,7 +232,10 @@ class CORE_EXPORT LayoutTheme : public RefCounted<LayoutTheme> {
   virtual bool ShouldHaveSpinButton(HTMLInputElement*) const;
 
   // Functions for <select> elements.
-  virtual bool DelegatesMenuListRendering() const { return false; }
+  virtual bool DelegatesMenuListRendering() const;
+  // This function has no effect for LayoutThemeAndroid, of which
+  // DelegatesMenuListRendering() always returns true.
+  void SetDelegatesMenuListRenderingForTesting(bool flag);
   virtual bool PopsMenuByArrowKeys() const { return false; }
   virtual bool PopsMenuBySpaceKey() const { return false; }
   virtual bool PopsMenuByReturnKey() const { return false; }
@@ -369,6 +372,8 @@ class CORE_EXPORT LayoutTheme : public RefCounted<LayoutTheme> {
   base::TimeDelta caret_blink_interval_ =
       base::TimeDelta::FromMilliseconds(500);
 
+  bool delegates_menu_list_rendering_ = false;
+
   // This color is expected to be drawn on a semi-transparent overlay,
   // making it more transparent than its alpha value indicates.
   static const RGBA32 kDefaultTapHighlightColor = 0x66000000;