Bug 403524: Make quirks-mode text draw order comply with CSS 2.1

Quirks-mode code draws text, and then all decorations. We need to instead draw
underlines, then overlines, -then- text, then line-throughs, as per CSS 2.1.

This involves refactoring nsTextFrame::PaintTextDecorations and
nsTextFrame::DrawText by merging them together, and also updating some
of their callers.

layout/generic/nsTextFrame.h

@@ -299,14 +299,6 @@ public:
   // The private DrawText() is what applies the text to a graphics context
   void PaintText(nsRenderingContext* aRenderingContext, nsPoint aPt,
                  const nsRect& aDirtyRect, const nsCharClipDisplayItem& aItem);
-  // helper: paint quirks-mode CSS text decorations
-  void PaintTextDecorations(gfxContext* aCtx, const gfxRect& aDirtyRect,
-                            const gfxPoint& aFramePt,
-                            const gfxPoint& aTextBaselinePt,
-                            nsTextPaintStyle& aTextStyle,
-                            PropertyProvider& aProvider,
-                            const nsCharClipDisplayItem::ClipEdges& aClipEdges,
-                            const nscolor* aOverrideColor = nsnull);
   // helper: paint text frame when we're impacted by at least one selection.
   // Return false if the text was not painted and we should continue with
   // the fast path.
@@ -333,7 +325,8 @@ public:
                                     PRUint32 aContentLength,
                                     nsTextPaintStyle& aTextPaintStyle,
                                     SelectionDetails* aDetails,
-                                    SelectionType* aAllTypes);
+                                    SelectionType* aAllTypes,
+                            const nsCharClipDisplayItem::ClipEdges& aClipEdges);
   // helper: paint text decorations for text selected by aSelectionType
   void PaintTextSelectionDecorations(gfxContext* aCtx,
                                      const gfxPoint& aFramePt,
@@ -445,15 +438,6 @@ protected:
                                nsRect* aVisualOverflowRect,
                                bool aIncludeTextDecorations);
 
