Bug 1052045 - Fix <select> validity status for listboxes and for non-placeholder empty valued options. r=bzbarsky
authorTom Puttemans <mozilla@tomputtemans.com>
Thu, 24 Nov 2016 22:15:33 +0100
changeset 324764 5644a5462ce5ab400a81fd350f06b9ea2fc40db6
parent 324763 b55a8d9517c8a4cace18cd153e84ed0a0f0c5653
child 324765 66c6b9f68e500b51fbfdf31768757ce49e0bc365
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersbzbarsky
bugs1052045
milestone53.0a1
Bug 1052045 - Fix <select> validity status for listboxes and for non-placeholder empty valued options. r=bzbarsky
dom/html/HTMLSelectElement.cpp
dom/html/HTMLSelectElement.h
dom/html/test/test_bug596511.html
dom/html/test/test_bug605124-2.html
dom/html/test/test_bug605125-2.html
dom/html/test/test_bug612730.html
dom/html/test/test_bug622597.html
layout/reftests/css-invalid/select/reftest.list
layout/reftests/css-invalid/select/select-required-multiple-invalid.html
layout/reftests/css-invalid/select/select-required-multiple-still-valid.html
layout/reftests/css-valid/select/reftest.list
layout/reftests/css-valid/select/select-required-multiple-invalid.html
layout/reftests/css-valid/select/select-required-multiple-still-valid.html
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/html/semantics/forms/the-select-element/select-validity.html
--- a/dom/html/HTMLSelectElement.cpp
+++ b/dom/html/HTMLSelectElement.cpp
@@ -52,47 +52,60 @@ NS_IMPL_ISUPPORTS(SelectState, SelectSta
 SafeOptionListMutation::SafeOptionListMutation(nsIContent* aSelect,
                                                nsIContent* aParent,
                                                nsIContent* aKid,
                                                uint32_t aIndex,
                                                bool aNotify)
   : 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;
     } else {
       // This is very unfortunate, but to handle mutation events properly,
       // 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);
   }
 }
 
 SafeOptionListMutation::~SafeOptionListMutation()
 {
   if (mSelect) {
     if (mNeedsRebuild || (mTopLevelMutation && mGuard.Mutated(1))) {
       mSelect->RebuildOptionsArray(true);
     }
     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
   }
 }
 
 //----------------------------------------------------------------------
 //
@@ -1769,29 +1782,34 @@ HTMLSelectElement::IsValueMissing()
   if (!Required()) {
     return false;
   }
 
   uint32_t length = Length();
 
   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;
     }
 
     if (IsOptionDisabled(option)) {
       continue;
     }
 
-    nsAutoString value;
-    MOZ_ALWAYS_SUCCEEDS(option->GetValue(value));
-    if (!value.IsEmpty()) {
-      return false;
-    }
+    return false;
   }
 
   return true;
 }
 
 void
 HTMLSelectElement::UpdateValueMissingValidityState()
 {
--- a/dom/html/HTMLSelectElement.h
+++ b/dom/html/HTMLSelectElement.h
@@ -101,16 +101,21 @@ private:
   static void* operator new(size_t) CPP_THROW_NEW { return 0; }
   static void operator delete(void*, size_t) {}
   /** The select element which option list is being mutated. */
   RefPtr<HTMLSelectElement> mSelect;
   /** true if the current mutation is the first one in the stack. */
   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;
 };
 
 
 /**
  * Implementation of &lt;select&gt;
  */
--- a/dom/html/test/test_bug596511.html
+++ b/dom/html/test/test_bug596511.html
@@ -138,26 +138,28 @@ function checkInvalidWhenValueMissing(el
   checkSufferingFromBeingMissing(select);
 
   select.add(new Option("foo", "foo"), null);
   checkNotSufferingFromBeingMissing(select);
 
   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;
   // TODO: having that working would require us to add a mutation observer on
   // the select element.
   checkSufferingFromBeingMissing(select, true);
 
   select.options[0].disabled = false
@@ -177,18 +179,23 @@ function checkInvalidWhenValueMissing(el
   /**
    * Non-multiple and size > 1.
    * Everything should be the same except moving the selection.
    */
   select.multiple = false;
   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);
   checkSufferingFromBeingMissing(select);
 
   select.options[0].selected = true;
   checkNotSufferingFromBeingMissing(select);
 
@@ -198,23 +205,23 @@ function checkInvalidWhenValueMissing(el
    * Multiple, any size.
    * We can select more than one element and at least needs a value.
    */
   select.multiple = true;
   select.size = 4;
   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);
 }
 
 var select = document.createElement("select");
 var content = document.getElementById('content');
 content.appendChild(select);
--- a/dom/html/test/test_bug605124-2.html
+++ b/dom/html/test/test_bug605124-2.html
@@ -83,27 +83,27 @@ function checkSelectElement(aElement)
   checkPseudoClass(aElement, false);
 
   aElement.selectedIndex = 1;
   checkPseudoClass(aElement, false);
   aElement.selectedIndex = 0;
   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]);
 checkElement(document.getElementsByTagName('textarea')[0]);
 checkSelectElement(document.getElementsByTagName('select')[0]);
 checkSelectElement(document.getElementsByTagName('select')[1]);
 
 </script>
