Bug 290125 - Create a pref to treat floated ::first-letter more like webkit/blink, not tightly wrapping the glyph extents. r=emilio

Historically, Gecko implemented the behavior allowed by CSS2 whereby a floated ::first-letter is "boxed"
tightly around the glyph shape, rather than using constant font-ascent and -descent metrics which may
leave a lot of blank space depending whether the character has any ascender/descender or not.

However, neither webkit nor blink do this, which leads to webcompat pain when sites are constructed
assuming their behavior.

Eventually, I think we should ideally reimplement ::first-letter entirely at frame-construction time,
rather than during reflow. But in the interest of minimizing risk here, and making it easy to flip
between our existing "legacy" behavior and the new "compatible" behavior, this patch leaves the
overall implementation unchanged and just alters the metrics used for the resulting first-letter
frame.

This patch creates an integer pref layout.css.floating-first-letter.tight-glyph-bounds to allow us
to choose between three behaviors:

    1: Use tight glyph bounds, and ignore line-height; the baseline of the floated letter automatically
       adjusts to wrap text around the "ink box" of the glyph. This is the existing Gecko behavior.

    0: Don't use tight glyph bounds, respect line-height: the floated letter acts like a normal <span>
       with float positioning; baseline position and vertical size are based on font metrics but not
       the specific shape of the individual glyph. This gives a similar result to webkit/blink.

   -1: Automatically choose between (1) and (0) based on heuristics to try and detect whether the page
       was written with the webkit/blink behavior (0) in mind; specifically, if there is a line-height
       of less than 1em, or a negative block-start margin, we assume the author was trying to eliminate
       excess blank space that behavior (0) tends to produce, and so we use that model.

Initially, this patch leaves the behavior unchanged for Beta/Release builds, but enables option -1 (use
heuristics to choose which layout model to apply) on Nightly so we can see how that works in practice.

Differential Revision: https://phabricator.services.mozilla.com/D165008

layout/generic/nsFirstLetterFrame.cpp

@@ -14,12 +14,14 @@
 #include "mozilla/PresShellInlines.h"
 #include "mozilla/RestyleManager.h"
 #include "mozilla/ServoStyleSet.h"
+#include "mozilla/StaticPrefs_layout.h"
 #include "nsIContent.h"
 #include "nsLayoutUtils.h"
 #include "nsLineLayout.h"
 #include "nsGkAtoms.h"
 #include "nsFrameManager.h"
 #include "nsPlaceholderFrame.h"
+#include "nsTextFrame.h"
 #include "nsCSSFrameConstructor.h"
 
 using namespace mozilla;
@@ -138,6 +140,46 @@ nsIFrame::SizeComputationResult nsFirstLetterFrame::ComputeSize(
                                        aSizeOverrides, aFlags);
 }
 