-  void DrawText(gfxContext* aCtx,
-                const gfxPoint& aTextBaselinePt,
-                PRUint32 aOffset,
-                PRUint32 aLength,
-                const gfxRect* aDirtyRect,
-                PropertyProvider* aProvider,
-                gfxFloat& aAdvanceWidth,
-                PRBool aDrawSoftHyphen);
-
   void PaintOneShadow(PRUint32 aOffset,
                       PRUint32 aLength,
                       nsCSSShadowItem* aShadowDetails,
@@ -521,6 +505,39 @@ protected:
   void GetTextDecorations(nsPresContext* aPresContext,
                           TextDecorations& aDecorations);
 
+  void DrawTextRun(gfxContext* const aCtx,
+                   const gfxPoint& aTextBaselinePt,
+                   PRUint32 aOffset,
+                   PRUint32 aLength,
+                   PropertyProvider& aProvider,
+                   gfxFloat& aAdvanceWidth,
+                   PRBool aDrawSoftHyphen);
+
+  void DrawTextRunAndDecorations(gfxContext* const aCtx,
+                                 const gfxPoint& aFramePt,
+                                 const gfxPoint& aTextBaselinePt,
+                                 PRUint32 aOffset,
+                                 PRUint32 aLength,
+                                 PropertyProvider& aProvider,
+                                 const nsTextPaintStyle& aTextStyle,
+                             const nsCharClipDisplayItem::ClipEdges& aClipEdges,
+                                 gfxFloat& aAdvanceWidth,
+                                 PRBool aDrawSoftHyphen,
+                                 const TextDecorations& aDecorations,
+                                 const nscolor* const aDecorationOverrideColor);
+
+  void DrawText(gfxContext* const aCtx,
+                const gfxPoint& aFramePt,
+                const gfxPoint& aTextBaselinePt,
+                PRUint32 aOffset,
+                PRUint32 aLength,
+                PropertyProvider& aProvider,
+                const nsTextPaintStyle& aTextStyle,
+                const nsCharClipDisplayItem::ClipEdges& aClipEdges,
+                gfxFloat& aAdvanceWidth,
+                PRBool aDrawSoftHyphen,
+                const nscolor* const aDecorationOverrideColor = nsnull);
+
   // Set non empty rect to aRect, it should be overflow rect or frame rect.
   // If the result rect is larger than the given rect, this returns PR_TRUE.
   PRBool CombineSelectionUnderlineRect(nsPresContext* aPresContext,

layout/generic/nsTextFrameThebes.cpp

@@ -326,7 +326,7 @@ public:
                                       float* aRelativeSize,
                                       PRUint8* aStyle);
 
-  nsPresContext* PresContext() { return mPresContext; }
+  nsPresContext* PresContext() const { return mPresContext; }
 
   enum {
     eIndexRawInput = 0,
@@ -4472,81 +4472,6 @@ nsTextFrame::UnionAdditionalOverflow(nsPresContext* aPresContext,
   AddStateBits(TEXT_SELECTION_UNDERLINE_OVERFLOWED);
 }
 
-void 
-nsTextFrame::PaintTextDecorations(
-               gfxContext* aCtx, const gfxRect& aDirtyRect,
-               const gfxPoint& aFramePt,
-               const gfxPoint& aTextBaselinePt,
-               nsTextPaintStyle& aTextPaintStyle,
-               PropertyProvider& aProvider,
-               const nsCharClipDisplayItem::ClipEdges& aClipEdges,
-               const nscolor* aOverrideColor)
-{
-  TextDecorations decorations;
-  GetTextDecorations(aTextPaintStyle.PresContext(), decorations);
-
-  if (!decorations.HasDecorationLines())
-    return;
-
-  // Hide text decorations if we're currently hiding @font-face fallback text
-  if (aProvider.GetFontGroup()->ShouldSkipDrawing())
-    return;
-
-  const gfxFloat app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
-
-  // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
-  nscoord x = NSToCoordRound(aFramePt.x);
-  nscoord width = GetRect().width;
-  aClipEdges.Intersect(&x, &width);
-
-  gfxPoint pt(x / app, 0);
-  gfxSize size(width / app, 0);
-  const gfxFloat ascent = gfxFloat(mAscent) / app;
-  const gfxFloat frameTop = aFramePt.y;
-
-  nscolor lineColor;
-  for (PRUint32 i = decorations.mUnderlines.Length(); i-- > 0; ) {
-    const LineDecoration& dec = decorations.mUnderlines[i];
-    gfxFontGroup* fontGroup = GetFontGroupForFrame(dec.mFrame);
-    const gfxFont::Metrics metrics = GetFirstFontMetrics(fontGroup);
-
-    size.height = metrics.underlineSize;
-    pt.y = (frameTop - dec.mBaselineOffset) / app;
-
-    lineColor = aOverrideColor ? *aOverrideColor : dec.mColor;
-    nsCSSRendering::PaintDecorationLine(aCtx, lineColor, pt, size, ascent,
-      fontGroup->GetUnderlineOffset(), NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE,
-      dec.mStyle);
-  }
-  for (PRUint32 i = decorations.mOverlines.Length(); i-- > 0; ) {
-    const LineDecoration& dec = decorations.mOverlines[i];
-
-    const gfxFont::Metrics metrics =
-      GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
-
-    size.height = metrics.underlineSize;
-    pt.y = (frameTop - dec.mBaselineOffset) / app;
-
-    lineColor = aOverrideColor ? *aOverrideColor : dec.mColor;
-    nsCSSRendering::PaintDecorationLine(aCtx, lineColor, pt, size, ascent,
-      metrics.maxAscent, NS_STYLE_TEXT_DECORATION_LINE_OVERLINE, dec.mStyle);
-  }
-  for (PRUint32 i = decorations.mStrikes.Length(); i-- > 0; ) {
-    const LineDecoration& dec = decorations.mStrikes[i];
-
-    const gfxFont::Metrics metrics =
-      GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
-
-    size.height = metrics.strikeoutSize;
-    pt.y = (frameTop - dec.mBaselineOffset) / app;
-
-    lineColor = aOverrideColor ? *aOverrideColor : dec.mColor;
-    nsCSSRendering::PaintDecorationLine(aCtx, lineColor, pt, size, ascent,
-      metrics.strikeoutOffset, NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH,
-      dec.mStyle);
-  }
-}
-
 static gfxFloat
 ComputeDescentLimitForSelectionUnderline(nsPresContext* aPresContext,
                                          nsTextFrame* aFrame,
@@ -4919,20 +4844,11 @@ nsTextFrame::PaintOneShadow(PRUint32 aOffset, PRUint32 aLength,
   // Draw the text onto our alpha-only surface to capture the alpha values.
   // Remember that the box blur context has a device offset on it, so we don't need to
   // translate any coordinates to fit on the surface.
-  gfxRect dirtyGfxRect(aDirtyRect.x, aDirtyRect.y, aDirtyRect.width, aDirtyRect.height);
   gfxFloat advanceWidth;
-  DrawText(shadowContext,
-           aTextBaselinePt + shadowOffset,
-           aOffset, aLength, &dirtyGfxRect, aProvider, advanceWidth,
-           (GetStateBits() & TEXT_HYPHEN_BREAK) != 0);
-
-  // This will only have an effect in quirks mode. Standards mode text-decoration shadow painting
-  // is handled in nsHTMLContainerFrame.cpp, so you must remember to consider that if you change
-  // any code behaviour here.
-  nsTextPaintStyle textPaintStyle(this);
-  PaintTextDecorations(shadowContext, dirtyGfxRect, aFramePt + shadowOffset,
-                       aTextBaselinePt + shadowOffset,
-                       textPaintStyle, *aProvider, aClipEdges, &shadowColor);
+  DrawText(shadowContext, aFramePt + shadowOffset,
+           aTextBaselinePt + shadowOffset, aOffset, aLength, *aProvider,
+           nsTextPaintStyle(this), aClipEdges, advanceWidth,
+           (GetStateBits() & TEXT_HYPHEN_BREAK) != 0, &shadowColor);
 
   contextBoxBlur.DoPaint();
   aCtx->Restore();
@@ -4942,12 +4858,13 @@ nsTextFrame::PaintOneShadow(PRUint32 aOffset, PRUint32 aLength,
 // aAllTypes, the union of all selection types that are applying to this text.
 bool
 nsTextFrame::PaintTextWithSelectionColors(gfxContext* aCtx,
-    const gfxPoint& aFramePt,
-    const gfxPoint& aTextBaselinePt, const gfxRect& aDirtyRect,
+    const gfxPoint& aFramePt, const gfxPoint& aTextBaselinePt,
+    const gfxRect& aDirtyRect,
     PropertyProvider& aProvider,
     PRUint32 aContentOffset, PRUint32 aContentLength,
     nsTextPaintStyle& aTextPaintStyle, SelectionDetails* aDetails,
-    SelectionType* aAllTypes)
+    SelectionType* aAllTypes,
+    const nsCharClipDisplayItem::ClipEdges& aClipEdges)
 {
   // Figure out which selections control the colors to use for each character.
   nsAutoTArray<SelectionDetails*,BIG_TEXT_NODE_SIZE> prevailingSelectionsBuffer;
@@ -5039,9 +4956,9 @@ nsTextFrame::PaintTextWithSelectionColors(gfxContext* aCtx,
     aCtx->SetColor(gfxRGBA(foreground));
     gfxFloat advance;
 
-    DrawText(aCtx, gfxPoint(aFramePt.x + xOffset, aTextBaselinePt.y),
-             offset, length, &aDirtyRect, &aProvider,
-             advance, hyphenWidth > 0);
+    DrawText(aCtx, aFramePt, gfxPoint(aFramePt.x + xOffset, aTextBaselinePt.y),
+             offset, length, aProvider, aTextPaintStyle, aClipEdges, advance,
+             hyphenWidth > 0);
     if (hyphenWidth) {
       advance += hyphenWidth;
     }
@@ -5122,7 +5039,8 @@ bool
 nsTextFrame::PaintTextWithSelection(gfxContext* aCtx,
     const gfxPoint& aFramePt,
     const gfxPoint& aTextBaselinePt, const gfxRect& aDirtyRect,
-    PropertyProvider& aProvider, PRUint32 aContentOffset, PRUint32 aContentLength,
+    PropertyProvider& aProvider,
+    PRUint32 aContentOffset, PRUint32 aContentLength,
     nsTextPaintStyle& aTextPaintStyle,
     const nsCharClipDisplayItem::ClipEdges& aClipEdges)
 {
@@ -5140,12 +5058,12 @@ nsTextFrame::PaintTextWithSelection(gfxContext* aCtx,
   SelectionType allTypes;
   if (!PaintTextWithSelectionColors(aCtx, aFramePt, aTextBaselinePt, aDirtyRect,
                                     aProvider, aContentOffset, aContentLength,
-                                    aTextPaintStyle, details, &allTypes)) {
+                                    aTextPaintStyle, details, &allTypes,
+                                    aClipEdges))
+  {
     DestroySelectionDetails(details);
     return false;
   }
-  PaintTextDecorations(aCtx, aDirtyRect, aFramePt, aTextBaselinePt,
-                       aTextPaintStyle, aProvider, aClipEdges);
   PRInt32 i;
   // Iterate through just the selection types that paint decorations and
   // paint decorations for any that actually occur in this frame. Paint
@@ -5389,21 +5307,21 @@ nsTextFrame::PaintText(nsRenderingContext* aRenderingContext, nsPoint aPt,
   ctx->SetColor(gfxRGBA(foregroundColor));
 
   gfxFloat advanceWidth;
-  DrawText(ctx, textBaselinePt, startOffset, maxLength, &dirtyRect, &provider,
-           advanceWidth, (GetStateBits() & TEXT_HYPHEN_BREAK) != 0);
-  PaintTextDecorations(ctx, dirtyRect, framePt, textBaselinePt,
-                       textPaintStyle, provider, clipEdges);
+  DrawText(ctx, framePt, textBaselinePt, startOffset, maxLength, provider,
+           textPaintStyle, clipEdges, advanceWidth,
+           (GetStateBits() & TEXT_HYPHEN_BREAK) != 0);
 }
 
 void
-nsTextFrame::DrawText(gfxContext* aCtx, const gfxPoint& aTextBaselinePt,
-                      PRUint32 aOffset, PRUint32 aLength,
-                      const gfxRect* aDirtyRect, PropertyProvider* aProvider,
-                      gfxFloat& aAdvanceWidth, PRBool aDrawSoftHyphen)
+nsTextFrame::DrawTextRun(gfxContext* const aCtx,
+                         const gfxPoint& aTextBaselinePt,
+                         PRUint32 aOffset, PRUint32 aLength,
+                         PropertyProvider& aProvider,
+                         gfxFloat& aAdvanceWidth,
+                         PRBool aDrawSoftHyphen)
 {
-  // Paint the text and soft-hyphen (if any) onto the given graphics context
   mTextRun->Draw(aCtx, aTextBaselinePt, aOffset, aLength,
-                 aProvider, &aAdvanceWidth);
+                 &aProvider, &aAdvanceWidth);
 
   if (aDrawSoftHyphen) {
     // Don't use ctx as the context, because we need a reference context here,
@@ -5420,6 +5338,112 @@ nsTextFrame::DrawText(gfxContext* aCtx, const gfxPoint& aTextBaselinePt,
   }
 }
 
+void
+nsTextFrame::DrawTextRunAndDecorations(
+    gfxContext* const aCtx,
+    const gfxPoint& aFramePt, const gfxPoint& aTextBaselinePt,
+    PRUint32 aOffset, PRUint32 aLength,
+    PropertyProvider& aProvider,
+    const nsTextPaintStyle& aTextStyle,
+    const nsCharClipDisplayItem::ClipEdges& aClipEdges,
+    gfxFloat& aAdvanceWidth,
+    PRBool aDrawSoftHyphen,
+    const TextDecorations& aDecorations,
+    const nscolor* const aDecorationOverrideColor)
+{
+    const gfxFloat app = aTextStyle.PresContext()->AppUnitsPerDevPixel();
+
+    // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
+    nscoord x = NSToCoordRound(aFramePt.x);
+    nscoord width = GetRect().width;
+    aClipEdges.Intersect(&x, &width);
+
+    gfxPoint decPt(x / app, 0);
+    gfxSize decSize(width / app, 0);
+    const gfxFloat ascent = gfxFloat(mAscent) / app;
+    const gfxFloat frameTop = aFramePt.y;
+
+    // Underlines
+    for (PRUint32 i = aDecorations.mUnderlines.Length(); i-- > 0; ) {
+      const LineDecoration& dec = aDecorations.mUnderlines[i];
+
+      const gfxFont::Metrics metrics =
+        GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
+
+      decSize.height = metrics.underlineSize;
+      decPt.y = (frameTop - dec.mBaselineOffset) / app;
+
+      const nscolor lineColor = aDecorationOverrideColor ? *aDecorationOverrideColor : dec.mColor;
+      nsCSSRendering::PaintDecorationLine(aCtx, lineColor, decPt, decSize, ascent,
+        metrics.underlineOffset, NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE,
+        dec.mStyle);
+    }
+    // Overlines
+    for (PRUint32 i = aDecorations.mOverlines.Length(); i-- > 0; ) {
+      const LineDecoration& dec = aDecorations.mOverlines[i];
+
+      const gfxFont::Metrics metrics =
+        GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
+
+      decSize.height = metrics.underlineSize;
+      decPt.y = (frameTop - dec.mBaselineOffset) / app;
+
+      const nscolor lineColor = aDecorationOverrideColor ? *aDecorationOverrideColor : dec.mColor;
+      nsCSSRendering::PaintDecorationLine(aCtx, lineColor, decPt, decSize, ascent,
+        metrics.maxAscent, NS_STYLE_TEXT_DECORATION_LINE_OVERLINE, dec.mStyle);
+    }
+
+    // CSS 2.1 mandates that text be painted after over/underlines, and *then*
+    // line-throughs
+    DrawTextRun(aCtx, aTextBaselinePt, aOffset, aLength, aProvider, aAdvanceWidth,
+                aDrawSoftHyphen);
+
+    // Line-throughs
+    for (PRUint32 i = aDecorations.mStrikes.Length(); i-- > 0; ) {
+      const LineDecoration& dec = aDecorations.mStrikes[i];
+
+      const gfxFont::Metrics metrics =
+        GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
+
+      decSize.height = metrics.strikeoutSize;
+      decPt.y = (frameTop - dec.mBaselineOffset) / app;
+
+      const nscolor lineColor = aDecorationOverrideColor ? *aDecorationOverrideColor : dec.mColor;
+      nsCSSRendering::PaintDecorationLine(aCtx, lineColor, decPt, decSize, ascent,
+        metrics.strikeoutOffset, NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH,
+        dec.mStyle);
+    }
+}
+
+void
+nsTextFrame::DrawText(
+    gfxContext* const aCtx,
+    const gfxPoint& aFramePt, const gfxPoint& aTextBaselinePt,
+    PRUint32 aOffset, PRUint32 aLength,
+    PropertyProvider& aProvider,
+    const nsTextPaintStyle& aTextStyle,
+    const nsCharClipDisplayItem::ClipEdges& aClipEdges,
+    gfxFloat& aAdvanceWidth,
+    PRBool aDrawSoftHyphen,
+    const nscolor* const aDecorationOverrideColor)
+{
+  TextDecorations decorations;
+  GetTextDecorations(aTextStyle.PresContext(), decorations);
+
+  // Hide text decorations if we're currently hiding @font-face fallback text
+  const bool drawDecorations = !aProvider.GetFontGroup()->ShouldSkipDrawing() &&
+                               decorations.HasDecorationLines();
+  if (drawDecorations) {
+    DrawTextRunAndDecorations(aCtx, aFramePt, aTextBaselinePt, aOffset, aLength,
+                              aProvider, aTextStyle, aClipEdges, aAdvanceWidth,
+                              aDrawSoftHyphen, decorations,
+                              aDecorationOverrideColor);
+  } else {
+    DrawTextRun(aCtx, aTextBaselinePt, aOffset, aLength, aProvider,
+                aAdvanceWidth, aDrawSoftHyphen);
+  }
+}
+
 PRInt16
 nsTextFrame::GetSelectionStatus(PRInt16* aSelectionFlags)
 {

layout/reftests/text-decoration/reftest.list

@@ -83,7 +83,7 @@ fails == underline-block-propagation-2-quirks.html underline-block-propagation-2
 fails == underline-block-propagation-standards.html underline-block-propagation-standards-ref.html # bug that decoration is drawn through non-text child (bug 428599)
 fails-if(Android) fails-if(d2d) == underline-block-propagation-2-standards.html underline-block-propagation-2-standards-ref.html # bug 585684
 == text-decoration-zorder-1-standards.html text-decoration-zorder-1-ref.html
-fails == text-decoration-zorder-1-quirks.html text-decoration-zorder-1-ref.html # bug 403524
+== text-decoration-zorder-1-quirks.html text-decoration-zorder-1-ref.html # bug 403524
 == table-quirk-1.html table-quirk-1-ref.html
 == table-quirk-2.html table-quirk-2-ref.html
 == text-decoration-propagation-1-quirks.html text-decoration-propagation-1-quirks-ref.html