Closed Bug 238473 Opened 18 years ago Closed 15 years ago

If inline element has padding and white space, its text-decoration is broken.

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: hsaito54, Assigned: hsaito54)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

When inline element has white space and border padding, its text-decoration is
broken, because the element separates into two or more lines.
This bug may be fixed by bug 236966, however I'll attach a patch.

We have following test case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2126&action=view
Attached patch patch (obsolete) — Splinter Review
Attached patch patch2 (obsolete) — Splinter Review
Attachment #144639 - Attachment is obsolete: true
At a guess, adding an extra member to all frames will not be accepted.

Why are we rendering the text-decoration on the frame before the split as we
are? It works when there is some text before the split, no?
Attached patch patch3 (obsolete) — Splinter Review
> At a guess, adding an extra member to all frames will not be accepted.

I moved an extra member to a class of nsHTMLContainerFrame.
Attachment #144650 - Attachment is obsolete: true
Do nsIFrame::GetNextInFlow, nsIFrame::GetPrevInFlow, or perhaps
nsIFrame::GetSkipSides not give you the information you want?

A summary of what they are:

When we break lines or pages, some frames are split -- and when a frame is split
the pieces are doubly linked, and the list can be accessed through GetPrevInFlow
and GetNextInFlow.  GetNextSibling is really something managed more by the
parent of the frame -- since the child list is stored as a linked list.  For
example, if we have the markup:

  <p><span><b>Some text.</b></span></p>