+bool nsFirstLetterFrame::UseTightBounds() const {
+  int v = StaticPrefs::layout_css_floating_first_letter_tight_glyph_bounds();
+
+  // Check for the simple cases:
+  //   pref value > 0: use legacy gecko behavior
+  //   pref value = 0: use webkit/blink-like behavior
+  if (v > 0) {
+    return true;
+  }
+  if (v == 0) {
+    return false;
+  }
+
+  // Pref value < 0: use heuristics to determine whether the page is assuming
+  // webkit/blink-style behavior:
+  // If line-height is less than font-size, or there is a negative block-start
+  // or -end margin, use webkit/blink behavior.
+  if (nsTextFrame* textFrame = do_QueryFrame(mFrames.FirstChild())) {
+    RefPtr<nsFontMetrics> fm = textFrame->InflatedFontMetrics();
+    if (textFrame->ComputeLineHeight() < fm->EmHeight()) {
+      return false;
+    }
+  }
+
+  const auto wm = GetWritingMode();
+  const auto& margin = StyleMargin()->mMargin;
+  const auto& bStart = margin.GetBStart(wm);
+  // Currently, we only check for margins with negative *length* values;
+  // negative percentages seem unlikely to be used/useful in this context.
+  if (bStart.ConvertsToLength() && bStart.ToLength() < 0) {
+    return false;
+  }
+  const auto& bEnd = margin.GetBEnd(wm);
+  if (bEnd.ConvertsToLength() && bEnd.ToLength() < 0) {
+    return false;
+  }
+
+  return true;
+}
+
 void nsFirstLetterFrame::Reflow(nsPresContext* aPresContext,
                                 ReflowOutput& aMetrics,
                                 const ReflowInput& aReflowInput,
@@ -196,11 +238,24 @@ void nsFirstLetterFrame::Reflow(nsPresContext* aPresContext,
 
     // Place and size the child and update the output metrics
     LogicalSize convertedSize = kidMetrics.Size(wm);
-    kid->SetRect(nsRect(bp.IStart(wm), bp.BStart(wm), convertedSize.ISize(wm),
-                        convertedSize.BSize(wm)));
+
+    const bool tightBounds = UseTightBounds();
+    const nscoord shift =
+        tightBounds ? 0
+                    // Shift by half of the difference between the line-height
+                    // we're going to use and current height of the kid frame.
+                    : (rs.GetLineHeight() - convertedSize.BSize(wm)) / 2;
+
+    kid->SetRect(nsRect(bp.IStart(wm), bp.BStart(wm) + shift,
+                        convertedSize.ISize(wm), convertedSize.BSize(wm)));
     kid->FinishAndStoreOverflow(&kidMetrics, rs.mStyleDisplay);
     kid->DidReflow(aPresContext, nullptr);
 
+    if (!tightBounds) {
+      // Adjust size to account for line-height.
+      convertedSize.BSize(wm) = rs.GetLineHeight();
+    }
+
     convertedSize.ISize(wm) += bp.IStartEnd(wm);
     convertedSize.BSize(wm) += bp.BStartEnd(wm);
     aMetrics.SetSize(wm, convertedSize);

layout/generic/nsFirstLetterFrame.h

@@ -79,6 +79,10 @@ class nsFirstLetterFrame final : public nsContainerFrame {
                                            nsIFrame** aContinuation,
                                            bool aIsFluid);
 
+  // Whether to use tight glyph bounds for a floating first-letter frame,
+  // or "loose" bounds based on font metrics rather than individual glyphs.
+  bool UseTightBounds() const;
+
  protected:
   nscoord mBaseline;
 

layout/generic/nsTextFrame.cpp

@@ -52,6 +52,7 @@
 #include "nsDisplayList.h"
 #include "nsIFrame.h"
 #include "nsIMathMLFrame.h"
+#include "nsFirstLetterFrame.h"
 #include "nsPlaceholderFrame.h"
 #include "nsTextFrameUtils.h"
 #include "nsTextRunTransformations.h"
@@ -2233,17 +2234,19 @@ static gfxFontGroup* GetFontGroupForFrame(
   return fontGroup;
 }
 
+nsFontMetrics* nsTextFrame::InflatedFontMetrics() const {
+  if (!mFontMetrics) {
+    float inflation = nsLayoutUtils::FontSizeInflationFor(this);
+    mFontMetrics = nsLayoutUtils::GetFontMetricsForFrame(this, inflation);
+  }
+  return mFontMetrics;
+}
+
 static gfxFontGroup* GetInflatedFontGroupForFrame(nsTextFrame* aFrame) {
   gfxTextRun* textRun = aFrame->GetTextRun(nsTextFrame::eInflated);
   if (textRun) {
     return textRun->GetFontGroup();
   }
-  if (!aFrame->InflatedFontMetrics()) {
-    float inflation = nsLayoutUtils::FontSizeInflationFor(aFrame);
-    RefPtr<nsFontMetrics> metrics =
-        nsLayoutUtils::GetFontMetricsForFrame(aFrame, inflation);
-    aFrame->SetInflatedFontMetrics(metrics);
-  }
   return aFrame->InflatedFontMetrics()->GetThebesFontGroup();
 }
 
@@ -3932,13 +3935,7 @@ void nsTextFrame::PropertyProvider::SetupJustificationSpacing(
 void nsTextFrame::PropertyProvider::InitFontGroupAndFontMetrics() const {
   if (!mFontMetrics) {
     if (mWhichTextRun == nsTextFrame::eInflated) {
-      if (!mFrame->InflatedFontMetrics()) {
-        float inflation = mFrame->GetFontSizeInflation();
-        mFontMetrics = nsLayoutUtils::GetFontMetricsForFrame(mFrame, inflation);
-        mFrame->SetInflatedFontMetrics(mFontMetrics);
-      } else {
       mFontMetrics = mFrame->InflatedFontMetrics();
-      }
     } else {
       mFontMetrics = nsLayoutUtils::GetFontMetricsForFrame(mFrame, 1.0f);
     }
@@ -4921,10 +4918,6 @@ gfxTextRun* nsTextFrame::GetUninflatedTextRun() const {
   return GetProperty(UninflatedTextRunProperty());
 }
 
-void nsTextFrame::SetInflatedFontMetrics(nsFontMetrics* aFontMetrics) {
-  mFontMetrics = aFontMetrics;
-}
-
 void nsTextFrame::SetTextRun(gfxTextRun* aTextRun, TextRunType aWhichTextRun,
                              float aInflation) {
   NS_ASSERTION(aTextRun, "must have text run");
@@ -5871,13 +5864,16 @@ void nsTextFrame::UnionAdditionalOverflow(nsPresContext* aPresContext,
   AddStateBits(TEXT_SELECTION_UNDERLINE_OVERFLOWED);
 }
 
+nscoord nsTextFrame::ComputeLineHeight() const {
+  return ReflowInput::CalcLineHeight(GetContent(), Style(), PresContext(),
+                                     NS_UNCONSTRAINEDSIZE,
+                                     GetFontSizeInflation());
+}
+
 gfxFloat nsTextFrame::ComputeDescentLimitForSelectionUnderline(
     nsPresContext* aPresContext, const gfxFont::Metrics& aFontMetrics) {
-  gfxFloat app = aPresContext->AppUnitsPerDevPixel();
-  nscoord lineHeightApp =
-      ReflowInput::CalcLineHeight(GetContent(), Style(), PresContext(),
-                                  NS_UNCONSTRAINEDSIZE, GetFontSizeInflation());
-  gfxFloat lineHeight = gfxFloat(lineHeightApp) / app;
+  const gfxFloat lineHeight =
+      gfxFloat(ComputeLineHeight()) / aPresContext->AppUnitsPerDevPixel();
   if (lineHeight <= aFontMetrics.maxHeight) {
     return aFontMetrics.maxDescent;
   }
@@ -9707,10 +9703,14 @@ void nsTextFrame::ReflowText(nsLineLayout& aLineLayout, nscoord aAvailableWidth,
 
   // The metrics for the text go in here
   gfxTextRun::Metrics textMetrics;
-  gfxFont::BoundingBoxType boundingBoxType =
-      IsFloatingFirstLetterChild() || IsInitialLetterChild()
-          ? gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS
-          : gfxFont::LOOSE_INK_EXTENTS;
+  gfxFont::BoundingBoxType boundingBoxType = gfxFont::LOOSE_INK_EXTENTS;
+  if (IsFloatingFirstLetterChild() || IsInitialLetterChild()) {
+    if (nsFirstLetterFrame* firstLetter = do_QueryFrame(GetParent())) {
+      if (firstLetter->UseTightBounds()) {
+        boundingBoxType = gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS;
+      }
+    }
+  }
 
   int32_t limitLength = length;
   int32_t forceBreak = aLineLayout.GetForcedBreakPosition(this);

layout/generic/nsTextFrame.h

@@ -723,6 +723,8 @@ class nsTextFrame : public nsIFrame {
                   DrawTarget* aDrawTarget, ReflowOutput& aMetrics,
                   nsReflowStatus& aStatus);
 
+  nscoord ComputeLineHeight() const;
+
   bool IsFloatingFirstLetterChild() const;
 
   bool IsInitialLetterChild() const;
@@ -745,8 +747,7 @@ class nsTextFrame : public nsIFrame {
    */
   void NotifyNativeAnonymousTextnodeChange(uint32_t aOldLength);
 
-  void SetInflatedFontMetrics(nsFontMetrics*);
-  nsFontMetrics* InflatedFontMetrics() const { return mFontMetrics; }
+  nsFontMetrics* InflatedFontMetrics() const;
 
   nsRect WebRenderBounds();
 
@@ -762,7 +763,7 @@ class nsTextFrame : public nsIFrame {
   friend class mozilla::nsDisplayTextGeometry;
   friend class mozilla::nsDisplayText;
 
-  RefPtr<nsFontMetrics> mFontMetrics;
+  mutable RefPtr<nsFontMetrics> mFontMetrics;
   RefPtr<gfxTextRun> mTextRun;
   nsTextFrame* mNextContinuation;
   // The key invariant here is that mContentOffset never decreases along

modules/libpref/init/StaticPrefList.yaml

@@ -8330,6 +8330,22 @@
   mirror: always
   rust: true
 
+# Whether to use tight bounds for floating ::first-letter (legacy Gecko behavior)
+# or loose bounds based on overall font metrics (WebKit/Blink-like behavior)?
+# Values mean:
+#     1   legacy Gecko behavior (tight bounds)
+#     0   loose typographic bounds (similar to webkit/blink)
+#    -1   auto behavior: use loose bounds if reduced line-height (<1em) or negative
+#         block-start margin is present; otherwise use tight bounds.
+- name: layout.css.floating-first-letter.tight-glyph-bounds
+  type: int32_t
+#ifdef NIGHTLY_BUILD
+  value: -1
+#else
+  value: 1
+#endif
+  mirror: always
+
 # Is support for the font-display @font-face descriptor enabled?
 - name: layout.css.font-display.enabled
   type: RelaxedAtomicBool