Bug 1214164 - Don't honor all <option> descendants of <select>; r=bz
authorAryeh Gregor <ayg@aryeh.name>
Tue, 13 Oct 2015 11:28:00 +0200
changeset 267523 10739e1e3d8c93b97219531d5ed49015383d23fd
parent 267522 1dfe9719da6c19b9410d0fe687f9d0c99c8e0974
child 267524 47725c1ad2b55c00e1f547c7e96c5acb60dba39b
push id66521
push usercbook@mozilla.com
push dateWed, 14 Oct 2015 07:31:53 +0000
treeherdermozilla-inbound@47725c1ad2b5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1214164
milestone44.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 1214164 - Don't honor all <option> descendants of <select>; r=bz Per spec, we should only pay attention to options that are children of the select, or of an optgroup child of the select.
dom/html/HTMLOptGroupElement.cpp
dom/html/HTMLOptGroupElement.h
dom/html/HTMLSelectElement.cpp
dom/html/HTMLSelectElement.h
testing/web-platform/meta/html/infrastructure/common-dom-interfaces/collections/htmloptionscollection.html.ini
--- a/dom/html/HTMLOptGroupElement.cpp
+++ b/dom/html/HTMLOptGroupElement.cpp
@@ -64,30 +64,24 @@ HTMLOptGroupElement::PreHandleEvent(Even
         uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED) {
       return NS_OK;
     }
   }
 
   return nsGenericHTMLElement::PreHandleEvent(aVisitor);
 }
 
