223764 - almost-standards mode underline weirdness

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This seems like an undesirable interaction of the quirks inline model with
standards-mode text-decorations.  Testcase coming up; all the underlines in the
testcase should be at the same level.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Things are fine in full standards mode (underlines all level).	In quirks mode
they don't quite line up, but quirks is just all weird.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: screenshot

Comment 7

[Daniel.S]

This seems to cause problems every once in a while. Someone should look into this, if there's time.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So I think the problem here is actually something that will be made worse by bug 403524 -- since bug 403524 will make this happen in quirks mode too.

The problem here is, I think, as follows:

The middle two <a> elements (assuming fixing the "<p") have, in nsLineLayout.cpp, the zeroEffectiveSpanBox flag set, so in nsLineLayout::VerticalAlignFrames we go through this code:

    if (minY > 0) {

      // shrink the content by moving its top down.  This is tricky, since
      // the top is the 0 for many coordinates, so what we do is
      // move everything else up.
      spanFramePFD->mAscent -= minY; // move the baseline up
      spanFramePFD->mBounds.height -= minY; // move the bottom up
      psd->mTopLeading += minY;

      pfd = psd->mFirstFrame;
      while (nsnull != pfd) {
        pfd->mBounds.y -= minY; // move all the children back up
        pfd->mFrame->SetRect(pfd->mBounds);
        pfd = pfd->mNext;
      }
      maxY -= minY; // since minY is in the frame's own coordinate system
      minY = 0;
    }

However, we then forget about this adjustment we've made to the frame's ascent, since nsInlineFrame.cpp has:

nscoord
nsInlineFrame::GetBaseline() const
{
  nscoord ascent = 0;
  nsRefPtr<nsFontMetrics> fm;
  nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fm));
  if (fm) {
    ascent = fm->MaxAscent();
  }
  return NS_MIN(mRect.height, ascent + GetUsedBorderAndPadding().top);
}


This bug will be fixed by somehow reflecting that adjustment in the ascent in the result of nsInlineFrame::GetBaseline.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm inclined to think we should do that by just giving nsInlineFrame an mBaseline member variable.

Comment 10

[Vitor Menezes]

Attachment: Testacase

Fix pesky p tag

Comment 11

[Vitor Menezes]

Attachment: Proposed patch

Adding the fields as described seems to fix it, but it'd be nice to get something reftest-able.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 549576 [details] [diff] [review]
Proposed patch

>diff --git a/layout/generic/nsFirstLetterFrame.cpp b/layout/generic/nsFirstLetterFrame.cpp

>   aMetrics.width += lr;
>   aMetrics.height += tb;
>   aMetrics.ascent += bp.top;
>-  mBaseline = aMetrics.ascent;

Actually, this is still needed for the !aReflowState.mLineLayout case, so you should just leave it rather than removing it, but probably add a comment "// for the floating first-letter case".

>diff --git a/layout/generic/nsInlineFrame.h b/layout/generic/nsInlineFrame.h
>--- a/layout/generic/nsInlineFrame.h
>+++ b/layout/generic/nsInlineFrame.h
>@@ -62,16 +62,19 @@ class nsAnonymousBlockFrame;
> /**
>  * Inline frame class.
>  *
>  * This class manages a list of child frames that are inline frames. Working with
>  * nsLineLayout, the class will reflow and place inline frames on a line.
>  */
> class nsInlineFrame : public nsInlineFrameSuper
> {
>+protected:
>+  nscoord mBaseline;
>+

Could you put this definition at the very end of the class instead of the very beginning?  That's where we tend to  put member variables.

> nsresult
> nsLineLayout::BeginSpan(nsIFrame* aFrame,
>                         const nsHTMLReflowState* aSpanReflowState,
>                         nscoord aLeftEdge,
>-                        nscoord aRightEdge)
>+                        nscoord aRightEdge,
>+                        nscoord* aBaseline)
> {
>   NS_ASSERTION(aRightEdge != NS_UNCONSTRAINEDSIZE,
>                "should no longer be using unconstrained sizes");
> #ifdef NOISY_REFLOW
>   nsFrame::IndentBy(stdout, mSpanDepth+1);
>   nsFrame::ListTag(stdout, aFrame);
>   printf(": BeginSpan leftEdge=%d rightEdge=%d\n", aLeftEdge, aRightEdge);
> #endif
> 
>   PerSpanData* psd;
>   nsresult rv = NewPerSpanData(&psd);
>+
>   if (NS_SUCCEEDED(rv)) {
>+    psd->mBaseline = aBaseline;
>+

Could you put this somewhere in the "Init new span" section 5 lines below instead of here (and skip the extra blank lines)?

r=dbaron with that

(And once you address those, could you push the update to https://hg.mozilla.org/users/vmenezes_mozilla.com/patches/ ?)

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

And probably the simplest way to reftest this is to do a != test comparing:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
        "http://www.w3.org/TR/html4/loose.dtd">
<title>test for bug 223764</title>
<p style="overflow:hidden"><span style="font-size: 100px; text-decoration:underline"><span style="font-size: 20px">hello</span></span></p>

</body>
</html>

to the same thing without the underline (since with the bug, the underline is way outside the box and is cut off with overflow:hidden).

Probably best to have the test both as above (in almost-standards mode) and in quirks mode.

Comment 14

[Vitor Menezes]

Attachment: Proposed patch rev 1

Added reftests for quirks and almost-standards modes. Also made code comply with dbaron's suggestions.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcf967393a0

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1dcf967393a0