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 369722 5644a5462ce5ab400a81fd350f06b9ea2fc40db6
parent 369721 b55a8d9517c8a4cace18cd153e84ed0a0f0c5653
child 369723 66c6b9f68e500b51fbfdf31768757ce49e0bc365
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1052045
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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>