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)

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.
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: 18 years ago
Resolution: --- → WORKSFORME
(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: 18 years ago18 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.

Attachment

General

Creator:
Created:
Updated:
Size: