Bug 1365399 - Handle selectedness and dirtiness correctly in Option constructor. r=smaug
authorJessica Jong <jjong@mozilla.com>
Fri, 26 May 2017 01:59:00 -0400
changeset 409762 933ff638ea341d44700c4a89eda866c53c8ee2dd
parent 409761 b7f8c279b92268625473c916e9f34b307c924625
child 409763 5e7872cb3b5c2ae734d119ae75f3b1caa27ef63d
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1365399, 927796, 942648
milestone55.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 1365399 - Handle selectedness and dirtiness correctly in Option constructor. r=smaug This bug fixes this statement in the spec (https://html.spec.whatwg.org/#dom-option): If selected is true, then set option's selectedness to true; otherwise set its selectedness to false (even if defaultSelected is true). And also reset the dirtiness. This patch also reverts the changes in Bug 927796 and 942648, which was added because IsSelected() returned inaccurately (called from SelectSomething()). Verified that the tests added in Bug 927796 and 942648 do pass. MozReview-Commit-ID: 8PVgvCIHPj4
dom/html/HTMLOptionElement.cpp
dom/html/test/test_bug596511.html
testing/web-platform/meta/html/semantics/forms/the-option-element/option-element-constructor.html.ini
--- a/dom/html/HTMLOptionElement.cpp
+++ b/dom/html/HTMLOptionElement.cpp
@@ -146,21 +146,16 @@ HTMLOptionElement::Index()
   int32_t index = defaultIndex;
   MOZ_ALWAYS_SUCCEEDS(options->GetOptionIndex(this, 0, true, &index));
   return index;
 }
 
 bool
 HTMLOptionElement::Selected() const
 {
-  // If we haven't been explictly selected or deselected, use our default value
-  if (!mSelectedChanged) {
-    return DefaultSelected();
-  }
-
   return mIsSelected;
 }
 
 bool
 HTMLOptionElement::DefaultSelected() const
 {
   return HasAttr(kNameSpaceID_None, nsGkAtoms::selected);
 }
@@ -188,42 +183,35 @@ HTMLOptionElement::BeforeSetAttr(int32_t
                                                     aValue, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::selected ||
       mSelectedChanged) {
     return NS_OK;
   }
 
-  bool defaultSelected = aValue;
-  // First make sure we actually set our mIsSelected state to reflect our new
-  // defaultSelected state.  If that turns out to be wrong,
-  // SetOptionsSelectedByIndex will fix it up.  But otherwise we can end up in a
-  // situation where mIsSelected is still false, but mSelectedChanged becomes
-  // true (later in this method, when we compare mIsSelected to
-  // defaultSelected), and then we start returning false for Selected() even
-  // though we're actually selected.
-  mIsSelected = defaultSelected;
-
   // We just changed out selected state (since we look at the "selected"
   // attribute when mSelectedChanged is false).  Let's tell our select about
   // it.
   HTMLSelectElement* selectInt = GetSelect();
   if (!selectInt) {
+    // If option is a child of select, SetOptionsSelectedByIndex will set
+    // mIsSelected if needed.
+    mIsSelected = aValue;
     return NS_OK;
   }
 
   NS_ASSERTION(!mSelectedChanged, "Shouldn't be here");
 
   bool inSetDefaultSelected = mIsInSetDefaultSelected;
   mIsInSetDefaultSelected = true;
 
   int32_t index = Index();
   uint32_t mask = HTMLSelectElement::SET_DISABLED;
-  if (defaultSelected) {
+  if (aValue) {
     mask |= HTMLSelectElement::IS_SELECTED;
   }
 
   if (aNotify) {
     mask |= HTMLSelectElement::NOTIFY;
   }
 
   // This can end up calling SetSelectedInternal if our selected state needs to
@@ -231,18 +219,18 @@ HTMLOptionElement::BeforeSetAttr(int32_t
   // SetOptionsSelectedByIndex that might depend on it working don't get
   // confused.
   selectInt->SetOptionsSelectedByIndex(index, index, mask);
 
   // Now reset our members; when we finish the attr set we'll end up with the
   // rigt selected state.
   mIsInSetDefaultSelected = inSetDefaultSelected;
   // mIsSelected might have been changed by SetOptionsSelectedByIndex.  Possibly
-  // more than once; make sure our mSelectedChanged state is set correctly.
-  mSelectedChanged = mIsSelected != defaultSelected;
+  // more than once; make sure our mSelectedChanged state is set back correctly.
+  mSelectedChanged = false;
 
   return NS_OK;
 }
 
 nsresult
 HTMLOptionElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue, bool aNotify)
@@ -421,23 +409,23 @@ HTMLOptionElement::Option(const GlobalOb
     // aNotify == false.
     aError = option->SetAttr(kNameSpaceID_None, nsGkAtoms::selected,
                              EmptyString(), false);
     if (aError.Failed()) {
       return nullptr;
     }
   }
 
-  if (aSelected) {
-    option->SetSelected(true, aError);
-    if (aError.Failed()) {
-      return nullptr;
-    }
+  option->SetSelected(aSelected, aError);
+  if (aError.Failed()) {
+    return nullptr;
   }
 
+  option->SetSelectedChanged(false);
+
   return option.forget();
 }
 
 nsresult
 HTMLOptionElement::CopyInnerTo(Element* aDest, bool aPreallocateChildren)
 {
   nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest, aPreallocateChildren);
   NS_ENSURE_SUCCESS(rv, rv);
--- a/dom/html/test/test_bug596511.html
+++ b/dom/html/test/test_bug596511.html
@@ -181,17 +181,17 @@ function checkInvalidWhenValueMissing(el
    * 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, true);
+  checkSufferingFromBeingMissing(select);
   select.remove(0);
 
   select.add(new Option("", "", true, true), null);
   checkNotSufferingFromBeingMissing(select);
 
   select.add(new Option("foo", "foo"), null);
   select.remove(0);
   checkSufferingFromBeingMissing(select);
@@ -205,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, true);
+  checkSufferingFromBeingMissing(select);
 
   select.add(new Option("", "", true), null);
-  checkSufferingFromBeingMissing(select, true);
+  checkSufferingFromBeingMissing(select);
 
   select.add(new Option("foo"), null);
-  checkSufferingFromBeingMissing(select, true);
+  checkSufferingFromBeingMissing(select);
 
   select.options[2].selected = true;
   checkNotSufferingFromBeingMissing(select);
 }
 
 var select = document.createElement("select");
 var content = document.getElementById('content');
 content.appendChild(select);
deleted file mode 100644
--- a/testing/web-platform/meta/html/semantics/forms/the-option-element/option-element-constructor.html.ini
+++ /dev/null
@@ -1,11 +0,0 @@
-[option-element-constructor.html]
-  type: testharness
-  [Option constructor handles selectedness correctly when specified with defaultSelected only]
-    expected: FAIL
-
-  [Option constructor does not set dirtiness (so, manipulating the selected content attribute still updates the selected IDL attribute)]
-    expected: FAIL
-
-  [Option constructor handles selectedness correctly, even when incongruous with defaultSelected]
-    expected: FAIL
-