Perform monospace-related adjustment using true parent style

There is currently some code in FontBuilder that tweaks the specified
font size if the current generic font family changes to or from
monospace. This code assumes that the FontDescription stored on the
ComputedStyle pre-CreateFont is the inherited FontDescription, but
this is no longer (always) true.

This means that declarations like font-size:1em can be applied (and
subjected to CreateFont) twice, which means that the second call to
CheckForGenericFamilyChange won't detect a change in the
monospace-ness, hence the specified size will fail to adjust.

This CL fixes this by propagating the actual parent style to
CheckForGenericFamilyChange.

Bug: 1086082, 1086680
Change-Id: Ia61a2327890f27497e4ea5f41dec00a5e0a450d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216511Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772032}

third_party/blink/renderer/core/css/resolver/font_builder.cc

@@ -254,18 +254,18 @@ float FontBuilder::GetComputedSizeFromSpecifiedSize(
 }
 
 void FontBuilder::CheckForGenericFamilyChange(
-    const FontDescription& old_description,
+    const FontDescription& parent_description,
     FontDescription& new_description) {
   DCHECK(document_);
   if (new_description.IsAbsoluteSize())
     return;
 
-  if (new_description.IsMonospace() == old_description.IsMonospace())
+  if (new_description.IsMonospace() == parent_description.IsMonospace())
     return;
 
   // For now, lump all families but monospace together.
   if (new_description.GenericFamily() != FontDescription::kMonospaceFamily &&
-      old_description.GenericFamily() != FontDescription::kMonospaceFamily)
+      parent_description.GenericFamily() != FontDescription::kMonospaceFamily)
     return;
 
   // We know the parent is monospace or the child is monospace, and that font
@@ -284,7 +284,7 @@ void FontBuilder::CheckForGenericFamilyChange(
             ? static_cast<float>(settings->GetDefaultFixedFontSize()) /
                   settings->GetDefaultFontSize()
             : 1;
-    size = old_description.IsMonospace()
+    size = parent_description.IsMonospace()
                ? new_description.SpecifiedSize() / fixed_scale_factor
                : new_description.SpecifiedSize() * fixed_scale_factor;
   }
