Closed Bug 223764 Opened 17 years ago Closed 9 years ago

almost-standards mode underline weirdness

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: bzbarsky, Assigned: vmenezes)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached file Testcase (obsolete) —
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.
Attached image screenshot
Attachment #134208 - Attachment is patch: false
Attachment #134208 - Attachment mime type: text/plain → text/html
Duplicate of this bug: 262941
Duplicate of this bug: 368785
Duplicate of this bug: 337568
Duplicate of this bug: 469371
This seems to cause problems every once in a while. Someone should look into this, if there's time.
Assignee: layout.fonts-and-text → nobody
Flags: wanted1.9.2?
OS: Linux → All
QA Contact: ian → layout.fonts-and-text
Hardware: x86 → All
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Depends on: 403524
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.
I'm inclined to think we should do that by just giving nsInlineFrame an mBaseline member variable.
Attached file Testacase
Fix pesky p tag
Attachment #134208 - Attachment is obsolete: true
Assignee: nobody → vmenezes
Attached patch Proposed patch (obsolete) — Splinter Review
Adding the fields as described seems to fix it, but it'd be nice to get something reftest-able.
Attachment #549576 - Flags: review?(dbaron)
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/ ?)
Attachment #549576 - Flags: review?(dbaron) → review+
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.
Added reftests for quirks and almost-standards modes. Also made code comply with dbaron's suggestions.
Attachment #549576 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/1dcf967393a0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 1466564
You need to log in before you can comment on or make changes to this bug.