Bug 1052045 - Fix <select> validity status for listboxes and for non-placeholder empty valued options. r=bzbarsky

--HG--
rename : layout/reftests/css-invalid/select/select-required-multiple-invalid.html => layout/reftests/css-invalid/select/select-required-multiple-still-valid.html
rename : layout/reftests/css-valid/select/select-required-multiple-invalid.html => layout/reftests/css-valid/select/select-required-multiple-still-valid.html

dom/html/HTMLSelectElement.cpp

@@ -57,8 +57,11 @@ SafeOptionListMutation::SafeOptionListMutation(nsIContent* aSelect,
   : mSelect(HTMLSelectElement::FromContentOrNull(aSelect))
   , mTopLevelMutation(false)
   , mNeedsRebuild(false)
+  , mNotify(aNotify)
+  , mInitialSelectedIndex(-1)
 {
   if (mSelect) {
+    mInitialSelectedIndex = mSelect->SelectedIndex();
     mTopLevelMutation = !mSelect->mMutating;
     if (mTopLevelMutation) {
       mSelect->mMutating = true;
@@ -67,13 +70,13 @@ SafeOptionListMutation::SafeOptionListMutation(nsIContent* aSelect,
       // option list must be up-to-date before inserting or removing options.
       // Fortunately this is called only if mutation event listener
       // adds or removes options.
-      mSelect->RebuildOptionsArray(aNotify);
+      mSelect->RebuildOptionsArray(mNotify);
     }
     nsresult rv;
     if (aKid) {
-      rv = mSelect->WillAddOptions(aKid, aParent, aIndex, aNotify);
+      rv = mSelect->WillAddOptions(aKid, aParent, aIndex, mNotify);
     } else {
-      rv = mSelect->WillRemoveOptions(aParent, aIndex, aNotify);
+      rv = mSelect->WillRemoveOptions(aParent, aIndex, mNotify);
     }
     mNeedsRebuild = NS_FAILED(rv);
   }
@@ -88,6 +91,16 @@ SafeOptionListMutation::~SafeOptionListMutation()
     if (mTopLevelMutation) {
       mSelect->mMutating = false;
     }
+    if (mSelect->SelectedIndex() != mInitialSelectedIndex) {
+      // We must have triggered the SelectSomething() codepath, which can cause
+      // our validity to change.  Unfortunately, our attempt to update validity
+      // in that case may not have worked correctly, because we actually call it
+      // before we have inserted the new <option>s into the DOM!  Go ahead and
+      // update validity here as needed, because by now we know our <option>s
+      // are where they should be.
+      mSelect->UpdateValueMissingValidityState();
+      mSelect->UpdateState(mNotify);
+    }
 #ifdef DEBUG
     mSelect->VerifyOptionsArray();
 #endif
@@ -1774,6 +1787,15 @@ HTMLSelectElement::IsValueMissing()
 
   for (uint32_t i = 0; i < length; ++i) {
     RefPtr<HTMLOptionElement> option = Item(i);
+    // Check for a placeholder label option, don't count it as a valid value.
+    if (i == 0 && !Multiple() && Size() <= 1 && option->GetParent() == this) {
+      nsAutoString value;
+      MOZ_ALWAYS_SUCCEEDS(option->GetValue(value));
+      if (value.IsEmpty()) {
+        continue;
+      }
+    }
+
     if (!option->Selected()) {
       continue;
     }
@@ -1782,11 +1804,7 @@ HTMLSelectElement::IsValueMissing()
       continue;
     }
 
-    nsAutoString value;
-    MOZ_ALWAYS_SUCCEEDS(option->GetValue(value));
-    if (!value.IsEmpty()) {
-      return false;
-    }
+    return false;
   }
 
   return true;

dom/html/HTMLSelectElement.h

@@ -106,6 +106,11 @@ private:
   bool                       mTopLevelMutation;
   /** true if it is known that the option list must be recreated. */
   bool                       mNeedsRebuild;
+  /** Whether we should be notifying when we make various method calls on
+      mSelect */
+  const bool                 mNotify;
+  /** The selected index at mutation start. */
+  int32_t                    mInitialSelectedIndex;
   /** Option list must be recreated if more than one mutation is detected. */
   nsMutationGuard            mGuard;
 };

dom/html/test/test_bug596511.html

@@ -143,16 +143,18 @@ function checkInvalidWhenValueMissing(element)
   select.add(new Option(), null);
   checkNotSufferingFromBeingMissing(select);
 
+  // The placeholder label can only be the first option, so a selected empty second option is valid
   select.options[1].selected = true;
-  checkSufferingFromBeingMissing(select);
+  checkNotSufferingFromBeingMissing(select);
 
   select.selectedIndex = 0;
   checkNotSufferingFromBeingMissing(select);
 
-  select.selectedIndex = 1;
+  select.add(select.options[0]);
+  select.selectedIndex = 0;
   checkSufferingFromBeingMissing(select);
 
-  select.remove(1);
+  select.remove(0);
   checkNotSufferingFromBeingMissing(select);
 
   select.options[0].disabled = true;
@@ -182,8 +184,13 @@ function checkInvalidWhenValueMissing(element)
   select.size = 4;
   checkSufferingFromBeingMissing(select);
 
+  // Setting defaultSelected to true should not make the option selected
   select.add(new Option("", "", true), null);
-  checkSufferingFromBeingMissing(select);
+  checkSufferingFromBeingMissing(select, true);
+  select.remove(0);
+
+  select.add(new Option("", "", true, true), null);
+  checkNotSufferingFromBeingMissing(select);
 
   select.add(new Option("foo", "foo"), null);
   select.remove(0);
@@ -203,13 +210,13 @@ function checkInvalidWhenValueMissing(element)
   checkSufferingFromBeingMissing(select);
 
   select.add(new Option("", "", true), null);
-  checkSufferingFromBeingMissing(select);
+  checkSufferingFromBeingMissing(select, true);
 
   select.add(new Option("", "", true), null);
-  checkSufferingFromBeingMissing(select);
+  checkSufferingFromBeingMissing(select, true);
 
   select.add(new Option("foo"), null);
-  checkSufferingFromBeingMissing(select);
+  checkSufferingFromBeingMissing(select, true);
 
   select.options[2].selected = true;
   checkNotSufferingFromBeingMissing(select);

dom/html/test/test_bug605124-2.html

@@ -88,17 +88,17 @@ function checkSelectElement(aElement)
   checkPseudoClass(aElement, false);
 
   aElement.blur();
-  checkPseudoClass(aElement, true);
+  checkPseudoClass(aElement, !aElement.multiple);
 
   // Focusing while :-moz-ui-invalid applies,
   // the pseudo-class should apply while changing selection if appropriate.
   aElement.focus();
-  checkPseudoClass(aElement, true);
+  checkPseudoClass(aElement, !aElement.multiple);
 
   aElement.selectedIndex = 1;
   checkPseudoClass(aElement, false);
   aElement.selectedIndex = 0;
-  checkPseudoClass(aElement, true);
+  checkPseudoClass(aElement, !aElement.multiple);
 }
 
 checkElement(document.getElementsByTagName('input')[0]);

