Last Comment Bug 223764 - almost-standards mode underline weirdness
: almost-standards mode underline weirdness
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Vitor Menezes
:
:
Mentors:
: 262941 337568 368785 469371 (view as bug list)
Depends on:
Blocks: 403524
  Show dependency treegraph
 
Reported: 2003-10-26 14:31 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-08-04 02:58 PDT (History)
14 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (286 bytes, text/html)
2003-10-26 14:33 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
screenshot (29.55 KB, image/png)
2003-10-26 14:34 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testacase (194 bytes, text/html)
2011-07-28 14:53 PDT, Vitor Menezes
no flags Details
Proposed patch (8.21 KB, patch)
2011-07-30 06:42 PDT, Vitor Menezes
dbaron: review+
Details | Diff | Splinter Review
Proposed patch rev 1 (12.29 KB, patch)
2011-08-01 15:38 PDT, Vitor Menezes
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2003-10-26 14:31:52 PST
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 [:bz] (still a bit busy) 2003-10-26 14:33:45 PST
Created attachment 134208 [details]
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 [:bz] (still a bit busy) 2003-10-26 14:34:39 PST
Created attachment 134209 [details]
screenshot
Comment 3 Daniel.S 2008-12-21 08:35:16 PST
*** Bug 262941 has been marked as a duplicate of this bug. ***
Comment 4 Daniel.S 2008-12-27 12:55:11 PST
*** Bug 368785 has been marked as a duplicate of this bug. ***
Comment 5 Daniel.S 2009-01-31 14:24:25 PST
*** Bug 337568 has been marked as a duplicate of this bug. ***
Comment 6 Daniel.S 2009-03-21 02:32:59 PDT
*** Bug 469371 has been marked as a duplicate of this bug. ***
Comment 7 Daniel.S 2009-03-21 02:37:13 PDT
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-7 (busy September 14-25) 2011-07-28 14:34:49 PDT
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-7 (busy September 14-25) 2011-07-28 14:36:50 PDT
I'm inclined to think we should do that by just giving nsInlineFrame an mBaseline member variable.
Comment 10 Vitor Menezes 2011-07-28 14:53:42 PDT
Created attachment 549235 [details]
Testacase

Fix pesky p tag
Comment 11 Vitor Menezes 2011-07-30 06:42:28 PDT
Created attachment 549576 [details] [diff] [review]
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-7 (busy September 14-25) 2011-07-30 08:08:46 PDT
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-7 (busy September 14-25) 2011-07-30 08:18:40 PDT
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 2011-08-01 15:38:03 PDT
Created attachment 549952 [details] [diff] [review]
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-7 (busy September 14-25) 2011-08-03 11:35:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcf967393a0
Comment 16 Marco Bonardo [::mak] 2011-08-04 02:58:12 PDT
http://hg.mozilla.org/mozilla-central/rev/1dcf967393a0

Note You need to log in before you can comment on or make changes to this bug.