Closed
Bug 238473
Opened 21 years ago
Closed 18 years ago
If inline element has padding and white space, its text-decoration is broken.
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: hsaito54, Assigned: hsaito54)
References
(Depends on 1 open bug)
Details
Attachments
(3 files, 7 obsolete files)
417 bytes,
text/html
|
Details | |
47.40 KB,
image/jpeg
|
Details | |
1.74 KB,
patch
|
dbaron
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Comment 2•21 years ago
|
||
Attachment #144639 -
Attachment is obsolete: true
Comment 3•21 years ago
|
||
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?
Assignee | ||
Comment 4•21 years ago
|
||
> At a guess, adding an extra member to all frames will not be accepted.
I moved an extra member to a class of nsHTMLContainerFrame.
Assignee | ||
Updated•21 years ago
|
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.
Assignee | ||
Comment 8•21 years ago
|
||
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
Assignee | ||
Comment 9•21 years ago
|
||
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.)
Assignee | ||
Comment 11•21 years ago
|
||
Attachment #144832 -
Attachment is obsolete: true
Assignee | ||
Comment 12•21 years ago
|
||
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.
Assignee | ||
Comment 13•21 years ago
|
||
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?
Assignee | ||
Comment 15•21 years ago
|
||
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.
Assignee | ||
Comment 16•21 years ago
|
||
(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.
Assignee | ||
Comment 17•21 years ago
|
||
> 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)
Assignee | ||
Updated•20 years ago
|
Attachment #145016 -
Flags: review?(dbaron)
Assignee | ||
Comment 20•20 years ago
|
||
For the testcase, the problem still remains, the changes of the second half is
required.
Attachment #145016 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
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?
Assignee | ||
Comment 22•20 years ago
|
||
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.
Comment 24•20 years ago
|
||
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.
_________________________
Comment 25•20 years ago
|
||
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.
Comment 26•18 years ago
|
||
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
Comment 27•18 years ago
|
||
-> WFM
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
(If it's still present, that is.)
Assignee | ||
Comment 30•18 years ago
|
||
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: 18 years ago → 18 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
You need to log in
before you can comment on or make changes to this bug.
Description
•