almost-standards mode underline weirdness

RESOLVED FIXED in mozilla8

Status

()

Core
Layout: Text
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: bz, Assigned: Vitor Menezes)

Tracking

Trunk
mozilla8
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
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.
(Reporter)

Comment 2

14 years ago
Created attachment 134209 [details]
screenshot
(Reporter)

Updated

14 years ago
Attachment #134208 - Attachment is patch: false
Attachment #134208 - Attachment mime type: text/plain → text/html

Updated

9 years ago
Duplicate of this bug: 262941

Updated

9 years ago
Duplicate of this bug: 368785

Updated

9 years ago
Duplicate of this bug: 337568

Updated

9 years ago
Duplicate of this bug: 469371

Comment 7

9 years ago
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-

Updated

7 years ago
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.
(Assignee)

Comment 10

6 years ago
Created attachment 549235 [details]
Testacase

Fix pesky p tag
Attachment #134208 - Attachment is obsolete: true
Blocks: 403524
No longer depends on: 403524
(Assignee)

Updated

6 years ago
Assignee: nobody → vmenezes
(Assignee)

Comment 11

6 years ago
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.
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.
(Assignee)

Comment 14

6 years ago
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.
Attachment #549576 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcf967393a0
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/1dcf967393a0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.