Avoid discontinuity when options drop to height:0 by using CalcFallbackRowHeight only when GetNumberOfOptions() == 0, thus eliminating the last caller of CalcFallbackRowHeight passing a non-zero option count. (Bug 467084) sr=roc r=bzbarsky a=blocking1.9.1+
authorL. David Baron <dbaron@dbaron.org>
Thu, 04 Dec 2008 08:09:53 -0800
changeset 22336 045908bd6528d9bd6cddff49d4d526acf85d49bd
parent 22335 f7e51a5e66551303de5042e9a6911c44e0e8bf5a
child 22337 fb1b3f68b09d2ee2bc6911f4b9231a1bfcaf4ecc
push idunknown
push userunknown
push dateunknown
reviewersroc, bzbarsky, blocking1.9.1
bugs467084
milestone1.9.2a1pre
Avoid discontinuity when options drop to height:0 by using CalcFallbackRowHeight only when GetNumberOfOptions() == 0, thus eliminating the last caller of CalcFallbackRowHeight passing a non-zero option count. (Bug 467084) sr=roc r=bzbarsky a=blocking1.9.1+
layout/forms/nsListControlFrame.cpp
layout/forms/nsListControlFrame.h
layout/reftests/bugs/467084-1-ref.html
layout/reftests/bugs/467084-1.html
layout/reftests/bugs/467084-2-ref.html
layout/reftests/bugs/467084-2.html
layout/reftests/bugs/reftest.list
--- a/layout/forms/nsListControlFrame.cpp
+++ b/layout/forms/nsListControlFrame.cpp
@@ -339,17 +339,17 @@ void nsListControlFrame::PaintFocus(nsIR
   if (childframe) {
     // get the child rect
     fRect = childframe->GetRect();
     // get it into our coordinates
     fRect.MoveBy(childframe->GetParent()->GetOffsetTo(this));
   } else {
     fRect.x = fRect.y = 0;
     fRect.width = GetScrollPortSize().width;
-    fRect.height = CalcFallbackRowHeight(0);
+    fRect.height = CalcFallbackRowHeight();
     fRect.MoveBy(containerFrame->GetOffsetTo(this));
   }
   fRect += aPt;
   
   PRBool lastItemIsSelected = PR_FALSE;
   if (focusedContent) {
     nsCOMPtr<nsIDOMHTMLOptionElement> domOpt =
       do_QueryInterface(focusedContent);
@@ -375,17 +375,17 @@ nsListControlFrame::InvalidateFocus()
     return;
 
   nsIFrame* containerFrame = GetOptionsContainer();
   if (containerFrame) {
     // Invalidating from the containerFrame because that's where our focus
     // is drawn.
     // The origin of the scrollport is the origin of containerFrame.
     nsRect invalidateArea = containerFrame->GetOverflowRect();
-    nsRect emptyFallbackArea(0, 0, GetScrollPortSize().width, CalcFallbackRowHeight(0));
+    nsRect emptyFallbackArea(0, 0, GetScrollPortSize().width, CalcFallbackRowHeight());
     invalidateArea.UnionRect(invalidateArea, emptyFallbackArea);
     containerFrame->Invalidate(invalidateArea);
   }
 }
 
 //---------------------------------------------------------
 // Frames are not refcounted, no need to AddRef
 NS_IMETHODIMP
@@ -504,19 +504,20 @@ nscoord
 nsListControlFrame::CalcHeightOfARow()
 {
   // Calculate the height of a single row in the listbox or dropdown list by
   // using the tallest thing in the subtree, since there may be option groups
   // in addition to option elements, either of which may be visible or
   // invisible, may use different fonts, etc.
   PRInt32 heightOfARow = GetMaxOptionHeight(GetOptionsContainer());
 
-  // Check to see if we have zero items 
-  if (heightOfARow == 0) {
-    heightOfARow = CalcFallbackRowHeight(GetNumberOfOptions());
+  // Check to see if we have zero items (and optimize by checking
+  // heightOfARow first)
+  if (heightOfARow == 0 && GetNumberOfOptions() == 0) {
+    heightOfARow = CalcFallbackRowHeight();
   }
 
   return heightOfARow;
 }
 
 nscoord
 nsListControlFrame::GetPrefWidth(nsIRenderingContext *aRenderingContext)
 {
@@ -1870,37 +1871,22 @@ nsListControlFrame::IsLeftButton(nsIDOME
     if (NS_SUCCEEDED(mouseEvent->GetButton(&whichButton))) {
       return whichButton != 0?PR_FALSE:PR_TRUE;
     }
   }
   return PR_FALSE;
 }
 
 nscoord
-nsListControlFrame::CalcFallbackRowHeight(PRInt32 aNumOptions)
+nsListControlFrame::CalcFallbackRowHeight()
 {
-  nsIFrame *fontFrame = nsnull;
-    
-  if (aNumOptions > 0) {
-    // Try the first option
-    nsCOMPtr<nsIContent> option = GetOptionContent(0);
-    if (option) {
-      fontFrame = PresContext()->PresShell()->GetPrimaryFrameFor(option);
-    }
-  }
-
-  if (!fontFrame) {
-    // Fall back to our own font
-    fontFrame = this;
-  }
-
   nscoord rowHeight = 0;
   
   nsCOMPtr<nsIFontMetrics> fontMet;
-  nsLayoutUtils::GetFontMetricsForFrame(fontFrame, getter_AddRefs(fontMet));
+  nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fontMet));
   if (fontMet) {
     fontMet->GetHeight(rowHeight);
   }
 
   return rowHeight;
 }
 
 nscoord