that gets split over two lines (at the space), then the |nsInlineFrame|s for
both the span and the b are split, as is the |nsTextFrame| for the text.  The
split frames are all next-in-flows/prev-in-flows.  However, GetNextSibling on
the first frame for the span should return the second part of the span (since
they're both children of the |nsBlockFrame| for the p), but GetNextSibling on
the first frame for the b should return null, since the first frame for the span
has only one child (the first frame for the b).  So a frame's next-in-flow is
only sometimes its sibling.

(I think that's right, anyway.)

nsIFrame::GetSkipSides says which sides of the border should not be drawn.


I haven't looked at this bug in detail, but it seems like it could be fixed
without doing what you did above.
Attachment #144788 - Flags: review-
I think you could just call |nsIFrame::GetSkipSides| in nsHTMLContainerFrame.cpp
and use the result to set some members of |bp| to |0|.
(In reply to comment #3)
> Why are we rendering the text-decoration on the frame before the split as we
> are? It works when there is some text before the split, no?

I think we're passing (due to this bug) a negative width to FillRect that is
wrapped around to a positive width.  Note that although the lines don't always
appear out to the right edge if you scroll right, if you invalidate the whole
window and the beginning of the inline is still visible, they do.
Attached patch patch4 (obsolete) — Splinter Review
dbaron, thanks for your many information.

I used not GetSkipSides but GetNextInFlow and GetPrevInFlow, because there was
empty frame (i.e. both height and width are zero) when the window was resized.
Attachment #144788 - Attachment is obsolete: true
Attached patch patch5 (obsolete) — Splinter Review
I'm sorry, patch4 is wrong.
Attachment #144826 - Attachment is obsolete: true
Comment on attachment 144832 [details] [diff] [review]
patch5

This seems reasonable at the detailed level, except for two things:

>+  nscoord leftBorderPad = bp.left;
>+  nscoord rightBorderPad = bp.right;

You don't need to create new variables.  You can just use |bp.left| and
|bp.right| throughout.

>+  result = GetPrevInFlow(&prevFrame);
>+  if (NS_SUCCEEDED(result) && prevFrame) {

GetPrevInFlow and GetNextInFlow can never fail, so don't bother checking the
return value.  (At some point we'll de-COM-ify them and give them the signature
|virtual nsIFrame* GetPrevInFlow()|, etc.)


However, I'm curious as to why GetSkipSides wasn't sufficient.	It seems like
any difference between what your code does and GetSkipSides would lead to
visible bugs, since you want to skip the border exactly when other code skips
the border.  (And there is a difference between the two, although I'm not sure
exactly when such cases would happen.)
Attached patch patch6 (obsolete) — Splinter Review
Attachment #144832 - Attachment is obsolete: true
Attached file testcase
The test case shows existence of other bugs about a layout. For example, if
window width is narrowed and a line-break occurs after clicking " BBBBBBBBBB ",
the space in a head and a tail will disappear or appear. Moreover, displays
differ by resizing and reload. However, a patch is effective about
text-decoration.
Attached patch patch7 (obsolete) — Splinter Review
Patch7 can use GetSkipSides and can lay out text, space and text-decoration
correctly for a testcase. Since change of nsInlineFrame.cpp is for calculating
y-position of the next line correctly in a nsLineLayout::VerticalAlignLine
function, space and text could be laied out correctly as a result.
Attachment #144974 - Attachment is obsolete: true
What does the nsInlineFrame.cpp change fix?
Attached image screen shot
A screen shot is shown. Left-hand side applies a patch and right-hand side is
original. If original, a space disappears and spacing of a line spreads,
moreover a layout changes with reloading.
(In reply to comment #14)
> What does the nsInlineFrame.cpp change fix?

In the following case 
1. inline element has a beginning or a trailing whitespace with padding
2. the line breaks at the space
y-position of the next line is miscalculated.
This problem is fixed. As a result, space and text can be laid out correctly.
> 1. inline element has a beginning or a trailing whitespace with padding

Even if paddings have a zero value, a line moves downward.

another testcase:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html><head><title></title></head>
<body>
<table cellSpacing="0" cellPadding="0" border="1" width="100%"><tbody><tr><td>
AAAAAAAAAAA<a href=""> BBBBBBBBBB </a>CCCCC
</td></tr></tbody></table>
</body></html>

result:
|AAAAAAAAAAA|
|BBBBBBBBBB | <---- This line moves downward
|CCCCC      |
Comment on attachment 145016 [details] [diff] [review]
patch7

putting review request in my queue so I don't lose track of this
Attachment #145016 - Flags: review?(dbaron)
I believe the patch in bug 258126 fixed this....
Depends on: 258126
Attachment #145016 - Flags: review?(dbaron)
Attached patch patchSplinter Review
For the testcase, the problem still remains, the changes of the second half is
required.
Attachment #145016 - Attachment is obsolete: true
Attachment #163515 - Flags: review?(dbaron)
Why should padding or border have any positioning effect on inlines that have
both a prev-in-flow and a next-in-flow?
This problem issues when the frame is empty since a white space was trimmed.
Since the value of the logicalHeight on nsLineLayout::VerticalAlignFrames is
zero, both psd->mBottomLeading and psd->mTopLeading are not zero. Then the
inline shifts downward. Furthermore, I think that each values of aMetrics should
not be set zero, since this frame has padding. I think that the inline was
wrapped (split) by resize reflow, if the inline has a prev-in-flow or a
next-in-flow.
My previous comment was based on reading an || as an &&.  Oops.

However, I think we really need to fix the line breaking code not to create
these empty inline continuations that shouldn't be there.  It seems like this
patch will make what we've previously covered up reasonably well appear.  Really
we should go the other direction and make it not exist at all.
The test case below is rendered incorrectly. Only the first line has the padding
applied but the text decoration only starts 41px from the left margin on all lines:

<?xml version="1.0" encoding="iso-8859-1"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<style type="text/css">
/*<![CDATA[*/
.link
{
 padding-left: 40px;
}
/*]]>*/
</style>
<title></title>
</head>
<body>
<p style="width: 15em"><a href="" class="link">Lorem ipsum dolor sit amet,
consectetuer adipiscing elit. Phasellus ullamcorper. Aenean tincidunt enim in
odio. Nullam commodo placerat magna. Phasellus pellentesque varius sapien.
Aliquam erat volutpat. Sed laoreet nunc eu mauris. Suspendisse potenti. Ut
imperdiet, lacus rhoncus rhoncus pharetra, ante lacus interdum mauris, vel
facilisis metus purus et ligula. Nam pretium, nulla id auctor ultricies, magna
ipsum tempor ligula, auctor venenatis ipsum elit nec lacus.</a></p>
</body>
</html>

Rendered as

    Lorem ipsum dolor sit amet,
    ___________________________

consectetuer adipiscing elit.
    _________________________
Klaus Johannes Rusch, please attach HTML as attachments; don't paste it into bug
comments.

That said, your testcase works fine (the underlining is where it should be) in a
current trunk build.  Make sure you're using a Mozilla 1.8b1 or Firefox
1.1-track build (NOT Firefox 1.0 or Firefox 1.0.1 or Mozilla 1.7.x) when testing
layout issues.
The testcase of comment 0 works for me

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070220 Minefield/3.0a3pre
-> WFM
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WORKSFORME
comment 14 - comment 17 and comment 20 - comment 23 need to be split into a separate bug.
(If it's still present, that is.)
Testcase (attachment 144976 [details]) can reproduce this problem yet on trunk-
20070324/linux.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment on attachment 163515 [details] [diff] [review]
patch

OK, let's try this.  I think removing the special handling here will probably fix a bunch of weird bugs, and I suspect this code was originally introduced because of block-within-inline cases that are now handled separately (since the inline part containing the block is now a block and not a continuation).

(Except with the commenting out replaced by removal and reindenting, which I have in my tree.)  r=dbaron

But I'd like roc to look it over as well.

Note that after we do this, we should remove other code that checks for these continuations, such as in nsInlineFrame::GetSkipSides and nsLineLayout::VerticalAlignFrames.  (I think, although maybe other things can make such empty continuations that we do need to check for.)
Attachment #163515 - Flags: superreview?(roc)
Attachment #163515 - Flags: review?(dbaron)
Attachment #163515 - Flags: review+
I'm not sure what the original code was trying to fix (it's archaic and has no associated bug number). For that reason alone I suggest we remove it.

However, it seems that this will change behaviour of, e.g.,

<p>This is some text. This is some text. This is some text. <span style="font-size:200px;"> Hello.</span>

If we break before "Hello", then the first line will get a large ascent. Is that what we want?

If we decide that we want collapsed whitespace to have zero descent and ascent, I suggest we just do that in nsTextFrame.
(In reply to comment #32)
> However, it seems that this will change behaviour of, e.g.,
> 
> <p>This is some text. This is some text. This is some text. <span
> style="font-size:200px;"> Hello.</span>

It doesn't change the behavior of that testcase (in quirks mode or standards mode).  No part of the span is on the first line.

If I remove the space before the start of the span, then I get the height on the first line in both standards and quirks mode, but that's not a change either -- it's what we do in Firefox 2.
Assignee: nobody → dbaron
Status: REOPENED → NEW
Attachment #163515 - Flags: superreview?(roc) → superreview+
Assignee: dbaron → hsaito54
Checked in to trunk.  Thanks for the patch, and sorry it took so long for me to get to the review.

I filed bug 379832 on doing additional code removal of code related to empty continuations.
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
Depends on: 385469
You need to log in before you can comment on or make changes to this bug.