@@ -293,7 +293,8 @@ void FontBuilder::CheckForGenericFamilyChange(
 }
 
 void FontBuilder::UpdateSpecifiedSize(FontDescription& font_description,
-                                      const ComputedStyle& style) {
+                                      const ComputedStyle& style,
+                                      const ComputedStyle* parent_style) {
   float specified_size = font_description.SpecifiedSize();
 
   if (!specified_size && font_description.KeywordSize())
@@ -302,7 +303,12 @@ void FontBuilder::UpdateSpecifiedSize(FontDescription& font_description,
 
   font_description.SetSpecifiedSize(specified_size);
 
-  CheckForGenericFamilyChange(style.GetFontDescription(), font_description);
+  // TODO(crbug.com/1086680): Avoid nullptr parent style.
+  const FontDescription& parent_description =
+      parent_style ? parent_style->GetFontDescription()
+                   : style.GetFontDescription();
+
+  CheckForGenericFamilyChange(parent_description, font_description);
 }
 
 void FontBuilder::UpdateAdjustedSize(FontDescription& font_description,
@@ -404,7 +410,8 @@ void FontBuilder::UpdateFontDescription(FontDescription& description,
     description.SetAdjustedSize(size);
 }
 
-void FontBuilder::CreateFont(ComputedStyle& style) {
+void FontBuilder::CreateFont(ComputedStyle& style,
+                             const ComputedStyle* parent_style) {
   DCHECK(document_);
 
   if (!flags_)
@@ -413,8 +420,7 @@ void FontBuilder::CreateFont(ComputedStyle& style) {
   FontDescription description = style.GetFontDescription();
 
   UpdateFontDescription(description, style.ComputeFontOrientation());
-
-  UpdateSpecifiedSize(description, style);
+  UpdateSpecifiedSize(description, style, parent_style);
   UpdateComputedSize(description, style);
 
   FontSelector* font_selector = document_->GetStyleEngine().GetFontSelector();
@@ -434,7 +440,7 @@ void FontBuilder::CreateFontForDocument(ComputedStyle& document_style) {
   SetSize(font_description,
           FontDescription::Size(FontSizeFunctions::InitialKeywordSize(), 0.0f,
                                 false));
-  UpdateSpecifiedSize(font_description, document_style);
+  UpdateSpecifiedSize(font_description, document_style, &document_style);
   UpdateComputedSize(font_description, document_style);
 
   font_description.SetOrientation(document_style.ComputeFontOrientation());

third_party/blink/renderer/core/css/resolver/font_builder.h

@@ -79,7 +79,7 @@ class CORE_EXPORT FontBuilder {
   // FIXME: These need to just vend a Font object eventually.
   void UpdateFontDescription(FontDescription&,
                              FontOrientation = FontOrientation::kHorizontal);
-  void CreateFont(ComputedStyle&);
+  void CreateFont(ComputedStyle&, const ComputedStyle* parent_style);
   void CreateFontForDocument(ComputedStyle&);
 
   bool FontDirty() const { return flags_; }
@@ -128,7 +128,9 @@ class CORE_EXPORT FontBuilder {
   // This function fixes up the default font size if it detects that the current
   // generic font family has changed. -dwh
   void CheckForGenericFamilyChange(const FontDescription&, FontDescription&);
-  void UpdateSpecifiedSize(FontDescription&, const ComputedStyle&);
+  void UpdateSpecifiedSize(FontDescription&,
+                           const ComputedStyle&,
+                           const ComputedStyle* parent_style);
   void UpdateComputedSize(FontDescription&, const ComputedStyle&);
   void UpdateAdjustedSize(FontDescription&,
                           const ComputedStyle&,

third_party/blink/renderer/core/css/resolver/font_builder_test.cc

@@ -49,7 +49,7 @@ TEST_F(FontBuilderInitTest, InitialFontSizeNotScaled) {
 
   FontBuilder builder(&GetDocument());
   builder.SetInitial(1.0f);  // FIXME: Remove unused param.
-  builder.CreateFont(*initial);
+  builder.CreateFont(*initial, initial.get());
 
   EXPECT_EQ(16.0f, initial->GetFontDescription().ComputedSize());
 }
@@ -68,12 +68,15 @@ TEST_P(FontBuilderAdditiveTest, OnlySetValueIsModified) {
   FontDescription parent_description;
   funcs.set_base_value(parent_description);
 
+  scoped_refptr<ComputedStyle> parent_style = ComputedStyle::Create();
+  parent_style->SetFontDescription(parent_description);
+
   scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
-  style->SetFontDescription(parent_description);
+  style->InheritFrom(*parent_style);
 
   FontBuilder font_builder(&GetDocument());
   funcs.set_value(font_builder);
-  font_builder.CreateFont(*style);
+  font_builder.CreateFont(*style, parent_style.get());
 
   FontDescription output_description = style->GetFontDescription();
 

third_party/blink/renderer/core/css/resolver/style_cascade.cc

@@ -288,8 +288,7 @@ void StyleCascade::ApplyHighPriority(CascadeResolver& resolver) {
     }
   }
 
-  state_.GetFontBuilder().CreateFont(
-      state_.StyleRef());
+  state_.GetFontBuilder().CreateFont(state_.StyleRef(), state_.ParentStyle());
   state_.SetConversionFontSizes(CSSToLengthConversionData::FontSizes(
       state_.Style(), state_.RootElementStyle()));
   state_.SetConversionZoom(state_.Style()->EffectiveZoom());

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -1078,8 +1078,7 @@ CompositorKeyframeValue* StyleResolver::CreateCompositorKeyframeValueSnapshot(
       cascade.Apply();
     } else {
       StyleBuilder::ApplyProperty(property.GetCSSPropertyName(), state, *value);
-      state.GetFontBuilder().CreateFont(
-          state.StyleRef());
+      state.GetFontBuilder().CreateFont(state.StyleRef(), parent_style);
       CSSVariableResolver(state).ResolveVariableDefinitions();
     }
   }
@@ -1321,7 +1320,7 @@ scoped_refptr<const ComputedStyle> StyleResolver::StyleForText(
 }
 
 void StyleResolver::UpdateFont(StyleResolverState& state) {
-  state.GetFontBuilder().CreateFont(state.StyleRef());
+  state.GetFontBuilder().CreateFont(state.StyleRef(), state.ParentStyle());
   state.SetConversionFontSizes(CSSToLengthConversionData::FontSizes(
       state.Style(), state.RootElementStyle()));
   state.SetConversionZoom(state.Style()->EffectiveZoom());

third_party/blink/web_tests/external/wpt/css/css-fonts/font-size-monospace-adjust-ref.html

+<!DOCTYPE html>
+<style>
+  textarea { font-size: 3em; }
+</style>
+<textarea>Textarea</textarea>

third_party/blink/web_tests/external/wpt/css/css-fonts/font-size-monospace-adjust.html

+<!DOCTYPE html>
+<link rel="help" href="https://crbug.com/1086082">
+<link rel="match" href="font-size-monospace-adjust-ref.html">
+<style>
+  textarea {
+    font-size: 3em;
+    transition: margin-bottom 1e10s steps(2, start);
+    margin-bottom: 10px;
+  }
+
+  .margin {
+    margin-bottom: 20px;
+  }
+</style>
+<textarea id=textarea>Textarea</textarea>
+<script>
+  document.documentElement.offsetTop;
+  textarea.classList.toggle('margin');
+</script>