--- a/dom/html/test/test_bug605125-2.html
+++ b/dom/html/test/test_bug605125-2.html
@@ -116,27 +116,31 @@ function checkSelectElement(aElement)
 
   aElement.selectedIndex = 1;
   checkPseudoClass(aElement, true);
   aElement.selectedIndex = 0;
   checkPseudoClass(aElement, true);
 
   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,
   // the pseudo-class should apply while changing selection if appropriate.
   aElement.focus();
   checkPseudoClass(aElement, false);
 
   aElement.selectedIndex = 1;
   checkPseudoClass(aElement, true);
   aElement.selectedIndex = 0;
-  checkPseudoClass(aElement, false);
+  checkPseudoClass(aElement, aElement.multiple);
 }
 
 checkElement(document.getElementsByTagName('input')[0]);
 checkElement(document.getElementsByTagName('textarea')[0]);
 checkSelectElement(document.getElementsByTagName('select')[0]);
 checkSelectElement(document.getElementsByTagName('select')[1]);
 
 </script>
--- a/dom/html/test/test_bug612730.html
+++ b/dom/html/test/test_bug612730.html
@@ -29,20 +29,20 @@ function runTest()
 {
   var select = document.getElementsByTagName('select')[0];
 
   select.addEventListener("focus", function() {
     select.removeEventListener("focus", arguments.callee, false);
 
     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);
 
   synthesizeMouse(select, 5, 5, {});
 }
 
 SimpleTest.waitForFocus(runTest);
--- a/dom/html/test/test_bug622597.html
+++ b/dom/html/test/test_bug622597.html
@@ -11,17 +11,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=622597">Mozilla Bug 622597</a>
 <p id="display"></p>
 <div id="content">
   <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>
 <pre id="test">
 <script type="application/javascript">
 
 /** Test for Bug 622597 **/
 
@@ -60,17 +60,17 @@ select.addEventListener("focus", functio
 textarea.addEventListener("focus", function() {
   textarea.removeEventListener("focus", arguments.callee, false);
 
   SimpleTest.executeSoon(function() {
     form.noValidate = false;
     SimpleTest.executeSoon(function() {
       checkPseudoClasses(textarea, false, true);
       form.noValidate = true;
-      select.selectedIndex = 1;
+      select.selectedIndex = 0;
       select.focus();
     });
   });
 }, false);
 
 input.addEventListener("invalid", function() {
   input.removeEventListener("invalid", arguments.callee, false);
 
--- a/layout/reftests/css-invalid/select/reftest.list
+++ b/layout/reftests/css-invalid/select/reftest.list
@@ -1,12 +1,12 @@
 needs-focus == select-valid.html select-ref.html
 fuzzy-if(skiaContent,1,3) needs-focus == select-invalid.html select-ref.html
 fuzzy-if(skiaContent,2,6) needs-focus == select-disabled.html select-disabled-ref.html
 fuzzy-if(skiaContent,2,6) needs-focus == select-dyn-disabled.html select-disabled-ref.html
 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
 fuzzy-if(skiaContent,2,5) needs-focus == select-fieldset-legend.html select-fieldset-legend-ref.html
rename from layout/reftests/css-invalid/select/select-required-multiple-invalid.html
rename to layout/reftests/css-invalid/select/select-required-multiple-still-valid.html
--- a/layout/reftests/css-invalid/select/select-required-multiple-invalid.html
+++ b/layout/reftests/css-invalid/select/select-required-multiple-still-valid.html
@@ -1,12 +1,12 @@
 <!DOCTYPE html>
 <html>
-  <!-- Test: if select is required and has all selected option value set to the
-             string string, :invalid should 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='invalid' required multiple>
+    <select class='notinvalid' required multiple>
       <option selected></option>
       <option selected value="">foo</option>
     </select>
   </body>
 </html>
--- a/layout/reftests/css-valid/select/reftest.list
+++ b/layout/reftests/css-valid/select/reftest.list
@@ -1,12 +1,12 @@
 fuzzy-if(skiaContent,2,7) needs-focus == select-valid.html select-ref.html
 fuzzy-if(skiaContent,2,5) needs-focus == select-invalid.html select-ref.html
 needs-focus == select-disabled.html select-disabled-ref.html
 fuzzy-if(skiaContent,1,5) needs-focus == select-dyn-disabled.html select-disabled-ref.html
 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
 needs-focus == select-fieldset-legend.html select-fieldset-legend-ref.html
rename from layout/reftests/css-valid/select/select-required-multiple-invalid.html
rename to layout/reftests/css-valid/select/select-required-multiple-still-valid.html
--- a/layout/reftests/css-valid/select/select-required-multiple-invalid.html
+++ b/layout/reftests/css-valid/select/select-required-multiple-still-valid.html
@@ -1,12 +1,12 @@
 <!DOCTYPE html>
 <html>
   <!-- Test: if select is required and has all selected option value set to the
-             string string, :valid should not apply. -->
+             empty string, :valid should still apply. -->
   <link rel='stylesheet' type='text/css' href='style.css'>
   <body>
-    <select class='notvalid' required multiple>
+    <select class='valid' required multiple>
       <option selected></option>
       <option selected value="">foo</option>
     </select>
   </body>
 </html>
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -39535,16 +39535,22 @@
           }
         ],
         "html/semantics/embedded-content/the-img-element/update-src-complete.html": [
           {
             "path": "html/semantics/embedded-content/the-img-element/update-src-complete.html",
             "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",
             "url": "/uievents/order-of-events/focus-events/focus-automated-blink-webkit.html"
           }
         ],
         "web-animations/animation-model/animation-types/spacing-keyframes-filters.html": [
           {
new file mode 100644
--- /dev/null
+++ b/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>