dom/html/test/test_bug605125-2.html

@@ -121,6 +121,10 @@ function checkSelectElement(aElement)
 
   aElement.blur();
   aElement.required = true;
+  // select set with multiple is only invalid if no option is selected
+  if (aElement.multiple) {
+    aElement.selectedIndex = -1;
+  }
   checkPseudoClass(aElement, false);
 
   // Focusing while :-moz-ui-invalid applies,
@@ -131,7 +135,7 @@ function checkSelectElement(aElement)
   aElement.selectedIndex = 1;
   checkPseudoClass(aElement, true);
   aElement.selectedIndex = 0;
-  checkPseudoClass(aElement, false);
+  checkPseudoClass(aElement, aElement.multiple);
 }
 
 checkElement(document.getElementsByTagName('input')[0]);

dom/html/test/test_bug612730.html

@@ -34,10 +34,10 @@ function runTest()
 
     isnot(select.selectedIndex, -1, "Something should have been selected");
 
-    ok(!select.matches(":-moz-ui-valid"),
-       ":-moz-ui-valid should not apply");
-    todo(!select.matches(":-moz-ui-invalid"),
-         ":-moz-ui-invalid should not apply");
+    ok(!select.matches(":-moz-ui-invalid"),
+       ":-moz-ui-invalid should not apply");
+    todo(!select.matches(":-moz-ui-valid"),
+         ":-moz-ui-valid should not apply");
 
     SimpleTest.finish();
   }, false);

dom/html/test/test_bug622597.html

@@ -16,7 +16,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=622597
   <form>
     <input required>
     <textarea required></textarea>
-    <select required><option>foo</option><option value="">bar</option></select>
+    <select required><option value="">foo</option><option selected>bar</option></select>
     <button>submit</button>
   </form>
 </div>
