Make nsRuleNode's use of font metrics for ch and ex units pass the correct language. (Bug 678671, patch 3) r=roc
authorL. David Baron <dbaron@dbaron.org>
Sun, 14 Aug 2011 10:08:04 -0700
changeset 75280 3a378e08192f9fcca5af00951674ee85898b0f90
parent 75279 f277fe9f70c39d760472ce223a60a78de64620a1
child 75281 83291ec2e28e44e466a819422c9850af8320b48b
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
reviewersroc
bugs678671
milestone8.0a1
Make nsRuleNode's use of font metrics for ch and ex units pass the correct language. (Bug 678671, patch 3) r=roc This makes these users (which are exceptions within layout, although low-level enough that it won't matter for font inflation work) call through to GetMetricsFor explicitly with the correct language, rather than using the broken nsPresContext::GetMetricsFor and its charset-detected language. This improves the correctness of our behavior for 'ch' and 'ex' CSS units when the font selection (or defaults) are language-dependent. It should also reduce the number of unique sets of font metrics requested (which helps nsFontCache effectiveness).
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
layout/style/test/Makefile.in
layout/style/test/test_ch_ex_no_infloops.html
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -179,77 +179,108 @@ static void EnsureBlockDisplay(PRUint8& 
     // make it a block
     display = NS_STYLE_DISPLAY_BLOCK;
   }
 }
 
 static nscoord CalcLengthWith(const nsCSSValue& aValue,
                               nscoord aFontSize,
                               const nsStyleFont* aStyleFont,
+                              const nsStyleVisibility* aStyleVisibility,
                               nsStyleContext* aStyleContext,
                               nsPresContext* aPresContext,
                               PRBool aUseProvidedRootEmSize,
                               PRBool aUseUserFontSet,
                               PRBool& aCanStoreInRuleTree);
 
 struct CalcLengthCalcOps : public css::BasicCoordCalcOps,
                            public css::NumbersAlreadyNormalizedOps
 {
   // All of the parameters to CalcLengthWith except aValue.
   const nscoord mFontSize;
   const nsStyleFont* const mStyleFont;
+  const nsStyleVisibility* const mStyleVisibility;
   nsStyleContext* const mStyleContext;
   nsPresContext* const mPresContext;
   const PRBool mUseProvidedRootEmSize;
   const PRBool mUseUserFontSet;
   PRBool& mCanStoreInRuleTree;
 
   CalcLengthCalcOps(nscoord aFontSize, const nsStyleFont* aStyleFont,
+                    const nsStyleVisibility* aStyleVisibility,
                     nsStyleContext* aStyleContext, nsPresContext* aPresContext,
                     PRBool aUseProvidedRootEmSize, PRBool aUseUserFontSet,
                     PRBool& aCanStoreInRuleTree)
     : mFontSize(aFontSize),
       mStyleFont(aStyleFont),
+      mStyleVisibility(aStyleVisibility),
       mStyleContext(aStyleContext),
       mPresContext(aPresContext),
       mUseProvidedRootEmSize(aUseProvidedRootEmSize),
       mUseUserFontSet(aUseUserFontSet),
       mCanStoreInRuleTree(aCanStoreInRuleTree)
   {
   }
 
   result_type ComputeLeafValue(const nsCSSValue& aValue)
   {
-    return CalcLengthWith(aValue, mFontSize, mStyleFont, mStyleContext,
-                          mPresContext, mUseProvidedRootEmSize,
+    return CalcLengthWith(aValue, mFontSize, mStyleFont, mStyleVisibility,
+                          mStyleContext, mPresContext, mUseProvidedRootEmSize,
                           mUseUserFontSet, mCanStoreInRuleTree);
   }
 };
 
 static inline nscoord ScaleCoord(const nsCSSValue &aValue, float factor)
 {
   return NSToCoordRoundWithClamp(aValue.GetFloatValue() * factor);
 }
 
+already_AddRefed<nsFontMetrics>
+GetMetricsFor(nsPresContext* aPresContext,
+              nsStyleContext* aStyleContext,
+              const nsStyleFont* aStyleFont,
+              const nsStyleVisibility* aStyleVisibility,
+              nscoord aFontSize, // overrides value from aStyleFont
+              PRBool aUseUserFontSet)
+{
+  nsFont font = aStyleFont->mFont;
+  font.size = aFontSize;
+  gfxUserFontSet *fs = nsnull;
+  if (aUseUserFontSet) {
+    fs = aPresContext->GetUserFontSet();
+  }
+  nsRefPtr<nsFontMetrics> fm;
+  if (!aStyleVisibility) {
+    aStyleVisibility = aStyleContext->GetStyleVisibility();
+  }
+  aPresContext->DeviceContext()->GetMetricsFor(font,
+                                               aStyleVisibility->mLanguage,
+                                               fs, *getter_AddRefs(fm));
+  return fm.forget();
+}
+
 static nscoord CalcLengthWith(const nsCSSValue& aValue,
                               nscoord aFontSize,
                               const nsStyleFont* aStyleFont,
+                              const nsStyleVisibility* aStyleVisibility,
                               nsStyleContext* aStyleContext,
                               nsPresContext* aPresContext,
                               PRBool aUseProvidedRootEmSize,
                               // aUseUserFontSet should always be PR_TRUE
                               // except when called from
                               // CalcLengthWithInitialFont.
                               PRBool aUseUserFontSet,
                               PRBool& aCanStoreInRuleTree)
 {
   NS_ASSERTION(aValue.IsLengthUnit() || aValue.IsCalcUnit(),
                "not a length or calc unit");
-  NS_ASSERTION(aStyleFont || aStyleContext, "Must have style data");
-  NS_ASSERTION(!aStyleFont || !aStyleContext, "Duplicate sources of data");
+  NS_ASSERTION((aStyleFont && aStyleVisibility) || aStyleContext,
+               "Must have style data");
+  NS_ASSERTION((!aStyleFont && !aStyleVisibility) || !aStyleContext,
+               "Duplicate sources of data");
   NS_ASSERTION(aPresContext, "Must have prescontext");
 
   if (aValue.IsFixedLengthUnit()) {
     return aValue.GetFixedLength(aPresContext);
   }
   if (aValue.IsPixelLengthUnit()) {
     return aValue.GetPixelLength();
   }
@@ -300,27 +331,25 @@ static nscoord CalcLengthWith(const nsCS
 
       return ScaleCoord(aValue, float(rootFontSize));
     }
     case eCSSUnit_EM: {
       return ScaleCoord(aValue, float(aFontSize));
       // XXX scale against font metrics height instead?
     }
     case eCSSUnit_XHeight: {
-      nsFont font = styleFont->mFont;
-      font.size = aFontSize;
       nsRefPtr<nsFontMetrics> fm =
-        aPresContext->GetMetricsFor(font, aUseUserFontSet);
+        GetMetricsFor(aPresContext, aStyleContext, styleFont,
+                      aStyleVisibility, aFontSize, aUseUserFontSet);
       return ScaleCoord(aValue, float(fm->XHeight()));
     }
     case eCSSUnit_Char: {
-      nsFont font = styleFont->mFont;
-      font.size = aFontSize;
       nsRefPtr<nsFontMetrics> fm =
-        aPresContext->GetMetricsFor(font, aUseUserFontSet);
+        GetMetricsFor(aPresContext, aStyleContext, styleFont,
+                      aStyleVisibility, aFontSize, aUseUserFontSet);
       gfxFloat zeroWidth = (fm->GetThebesFontGroup()->GetFontAt(0)
                             ->GetMetrics().zeroOrAveCharWidth);
 
       return ScaleCoord(aValue, ceil(aPresContext->AppUnitsPerDevPixel() *
                                      zeroWidth));
     }
     // For properties for which lengths are the *only* units accepted in
     // calc(), we can handle calc() here and just compute a final
@@ -328,17 +357,18 @@ static nscoord CalcLengthWith(const nsCS
     // properties by not calling CalcLength in those cases:  SetCoord
     // only calls CalcLength for a calc when it is appropriate to do so.
     case eCSSUnit_Calc:
     case eCSSUnit_Calc_Plus:
     case eCSSUnit_Calc_Minus:
     case eCSSUnit_Calc_Times_L:
     case eCSSUnit_Calc_Times_R:
     case eCSSUnit_Calc_Divided: {
-      CalcLengthCalcOps ops(aFontSize, aStyleFont, aStyleContext, aPresContext,
+      CalcLengthCalcOps ops(aFontSize, aStyleFont, aStyleVisibility,
+                            aStyleContext, aPresContext,
                             aUseProvidedRootEmSize, aUseUserFontSet,
                             aCanStoreInRuleTree);
       return css::ComputeCalc(aValue, ops);
     }
     default:
       NS_NOTREACHED("unexpected unit");
       break;
   }
@@ -348,17 +378,18 @@ static nscoord CalcLengthWith(const nsCS
 /* static */ nscoord
 nsRuleNode::CalcLength(const nsCSSValue& aValue,
                        nsStyleContext* aStyleContext,
                        nsPresContext* aPresContext,
                        PRBool& aCanStoreInRuleTree)
 {
   NS_ASSERTION(aStyleContext, "Must have style data");
 
-  return CalcLengthWith(aValue, -1, nsnull, aStyleContext, aPresContext,
+  return CalcLengthWith(aValue, -1, nsnull, nsnull,
+                        aStyleContext, aPresContext,
                         PR_FALSE, PR_TRUE, aCanStoreInRuleTree);
 }
 
 /* Inline helper function to redirect requests to CalcLength. */
 static inline nscoord CalcLength(const nsCSSValue& aValue,
                                  nsStyleContext* aStyleContext,
                                  nsPresContext* aPresContext,
                                  PRBool& aCanStoreInRuleTree)
@@ -367,18 +398,20 @@ static inline nscoord CalcLength(const n
                                 aPresContext, aCanStoreInRuleTree);
 }
 
 /* static */ nscoord
 nsRuleNode::CalcLengthWithInitialFont(nsPresContext* aPresContext,
                                       const nsCSSValue& aValue)
 {
   nsStyleFont defaultFont(aPresContext);
+  nsStyleVisibility defaultVisibility(aPresContext); // FIXME: best language?
   PRBool canStoreInRuleTree;
-  return CalcLengthWith(aValue, -1, &defaultFont, nsnull, aPresContext,
+  return CalcLengthWith(aValue, -1, &defaultFont, &defaultVisibility,
+                        nsnull, aPresContext,
                         PR_TRUE, PR_FALSE, canStoreInRuleTree);
 }
 
 struct LengthPercentPairCalcOps : public css::NumbersAlreadyNormalizedOps
 {
   typedef nsRuleNode::ComputedCalc result_type;
 
   LengthPercentPairCalcOps(nsStyleContext* aContext,
@@ -2437,39 +2470,43 @@ ComputeScriptLevelSize(const nsStyleFont
 }
 
 struct SetFontSizeCalcOps : public css::BasicCoordCalcOps,
                             public css::NumbersAlreadyNormalizedOps
 {
   // The parameters beyond aValue that we need for CalcLengthWith.
   const nscoord mParentSize;
   const nsStyleFont* const mParentFont;
+  const nsStyleVisibility* const mLanguageVisibility;
   nsPresContext* const mPresContext;
   const PRBool mAtRoot;
   PRBool& mCanStoreInRuleTree;
 
   SetFontSizeCalcOps(nscoord aParentSize, const nsStyleFont* aParentFont,
+                     const nsStyleVisibility* aLanguageVisibility,
                      nsPresContext* aPresContext, PRBool aAtRoot,
                      PRBool& aCanStoreInRuleTree)
     : mParentSize(aParentSize),
       mParentFont(aParentFont),
+      mLanguageVisibility(aLanguageVisibility),
       mPresContext(aPresContext),
       mAtRoot(aAtRoot),
       mCanStoreInRuleTree(aCanStoreInRuleTree)
   {
   }
 
   result_type ComputeLeafValue(const nsCSSValue& aValue)
   {
     nscoord size;
     if (aValue.IsLengthUnit()) {
       // Note that font-based length units use the parent's size
       // unadjusted for scriptlevel changes. A scriptlevel change
       // between us and the parent is simply ignored.
-      size = CalcLengthWith(aValue, mParentSize, mParentFont,
+      size = CalcLengthWith(aValue, mParentSize,
+                            mParentFont, mLanguageVisibility,
                             nsnull, mPresContext, mAtRoot,
                             PR_TRUE, mCanStoreInRuleTree);
       if (!aValue.IsRelativeLengthUnit()) {
         size = nsStyleFont::ZoomText(mPresContext, size);
       }
     }
     else if (eCSSUnit_Percent == aValue.GetUnit()) {
       mCanStoreInRuleTree = PR_FALSE;
@@ -2487,16 +2524,17 @@ struct SetFontSizeCalcOps : public css::
   }
 };
 
 /* static */ void
 nsRuleNode::SetFontSize(nsPresContext* aPresContext,
                         const nsRuleData* aRuleData,
                         const nsStyleFont* aFont,
                         const nsStyleFont* aParentFont,
+                        const nsStyleVisibility* aLanguageVisibility,
                         nscoord* aSize,
                         const nsFont& aSystemFont,
                         nscoord aParentSize,
                         nscoord aScriptLevelAdjustedParentSize,
                         PRBool aUsedStartStruct,
                         PRBool aAtRoot,
                         PRBool& aCanStoreInRuleTree)
 {
@@ -2545,18 +2583,18 @@ nsRuleNode::SetFontSize(nsPresContext* a
       }
     } else {
       NS_NOTREACHED("unexpected value");
     }
   }
   else if (sizeValue->IsLengthUnit() ||
            sizeValue->GetUnit() == eCSSUnit_Percent ||
            sizeValue->IsCalcUnit()) {
-    SetFontSizeCalcOps ops(aParentSize, aParentFont, aPresContext, aAtRoot,
-                           aCanStoreInRuleTree);
+    SetFontSizeCalcOps ops(aParentSize, aParentFont, aLanguageVisibility,
+                           aPresContext, aAtRoot, aCanStoreInRuleTree);
     *aSize = css::ComputeCalc(*sizeValue, ops);
     if (*aSize < 0) {
       NS_ABORT_IF_FALSE(sizeValue->IsCalcUnit(),
                         "negative lengths and percents should be rejected "
                         "by parser");
       *aSize = 0;
     }
     // Zoom is handled inside the calc ops when needed.
@@ -2617,16 +2655,22 @@ nsRuleNode::SetFont(nsPresContext* aPres
                     PRUint8 aGenericFontID, const nsRuleData* aRuleData,
                     const nsStyleFont* aParentFont,
                     nsStyleFont* aFont, PRBool aUsedStartStruct,
                     PRBool& aCanStoreInRuleTree)
 {
   const nsFont* defaultVariableFont =
     aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID);
   PRBool atRoot = !aContext->GetParent();
+  // We get '%', 'em', 'ex', and 'ch' units from aParentFont.  For 'ex'
+  // and 'ch' units we use font metrics, which depend on language.  It
+  // makes sense to use the language from the same style context as
+  // aParentFont.
+  const nsStyleVisibility *languageVisibility =
+    (atRoot ? aContext : aContext->GetParent())->GetStyleVisibility();
 
   // -moz-system-font: enum (never inherit!)
   nsFont systemFont;
   const nsCSSValue* systemFontValue = aRuleData->ValueForSystemFont();
   if (eCSSUnit_Enumerated == systemFontValue->GetUnit()) {
     nsSystemFontID sysID;
     switch (systemFontValue->GetIntValue()) {
       case NS_STYLE_FONT_CAPTION:       sysID = eSystemFont_Caption;      break;    // css2
@@ -2805,17 +2849,18 @@ nsRuleNode::SetFont(nsPresContext* aPres
 
   // -moz-script-min-size: length
   const nsCSSValue* scriptMinSizeValue = aRuleData->ValueForScriptMinSize();
   if (scriptMinSizeValue->IsLengthUnit()) {
     // scriptminsize in font units (em, ex) has to be interpreted relative
     // to the parent font, or the size definitions are circular and we
     //
     aFont->mScriptMinSize =
-      CalcLengthWith(*scriptMinSizeValue, aParentFont->mSize, aParentFont,
+      CalcLengthWith(*scriptMinSizeValue, aParentFont->mSize,
+                     aParentFont, languageVisibility,
                      nsnull, aPresContext, atRoot, PR_TRUE,
                      aCanStoreInRuleTree);
   }
 
   // -moz-script-size-multiplier: factor, inherit, initial
   SetFactor(*aRuleData->ValueForScriptSizeMultiplier(),
             aFont->mScriptSizeMultiplier,
             aCanStoreInRuleTree, aParentFont->mScriptSizeMultiplier,
@@ -2873,31 +2918,32 @@ nsRuleNode::SetFont(nsPresContext* aPres
   // font-size: enum, length, percent, inherit
   nscoord scriptLevelAdjustedParentSize = aParentFont->mSize;
   nscoord scriptLevelAdjustedUnconstrainedParentSize;
   scriptLevelAdjustedParentSize =
     ComputeScriptLevelSize(aFont, aParentFont, aPresContext,
                            &scriptLevelAdjustedUnconstrainedParentSize);
   NS_ASSERTION(!aUsedStartStruct || aFont->mScriptUnconstrainedSize == aFont->mSize,
                "If we have a start struct, we should have reset everything coming in here");
-  SetFontSize(aPresContext, aRuleData, aFont, aParentFont, &aFont->mSize,
+  SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
+              languageVisibility, &aFont->mSize,
               systemFont, aParentFont->mSize, scriptLevelAdjustedParentSize,
               aUsedStartStruct, atRoot, aCanStoreInRuleTree);
   if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
       scriptLevelAdjustedParentSize == scriptLevelAdjustedUnconstrainedParentSize) {
     // Fast path: we have not been affected by scriptminsize so we don't
     // need to call SetFontSize again to compute the
     // scriptminsize-unconstrained size. This is OK even if we have a
     // start struct, because if we have a start struct then 'font-size'
     // was specified and so scriptminsize has no effect.
     aFont->mScriptUnconstrainedSize = aFont->mSize;
   } else {
     SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
-                &aFont->mScriptUnconstrainedSize, systemFont,
-                aParentFont->mScriptUnconstrainedSize,
+                languageVisibility, &aFont->mScriptUnconstrainedSize,
+                systemFont, aParentFont->mScriptUnconstrainedSize,
                 scriptLevelAdjustedUnconstrainedParentSize,
                 aUsedStartStruct, atRoot, aCanStoreInRuleTree);
   }
   NS_ASSERTION(aFont->mScriptUnconstrainedSize <= aFont->mSize,
                "scriptminsize should never be making things bigger");
 
   // enforce the user' specified minimum font-size on the value that we expose
   // (but don't change font-size:0)
@@ -4551,16 +4597,20 @@ nsRuleNode::ComputeVisibilityData(void* 
                                   nsStyleContext* aContext,
                                   nsRuleNode* aHighestNode,
                                   const RuleDetail aRuleDetail,
                                   const PRBool aCanStoreInRuleTree)
 {
   COMPUTE_START_INHERITED(Visibility, (mPresContext),
                           visibility, parentVisibility)
 
+  // IMPORTANT: No properties in this struct have lengths in them.  We
+  // depend on this since CalcLengthWith can call GetStyleVisibility()
+  // to get the language for resolving fonts!
+
   // direction: enum, inherit, initial
   SetDiscrete(*aRuleData->ValueForDirection(), visibility->mDirection,
               canStoreInRuleTree,
               SETDSC_ENUMERATED, parentVisibility->mDirection,
               (GET_BIDI_OPTION_DIRECTION(mPresContext->GetBidi())
                == IBMBIDI_TEXTDIRECTION_RTL)
               ? NS_STYLE_DIRECTION_RTL : NS_STYLE_DIRECTION_LTR,
               0, 0, 0, 0);
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -595,16 +595,17 @@ protected:
                         RuleDetail aRuleDetail,
                         const PRBool aCanStoreInRuleTree);
 
   // helpers for |ComputeFontData| that need access to |mNoneBits|:
   static void SetFontSize(nsPresContext* aPresContext,
                           const nsRuleData* aRuleData,
                           const nsStyleFont* aFont,
                           const nsStyleFont* aParentFont,
+                          const nsStyleVisibility* aLanguageVisibility,
                           nscoord* aSize,
                           const nsFont& aSystemFont,
                           nscoord aParentSize,
                           nscoord aScriptLevelAdjustedParentSize,
                           PRBool aUsedStartStruct,
                           PRBool aAtRoot,
                           PRBool& aCanStoreInRuleTree);
 
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -128,16 +128,17 @@ GARBAGE += css_properties.js
 		test_bug652486.html \
 		test_bug657143.html \
 		test_bug664955.html \
 		test_bug667520.html \
 		test_bug645998.html \
 		file_bug645998-1.css \
 		file_bug645998-2.css \
 		test_cascade.html \
+		test_ch_ex_no_infloops.html \
 		test_compute_data_with_start_struct.html \
 		test_computed_style.html \
 		test_computed_style_no_pseudo.html \
 		test_css_cross_domain.html \
 		test_css_eof_handling.html \
 		test_descriptor_storage.html \
 		test_descriptor_syntax_errors.html \
 		test_dont_use_document_colors.html \
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_ch_ex_no_infloops.html
@@ -0,0 +1,64 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=678671
+-->
+<head>
+  <title>Test for Bug 678671</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="property_database.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=678671">Mozilla Bug 678671</a>
+<p id="display"></p>
+<div id="content"></div>
+<script type="application/javascript">
+
+/** Test for Bug 678671 **/
+
+/**
+ * Test 'ex' and 'ch' units in every place we possible can to make
+ * sure they don't cause an infinite loop.
+ */
+
+var content = document.getElementById("content");
+var cs = getComputedStyle(content, "");
+
+for (var prop in gCSSProperties) {
+  var info = gCSSProperties[prop];
+  if (info.backend_only) {
+    continue;
+  }
+  function test_val(v) {
+    content.style.setProperty(prop, v, "");
+    isnot(get_computed_value(cs, prop), "",
+      "Setting '" + prop + "' to '" + v + "' should not cause infinite loop");
+  }
+  test_val('3ex');
+  test_val('2ch');
+  function test_replaced_values(value_list) {
+    // For each item in value_list, if it looks like it has a dimension
+    // in it, replace those dimensions with 3ex and 2ch and test it.
+    for (var i = 0; i < value_list.length; ++i) {
+      var value = value_list[i];
+      function try_replace(withval) {
+        var rep = value.replace(/[0-9.]+[a-zA-Z]+/g, withval)
+        if (rep != value) {
+          test_val(rep);
+        }
+      }
+      try_replace('3ex');
+      try_replace('2ch');
+    }
+  }
+  test_replaced_values(info.initial_values);
+  test_replaced_values(info.other_values);
+  content.style.removeProperty(prop);
+}
+
+</script>
+</pre>
+</body>
+</html>