-nsIContent*
+Element*
 HTMLOptGroupElement::GetSelect()
 {
-  nsIContent* parent = this;
-  while ((parent = parent->GetParent()) && parent->IsHTMLElement()) {
-    if (parent->IsHTMLElement(nsGkAtoms::select)) {
-      return parent;
-    }
-    if (!parent->IsHTMLElement(nsGkAtoms::optgroup)) {
-      break;
-    }
+  Element* parent = nsINode::GetParentElement();
+  if (!parent || !parent->IsHTMLElement(nsGkAtoms::select)) {
+    return nullptr;
   }
-  
-  return nullptr;
+  return parent;
 }
 
 nsresult
 HTMLOptGroupElement::InsertChildAt(nsIContent* aKid,
                                    uint32_t aIndex,
                                    bool aNotify)
 {
   SafeOptionListMutation safeMutation(GetSelect(), this, aKid, aIndex, aNotify);
--- a/dom/html/HTMLOptGroupElement.h
+++ b/dom/html/HTMLOptGroupElement.h
@@ -71,15 +71,15 @@ protected:
   virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 
 protected:
 
   /**
    * Get the select content element that contains this option
    * @param aSelectElement the select element [OUT]
    */
-  nsIContent* GetSelect();
+  Element* GetSelect();
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif /* mozilla_dom_HTMLOptGroupElement_h */
--- a/dom/html/HTMLSelectElement.cpp
+++ b/dom/html/HTMLSelectElement.cpp
@@ -221,17 +221,43 @@ HTMLSelectElement::RemoveChildAt(uint32_
 
 void
 HTMLSelectElement::InsertOptionsIntoList(nsIContent* aOptions,
                                          int32_t aListIndex,
                                          int32_t aDepth,
                                          bool aNotify)
 {
   int32_t insertIndex = aListIndex;
-  InsertOptionsIntoListRecurse(aOptions, &insertIndex, aDepth);
+
+  HTMLOptionElement* optElement = HTMLOptionElement::FromContent(aOptions);
+  if (optElement) {
+    mOptions->InsertOptionAt(optElement, insertIndex);
+    insertIndex++;
+  } else {
+    // If it's at the top level, then we just found out there are non-options
+    // at the top level, which will throw off the insert count
+    if (aDepth == 0) {
+      mNonOptionChildren++;
+    }
+
+    // Deal with optgroups
+    if (aOptions->IsHTMLElement(nsGkAtoms::optgroup)) {
+      mOptGroupCount++;
+
+      for (nsIContent* child = aOptions->GetFirstChild();
+           child;
+           child = child->GetNextSibling()) {
+        optElement = HTMLOptionElement::FromContent(child);
+        if (optElement) {
+          mOptions->InsertOptionAt(optElement, insertIndex);
+          insertIndex++;
+        }
+      }
+    }
+  }
 
   // Deal with the selected list
   if (insertIndex - aListIndex) {
     // Fix the currently selected index
     if (aListIndex <= mSelectedIndex) {
       mSelectedIndex += (insertIndex - aListIndex);
       SetSelectionChanged(true, aNotify);
     }
@@ -277,19 +303,50 @@ HTMLSelectElement::InsertOptionsIntoList
 
 nsresult
 HTMLSelectElement::RemoveOptionsFromList(nsIContent* aOptions,
                                          int32_t aListIndex,
                                          int32_t aDepth,
                                          bool aNotify)
 {
   int32_t numRemoved = 0;
-  nsresult rv = RemoveOptionsFromListRecurse(aOptions, aListIndex, &numRemoved,
-                                             aDepth);
-  NS_ENSURE_SUCCESS(rv, rv);
+
+  HTMLOptionElement* optElement = HTMLOptionElement::FromContent(aOptions);
+  if (optElement) {
+    if (mOptions->ItemAsOption(aListIndex) != optElement) {
+      NS_ERROR("wrong option at index");
+      return NS_ERROR_UNEXPECTED;
+    }
+    mOptions->RemoveOptionAt(aListIndex);
+    numRemoved++;
+  } else {
+    // Yay, one less artifact at the top level.
+    if (aDepth == 0) {
+      mNonOptionChildren--;
+    }
+
+    // Recurse down deeper for options
+    if (mOptGroupCount && aOptions->IsHTMLElement(nsGkAtoms::optgroup)) {
+      mOptGroupCount--;
+
+      for (nsIContent* child = aOptions->GetFirstChild();
+          child;
+          child = child->GetNextSibling()) {
+        optElement = HTMLOptionElement::FromContent(child);
+        if (optElement) {
+          if (mOptions->ItemAsOption(aListIndex) != optElement) {
+            NS_ERROR("wrong option at index");
+            return NS_ERROR_UNEXPECTED;
+          }
+          mOptions->RemoveOptionAt(aListIndex);
+          numRemoved++;
+        }
+      }
+    }
+  }
 
   if (numRemoved) {
     // Tell the widget we removed the options
     nsISelectControlFrame* selectFrame = GetSelectFrame();
     if (selectFrame) {
       nsAutoScriptBlocker scriptBlocker;
       for (int32_t i = aListIndex; i < aListIndex + numRemoved; ++i) {
         selectFrame->RemoveOption(i);
@@ -319,116 +376,31 @@ HTMLSelectElement::RemoveOptionsFromList
 
       UpdateState(aNotify);
     }
   }
 
   return NS_OK;
 }
 
-// If the document is such that recursing over these options gets us
-// deeper than four levels, there is something terribly wrong with the
-// world.
-void
-HTMLSelectElement::InsertOptionsIntoListRecurse(nsIContent* aOptions,
-                                                int32_t* aInsertIndex,
-                                                int32_t aDepth)
-{
-  // We *assume* here that someone's brain has not gone horribly
-  // wrong by putting <option> inside of <option>.  I'm sorry, I'm
-  // just not going to look for an option inside of an option.
-  // Sue me.
-
-  HTMLOptionElement* optElement = HTMLOptionElement::FromContent(aOptions);
-  if (optElement) {
-    mOptions->InsertOptionAt(optElement, *aInsertIndex);
-    (*aInsertIndex)++;
-    return;
-  }
-
-  // If it's at the top level, then we just found out there are non-options
-  // at the top level, which will throw off the insert count
-  if (aDepth == 0) {
-    mNonOptionChildren++;
-  }
-
-  // Recurse down into optgroups
-  if (aOptions->IsHTMLElement(nsGkAtoms::optgroup)) {
-    mOptGroupCount++;
-
-    for (nsIContent* child = aOptions->GetFirstChild();
-         child;
-         child = child->GetNextSibling()) {
-      InsertOptionsIntoListRecurse(child, aInsertIndex, aDepth + 1);
-    }
-  }
-}
-
-// If the document is such that recursing over these options gets us deeper than
-// four levels, there is something terribly wrong with the world.
-nsresult
-HTMLSelectElement::RemoveOptionsFromListRecurse(nsIContent* aOptions,
-                                                int32_t aRemoveIndex,
-                                                int32_t* aNumRemoved,
-                                                int32_t aDepth)
-{
-  // We *assume* here that someone's brain has not gone horribly
-  // wrong by putting <option> inside of <option>.  I'm sorry, I'm
-  // just not going to look for an option inside of an option.
-  // Sue me.
-
-  nsCOMPtr<nsIDOMHTMLOptionElement> optElement(do_QueryInterface(aOptions));
-  if (optElement) {
-    if (mOptions->ItemAsOption(aRemoveIndex) != optElement) {
-      NS_ERROR("wrong option at index");
-      return NS_ERROR_UNEXPECTED;
-    }
-    mOptions->RemoveOptionAt(aRemoveIndex);
-    (*aNumRemoved)++;
-    return NS_OK;
-  }
-
-  // Yay, one less artifact at the top level.
-  if (aDepth == 0) {
-    mNonOptionChildren--;
-  }
-
-  // Recurse down deeper for options
-  if (mOptGroupCount && aOptions->IsHTMLElement(nsGkAtoms::optgroup)) {
-    mOptGroupCount--;
-
-    for (nsIContent* child = aOptions->GetFirstChild();
-         child;
-         child = child->GetNextSibling()) {
-      nsresult rv = RemoveOptionsFromListRecurse(child,
-                                                 aRemoveIndex,
-                                                 aNumRemoved,
-                                                 aDepth + 1);
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
-  }
-
-  return NS_OK;
-}
-
 // XXXldb Doing the processing before the content nodes have been added
 // to the document (as the name of this function seems to require, and
 // as the callers do), is highly unusual.  Passing around unparented
 // content to other parts of the app can make those things think the
 // options are the root content node.
 NS_IMETHODIMP
 HTMLSelectElement::WillAddOptions(nsIContent* aOptions,
                                   nsIContent* aParent,
                                   int32_t aContentIndex,
                                   bool aNotify)
 {
-  int32_t level = GetContentDepth(aParent);
-  if (level == -1) {
-    return NS_ERROR_FAILURE;
+  if (this != aParent && this != aParent->GetParent()) {
+    return NS_OK;
   }
+  int32_t level = aParent == this ? 0 : 1;
 
   // Get the index where the options will be inserted
   int32_t ind = -1;
   if (!mNonOptionChildren) {
     // If there are no artifacts, aContentIndex == ind
     ind = aContentIndex;
   } else {
     // If there are artifacts, we have to get the index of the option the
@@ -457,21 +429,20 @@ HTMLSelectElement::WillAddOptions(nsICon
   return NS_OK;
 }
 
 NS_IMETHODIMP
 HTMLSelectElement::WillRemoveOptions(nsIContent* aParent,
                                      int32_t aContentIndex,
                                      bool aNotify)
 {
-  int32_t level = GetContentDepth(aParent);
-  NS_ASSERTION(level >= 0, "getting notified by unexpected content");
-  if (level == -1) {
-    return NS_ERROR_FAILURE;
+  if (this != aParent && this != aParent->GetParent()) {
+    return NS_OK;
   }
+  int32_t level = this == aParent ? 0 : 1;
 
   // Get the index where the options will be removed
   nsIContent* currentKid = aParent->GetChildAt(aContentIndex);
   if (currentKid) {
     int32_t ind;
     if (!mNonOptionChildren) {
       // If there are no artifacts, aContentIndex == ind
       ind = aContentIndex;
@@ -485,34 +456,16 @@ HTMLSelectElement::WillRemoveOptions(nsI
       NS_ENSURE_SUCCESS(rv, rv);
     }
   }
 
   return NS_OK;
 }
 
 int32_t
-HTMLSelectElement::GetContentDepth(nsIContent* aContent)
-{
-  nsIContent* content = aContent;
-
-  int32_t retval = 0;
-  while (content != this) {
-    retval++;
-    content = content->GetParent();
-    if (!content) {
-      retval = -1;
-      break;
-    }
-  }
-
-  return retval;
-}
-
-int32_t
 HTMLSelectElement::GetOptionIndexAt(nsIContent* aOptions)
 {
   // Search this node and below.
   // If not found, find the first one *after* this node.
   int32_t retval = GetFirstOptionIndex(aOptions);
   if (retval == -1) {
     retval = GetOptionIndexAfter(aOptions);
   }
@@ -523,19 +476,17 @@ HTMLSelectElement::GetOptionIndexAt(nsIC
 int32_t
 HTMLSelectElement::GetOptionIndexAfter(nsIContent* aOptions)
 {
   // - If this is the select, the next option is the last.
   // - If not, search all the options after aOptions and up to the last option
   //   in the parent.
   // - If it's not there, search for the first option after the parent.
   if (aOptions == this) {
-    uint32_t len;
-    GetLength(&len);
-    return len;
+    return Length();
   }
 
   int32_t retval = -1;
 
   nsCOMPtr<nsIContent> parent = aOptions->GetParent();
 
   if (parent) {
     int32_t index = parent->IndexOf(aOptions);
@@ -553,18 +504,16 @@ HTMLSelectElement::GetOptionIndexAfter(n
 
 int32_t
 HTMLSelectElement::GetFirstOptionIndex(nsIContent* aOptions)
 {
   int32_t listIndex = -1;
   HTMLOptionElement* optElement = HTMLOptionElement::FromContent(aOptions);
   if (optElement) {
     GetOptionIndex(optElement, 0, true, &listIndex);
-    // If you nested stuff under the option, you're just plain
-    // screwed.  *I'm* not going to aid and abet your evil deed.
     return listIndex;
   }
 
   listIndex = GetFirstChildOptionIndex(aOptions, 0, aOptions->GetChildCount());
 
   return listIndex;
 }
 
@@ -1780,35 +1729,42 @@ HTMLSelectElement::DispatchContentReset(
       if (listFrame) {
         listFrame->OnContentReset();
       }
     }
   }
 }
 
 static void
-AddOptionsRecurse(nsIContent* aRoot, HTMLOptionsCollection* aArray)
+AddOptions(nsIContent* aRoot, HTMLOptionsCollection* aArray)
 {
-  for (nsIContent* cur = aRoot->GetFirstChild();
-       cur;
-       cur = cur->GetNextSibling()) {
-    HTMLOptionElement* opt = HTMLOptionElement::FromContent(cur);
+  for (nsIContent* child = aRoot->GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    HTMLOptionElement* opt = HTMLOptionElement::FromContent(child);
     if (opt) {
       aArray->AppendOption(opt);
-    } else if (cur->IsHTMLElement(nsGkAtoms::optgroup)) {
-      AddOptionsRecurse(cur, aArray);
+    } else if (child->IsHTMLElement(nsGkAtoms::optgroup)) {
+      for (nsIContent* grandchild = child->GetFirstChild();
+           grandchild;
+           grandchild = grandchild->GetNextSibling()) {
+        opt = HTMLOptionElement::FromContent(grandchild);
+        if (opt) {
+          aArray->AppendOption(opt);
+        }
+      }
     }
   }
 }
 
 void
 HTMLSelectElement::RebuildOptionsArray(bool aNotify)
 {
   mOptions->Clear();
-  AddOptionsRecurse(this, mOptions);
+  AddOptions(this, mOptions);
   FindSelectedIndex(0, aNotify);
 }
 
 bool
 HTMLSelectElement::IsValueMissing()
 {
   if (!Required()) {
     return false;
@@ -1858,38 +1814,39 @@ HTMLSelectElement::GetValidationMessage(
     default: {
       return nsIConstraintValidation::GetValidationMessage(aValidationMessage, aType);
     }
   }
 }
 
 #ifdef DEBUG
 
-static void
-VerifyOptionsRecurse(nsIContent* aRoot, int32_t& aIndex,
-                     HTMLOptionsCollection* aArray)
-{
-  for (nsIContent* cur = aRoot->GetFirstChild();
-       cur;
-       cur = cur->GetNextSibling()) {
-    nsCOMPtr<nsIDOMHTMLOptionElement> opt = do_QueryInterface(cur);
-    if (opt) {
-      NS_ASSERTION(opt == aArray->ItemAsOption(aIndex++),
-                   "Options collection broken");
-    } else if (cur->IsHTMLElement(nsGkAtoms::optgroup)) {
-      VerifyOptionsRecurse(cur, aIndex, aArray);
-    }
-  }
-}
-
 void
 HTMLSelectElement::VerifyOptionsArray()
 {
-  int32_t aIndex = 0;
-  VerifyOptionsRecurse(this, aIndex, mOptions);
+  int32_t index = 0;
+  for (nsIContent* child = nsINode::GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    HTMLOptionElement* opt = HTMLOptionElement::FromContent(child);
+    if (opt) {
+      NS_ASSERTION(opt == mOptions->ItemAsOption(index++),
+                   "Options collection broken");
+    } else if (child->IsHTMLElement(nsGkAtoms::optgroup)) {
+      for (nsIContent* grandchild = child->GetFirstChild();
+           grandchild;
+           grandchild = grandchild->GetNextSibling()) {
+        opt = HTMLOptionElement::FromContent(grandchild);
+        if (opt) {
+          NS_ASSERTION(opt == mOptions->ItemAsOption(index++),
+                       "Options collection broken");
+        }
+      }
+    }
+  }
 }
 
 #endif
 
 void
 HTMLSelectElement::UpdateBarredFromConstraintValidation()
 {
   SetBarredFromConstraintValidation(IsDisabled());
--- a/dom/html/HTMLSelectElement.h
+++ b/dom/html/HTMLSelectElement.h
@@ -497,48 +497,22 @@ protected:
    * @param aOptions the option or optgroup being added
    * @param aListIndex the index to start removing options from the list at
    * @param aDepth the depth of aOptions (1=direct child of select ...)
    */
   nsresult RemoveOptionsFromList(nsIContent* aOptions,
                                  int32_t aListIndex,
                                  int32_t aDepth,
                                  bool aNotify);
-  /**
-   * Insert option(s) into the options[] array (called by InsertOptionsIntoList)
-   * @param aOptions the option or optgroup being added
-   * @param aInsertIndex the index to start adding options into the list at
-   * @param aDepth the depth of aOptions (1=direct child of select ...)
-   */
-  void InsertOptionsIntoListRecurse(nsIContent* aOptions,
-                                    int32_t* aInsertIndex,
-                                    int32_t aDepth);
-  /**
-   * Remove option(s) from the options[] array (called by RemoveOptionsFromList)
-   * @param aOptions the option or optgroup being added
-   * @param aListIndex the index to start removing options from the list at
-   * @param aNumRemoved the number removed so far [OUT]
-   * @param aDepth the depth of aOptions (1=direct child of select ...)
-   */
-  nsresult RemoveOptionsFromListRecurse(nsIContent* aOptions,
-                                        int32_t aRemoveIndex,
-                                        int32_t* aNumRemoved,
-                                        int32_t aDepth);
 
   // nsIConstraintValidation
   void UpdateBarredFromConstraintValidation();
   bool IsValueMissing();
 
   /**
-   * Find out how deep this content is from the select (1=direct child)
-   * @param aContent the content to check
-   * @return the depth
-   */
-  int32_t GetContentDepth(nsIContent* aContent);
-  /**
    * Get the index of the first option at, under or following the content in
    * the select, or length of options[] if none are found
    * @param aOptions the content
    * @return the index of the first option
    */
   int32_t GetOptionIndexAt(nsIContent* aOptions);
   /**
    * Get the next option following the content in question (not at or under)
deleted file mode 100644
--- a/testing/web-platform/meta/html/infrastructure/common-dom-interfaces/collections/htmloptionscollection.html.ini
+++ /dev/null
@@ -1,5 +0,0 @@
-[htmloptionscollection.html]
-  type: testharness
-  [Insert <optgroup><optgroup><option>6</option></optgroup></optgroup> into <select>]
-    expected: FAIL
-