@@ -65,7 +65,7 @@ textarea.addEventListener("focus", function() {
     SimpleTest.executeSoon(function() {
       checkPseudoClasses(textarea, false, true);
       form.noValidate = true;
-      select.selectedIndex = 1;
+      select.selectedIndex = 0;
       select.focus();
     });
   });

layout/reftests/css-invalid/select/reftest.list

@@ -5,7 +5,7 @@ fuzzy-if(skiaContent,2,6) needs-focus == select-dyn-disabled.html select-disable
 fuzzy-if(skiaContent,1,3) needs-focus == select-dyn-not-disabled.html select-ref.html
 needs-focus == select-required-invalid.html select-required-ref.html
 needs-focus == select-required-valid.html select-required-ref.html
-needs-focus == select-required-multiple-invalid.html select-required-multiple-ref.html
+needs-focus == select-required-multiple-still-valid.html select-required-multiple-ref.html
 fuzzy-if(skiaContent,1,250) needs-focus == select-required-multiple-valid.html select-required-multiple-ref.html
 fails-if(Android) fuzzy-if(skiaContent&&!Android,1,3) needs-focus == select-disabled-fieldset-1.html select-fieldset-ref.html
 fails-if(Android) fuzzy-if(skiaContent&&!Android,2,3) needs-focus == select-disabled-fieldset-2.html select-fieldset-ref.html

layout/reftests/css-invalid/select/select-required-multiple-still-valid.html

@@ -1,10 +1,10 @@
 <!DOCTYPE html>
 <html>
-  <!-- Test: if select is required and has all selected option value set to the
-             string string, :valid should not apply. -->
+  <!-- Test: if select is required and has all selected option values set to the
+             empty string, :invalid should still not apply. -->
   <link rel='stylesheet' type='text/css' href='style.css'>
   <body>
-    <select class='notvalid' required multiple>
+    <select class='notinvalid' required multiple>
       <option selected></option>
       <option selected value="">foo</option>
     </select>

layout/reftests/css-valid/select/reftest.list

@@ -5,7 +5,7 @@ fuzzy-if(skiaContent,1,5) needs-focus == select-dyn-disabled.html select-disable
 fuzzy-if(skiaContent,2,5) needs-focus == select-dyn-not-disabled.html select-ref.html
 needs-focus == select-required-invalid.html select-required-ref.html
 needs-focus == select-required-valid.html select-required-ref.html
-needs-focus == select-required-multiple-invalid.html select-required-multiple-ref.html
+needs-focus == select-required-multiple-still-valid.html select-required-multiple-ref.html
 fuzzy-if(skiaContent,1,250) needs-focus == select-required-multiple-valid.html select-required-multiple-ref.html
 fails-if(Android) needs-focus == select-disabled-fieldset-1.html select-fieldset-ref.html
 fails-if(Android) fuzzy-if(skiaContent&&!Android,1,3) needs-focus == select-disabled-fieldset-2.html select-fieldset-ref.html

layout/reftests/css-valid/select/select-required-multiple-still-valid.html

@@ -1,10 +1,10 @@
 <!DOCTYPE html>
 <html>
   <!-- Test: if select is required and has all selected option value set to the
-             string string, :invalid should apply. -->
+             empty string, :valid should still apply. -->
   <link rel='stylesheet' type='text/css' href='style.css'>
   <body>
-    <select class='invalid' required multiple>
+    <select class='valid' required multiple>
       <option selected></option>
       <option selected value="">foo</option>
     </select>

testing/web-platform/meta/MANIFEST.json

@@ -39540,6 +39540,12 @@
             "url": "/html/semantics/embedded-content/the-img-element/update-src-complete.html"
           }
         ],