--- a/layout/forms/nsListControlFrame.h
+++ b/layout/forms/nsListControlFrame.h
@@ -368,19 +368,18 @@ protected:
    */
   PRBool   IsContentSelectedByIndex(PRInt32 aIndex) const;
 
   PRBool   IsOptionElement(nsIContent* aContent);
   PRBool   CheckIfAllFramesHere();
   PRInt32  GetIndexFromContent(nsIContent *aContent);
   PRBool   IsLeftButton(nsIDOMEvent* aMouseEvent);
 
-  // aNumOptions is the number of options we have; if we have none,
-  // we'll just guess at a row height based on our own style.
-  nscoord  CalcFallbackRowHeight(PRInt32 aNumOptions);
+  // guess at a row height based on our own style.
+  nscoord  CalcFallbackRowHeight();
 
   // CalcIntrinsicHeight computes our intrinsic height (taking the "size"
   // attribute into account).  This should only be called in non-dropdown mode.
   nscoord CalcIntrinsicHeight(nscoord aHeightOfARow, PRInt32 aNumberOfOptions);
 
   // Dropped down stuff
   void     SetComboboxItem(PRInt32 aIndex);
 
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/467084-1-ref.html
@@ -0,0 +1,19 @@
+<!DOCTYPE HTML>
+<title>Testcase for side issue in bug 467084</title>
+<style type="text/css">
+
+div { background: yellow; }
+select {
+    margin: 2em 0; /* be bigger than the line-height */
+    visibility: hidden;
+}
+option { height: 1px; min-height: 0; }
+
+</style>
+<div>
+<select size="3">
+  <option>One</option>
+  <option>Two</option>
+  <option>Three</option>
+</select>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/467084-1.html
@@ -0,0 +1,19 @@
+<!DOCTYPE HTML>
+<title>Testcase for side issue in bug 467084</title>
+<style type="text/css">
+
+div { background: yellow; padding-bottom: 3px; }
+select {
+    margin: 2em 0; /* be bigger than the line-height */
+    visibility: hidden;
+}
+option { height: 0; min-height: 0; }
+
+</style>
+<div>
+<select size="3">
+  <option>One</option>
+  <option>Two</option>
+  <option>Three</option>
+</select>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/467084-2-ref.html
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML>
+<title>Testcase for side issue in bug 467084</title>
+<style type="text/css">
+
+select { font-size: 50px; }
+
+</style>
+<select size="3">
+  <option></option>
+</select>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/467084-2.html
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML>
+<title>Testcase for side issue in bug 467084</title>
+<style type="text/css">
+
+option { font-size: 50px; }
+
+</style>
+<select size="3">
+  <option></option>
+</select>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -977,8 +977,10 @@ fails == 461512-1.html 461512-1-ref.html
 == 462844-3.html 462844-ref.html
 == 462844-4.html 462844-ref.html
 == 463204-1.html 463204-1-ref.html
 == 463217-1.xul 463217-1-ref.xul
 == 463952-1.html 463952-1-ref.html
 == 464811-1.html 464811-1-ref.html
 == 466395-1.html 466395-1-ref.html
 == 466395-2.html 466395-2-ref.html
+== 467084-1.html 467084-1-ref.html
+== 467084-2.html 467084-2-ref.html