+        "html/semantics/forms/the-select-element/select-validity.html": [
+          {
+            "path": "html/semantics/forms/the-select-element/select-validity.html",
+            "url": "/html/semantics/forms/the-select-element/select-validity.html"
+          }
+        ],
         "uievents/order-of-events/focus-events/focus-automated-blink-webkit.html": [
           {
             "path": "uievents/order-of-events/focus-events/focus-automated-blink-webkit.html",

testing/web-platform/tests/html/semantics/forms/the-select-element/select-validity.html

@@ -0,0 +1,95 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>HTMLSelectElement.checkValidity</title>
+<link rel="help" href="https://html.spec.whatwg.org/multipage/forms.html#the-select-element:attr-select-required-4">
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<div id=log></div>
+<script>
+
+test(function() {
+  var select = document.createElement('select');
+  assert_true(select.willValidate, "A select element is a submittable element that is a candidate for constraint validation.");
+  var placeholder = document.createElement('option');
+  select.appendChild(placeholder);
+  assert_true(select.checkValidity(), "Always valid when the select isn't a required value.");
+  select.required = true;
+  assert_true(placeholder.selected, "If display size is 1, multiple is absent and no options have selectedness true, the first option is selected.");
+  assert_equals(select.value, "", "The placeholder's value should be the select's value right now");
+  assert_false(select.checkValidity(), "A selected placeholder option should invalidate the select.");
+  var emptyOption = document.createElement('option');
+  select.appendChild(emptyOption);
+  emptyOption.selected = true;
+  assert_equals(select.value, "", "The empty value should be set.");
+  assert_true(select.checkValidity(), "An empty non-placeholder option should be a valid choice.");
+  var filledOption = document.createElement('option');
+  filledOption.value = "test";
+  select.appendChild(filledOption);
+  filledOption.selected = true;
+  assert_equals(select.value, "test", "The non-empty value should be set.");
+  assert_true(select.checkValidity(), "A non-empty non-placeholder option should be a valid choice.");
+  select.removeChild(placeholder);
+  select.appendChild(emptyOption); // move emptyOption to second place
+  emptyOption.selected = true;
+  assert_equals(select.value, "", "The empty value should be set.");
+  assert_true(select.checkValidity(), "Only the first option can be seen as a placeholder.");
+  placeholder.disabled = true;
+  select.insertBefore(placeholder, filledOption);
+  placeholder.selected = true;
+  assert_equals(select.value, "", "A disabled first placeholder option should result in an empty value.");
+  assert_false(select.checkValidity(), "A disabled first placeholder option should invalidate the select.");
+}, "Placeholder label options within a select");
+
+test(function() {
+  var select = document.createElement('select');
+  select.required = true;
+  var optgroup = document.createElement('optgroup');
+  var emptyOption = document.createElement('option');
+  optgroup.appendChild(emptyOption);
+  select.appendChild(optgroup);
+  emptyOption.selected = true;
+  assert_equals(select.value, "", "The empty value should be set.");
+  assert_true(select.checkValidity(), "The first option is not considered a placeholder if it is located within an optgroup.");
+  var otherEmptyOption = document.createElement('option');
+  otherEmptyOption.value = "";
+  select.appendChild(otherEmptyOption);
+  otherEmptyOption.selected = true;
+  assert_equals(select.value, "", "The empty value should be set.");
+  assert_true(select.checkValidity(), "The empty option should be accepted as it is not the first option in the tree ordered list.");
+}, "Placeholder label-like options within optgroup");
+
+test(function() {
+  var select = document.createElement('select');
+  select.required = true;
+  select.size = 2;
+  var emptyOption = document.createElement('option');
+  select.appendChild(emptyOption);
+  assert_false(emptyOption.selected, "Display size is not 1, so the first option should not be selected.");
+  assert_false(select.checkValidity(), "If no options are selected the select must be seen as invalid.");
+  emptyOption.selected = true;
+  assert_true(select.checkValidity(), "If one option is selected, the select should be considered valid.");
+  var otherEmptyOption = document.createElement('option');
+  otherEmptyOption.value = "";
+  select.appendChild(otherEmptyOption);
+  otherEmptyOption.selected = true;
+  assert_false(emptyOption.selected, "Whenever an option has its selectiveness set to true, the other options must be set to false.");
+  otherEmptyOption.selected = false;
+  assert_false(otherEmptyOption.selected, "It should be possible to set the selectiveness to false with a display size more than one.");
+  assert_false(select.checkValidity(), "If no options are selected the select must be seen as invalid.");
+}, "Validation on selects with display size set as more than one");
+
+test(function() {
+  var select = document.createElement('select');
+  select.required = true;
+  select.multiple = true;
+  var emptyOption = document.createElement('option');
+  select.appendChild(emptyOption);
+  assert_false(select.checkValidity(), "If no options are selected the select must be seen as invalid.");
+  emptyOption.selected = true;
+  assert_true(select.checkValidity(), "If one option is selected, the select should be considered valid.");
+  var optgroup = document.createElement('optgroup');
+  optgroup.appendChild(emptyOption); // Move option to optgroup
+  select.appendChild(optgroup);
+  assert_true(select.checkValidity(), "If one option within an optgroup or not is selected, the select should be considered valid.");
+}, "Validation on selects with multiple set");
+</script>