Closed Bug 393096 Opened 17 years ago Closed 17 years ago

page renders too wide since reflow branch

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta1

People

(Reporter: vlad, Assigned: roc)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: regression, Whiteboard: [dbaron-1.9:RwCr])

Attachments

(3 files, 7 obsolete files)

somewhat reduced testcase
11.06 KB, text/html
Details
v8 (updated to trunk)
50.73 KB, patch
dbaron
: review+
roc
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Screen shot comparing Minefield & IE7
132.42 KB, image/png
Details
The given URL renders very wide on the current trunk; it renders as expected in Fx2 and IE. The problem seems to be related to the bottommost table, with the list of users -- the structure there is something like <td><span> <div>..</div> <div>..</div> ... </span></td>: 13:10 <@bz> vlad: plus it 13:10 <@bz> vlad: it's {inc} 13:10 <@bz> vlad: if I remove one of those divs and put it back, things wrap 13:10 <@bz> vlad: so definitely a bug I can confirm that removing one of those divs in DOMI causes the page to wrap correctly.
Flags: blocking1.9+
Here's a somewhat reduced testcase. When you delete any of the divs, the page will rewrap. If you remove the <b></b> in the original document, the page does not rewrap after removing a div -- same thing if the &nbsp;'s are removed from the original.
Page renders as expected if the "white-space: nowrap" is removed from the divs.
(In reply to comment #2) > Page renders as expected if the "white-space: nowrap" is removed from the divs. > I am not at all sure the current behavior is incorrect. Opera renders this page identically to current trunk builds. It would seem to me that if you tell it nowrap and it does not wrap it is probably rendering as it was instructed to.
We should be wrapping at the spaces between the <div>s, as we do without the table. The fact that we're not is a bug. Note that wrapping all the divs in a left float has the same problem, so the issue is the min width calculation.
Also note that on the testcase we don't have opera compat, since Opera puts it all on one line while we end up wrapping at one of the spaces for some reason.
And Opera consistently disallows wrapping at those spaces even if that stuff is directly in <body>, while we do allow wrapping at those points.
(In reply to comment #4) > We should be wrapping at the spaces between the <div>s, as we do without the > table. The fact that we're not is a bug. > > Note that wrapping all the divs in a left float has the same problem, so the > issue is the min width calculation. > Yeah. That makes sense, I should have looked at the page source more closely. I guess Opera just got this wrong as well. (In reply to comment #5) > Also note that on the testcase we don't have opera compat, since Opera puts it > all on one line while we end up wrapping at one of the spaces for some reason. > I noticed that as well on the testcase. My original comment was on the webpage in the URL. It does seem odd that after refusing to wrap between all the divs it suddenly finds one that it decides it should wrap at.
We just wrap at the min-width, whatever we computed that to be... Why it's not always ending up equal to the pref-width is a separate question. ;)
Looks like possibly a textrun/textframe issue; in this case we have a single textrun for all the text, right? Note that: <html style="width: 0"> <span style="float: left"> <span style="white-space: nowrap;"> <span>Tebos</span>&nbsp; </span> <span style="white-space: nowrap;"> <span>Tebos</span>&nbsp; </span> <span style="white-space: nowrap;"> <span>Tebos</span>&nbsp; </span> </html> wraps wrong, while removing the space befor the inner span in the second outer span wraps right. Looking at the textrun, we seem to collaps the nowrap space inside each span with the wrappable space preceding that span. That looks wrong to me.
OK. There are actually 2 separate issues here, so this needs to be made into 2 bugs. The webpage in the URL field displays correctly with alpha1 and shows the issue int he bug with alpha2 so that would appear to be an issue caused by the reflow branch landing. The issue in the attached testcase exists in the alpha1 build so it pre-dates the reflow branch, so cannot really be the same issue as in the webpage. WIth FIrefox 2 it displays correctly, but with both alph1 and alpha2 it displays identically to Opera. The fact that th current builds actually do have a line break somewhere is evidently the result of a post alpha2 checkin.
> so that would appear to be an issue caused by the reflow branch landing. Right. We knew that before the bug got filed... See deps. > so cannot really be the same issue as in the webpage. Why not? Reflow branch could easily just make existing buggy code affect layout.
So with the testcase in comment 9, here's what I see: * We get into nsTextFrame::AddInlineMinWidthForFlow for the textframe that contains the "&nbsp; " at the end of the first outer span. * start == 6; flowEndInTextRun == 8 * We can't break before the &nbsp; * We can't break before the space * We get into that code for the newline * start == 8; flowEndInTextRun == 8 * We bail out * We get into that code for the leading space in the second outer span * start == 8; flowEndInTextRun == 8 * We bail out * We get into that code for the text in the second span * start == 8; flowEndInTextRun == 13 * We can't break before the first letter That said: (gdb) p mTextRun->CanBreakLineBefore(6) $37 = 0 (gdb) p mTextRun->CanBreakLineBefore(7) $38 = 0 (gdb) p mTextRun->CanBreakLineBefore(8) $39 = 0 (gdb) p mTextRun->CanBreakLineBefore(9) $40 = 0 (gdb) p mTextRun->CanBreakLineBefore(10) $41 = 0 (gdb) p mTextRun->CanBreakLineBefore(11) $42 = 0 (gdb) p mTextRun->CanBreakLineBefore(12) $43 = 0 (gdb) p mTextRun->CanBreakLineBefore(13) $44 = 0 (gdb) p mTextRun->CanBreakLineBefore(14) $45 = 0 That looks wrong. We should have a break opportunity somewhere in there.
(In reply to comment #11) > > so cannot really be the same issue as in the webpage. > > Why not? Reflow branch could easily just make existing buggy code affect > layout. > Hmm. I guess that is true also. It could be that before the reflow branch landing, the overall width was being constrained gy something that no longer constrains it. In any event I constructed this table which might be helpful: Version Forum URL attachment 277602 [details] Firefox 2 Fits horizontally Fits horizontally alpha1 Fits horizontally dislays on a single line alpha2-alpha5 Is too wide displays on a single line alpha6-current Is too wide displays on 2 lines
The space should collapse, but it should leave a break opportunity in the textrun. This is my bug.
Assignee: nobody → roc
(In reply to comment #14) > The space should collapse, but it should leave a break opportunity in the > textrun. This is my bug. Won't that make you fail Hixie's whitespace tests? That said, if that would break too many Web pages, we probably need to get CSS 2.1 changed. That said, I though Safari and Opera have implemented those rules for a while.
So it looks like when we're appending the text starting with Tebos we end up in nsLineBreaker::AppendText wth mAfterSpace true, and then we do: 267 breakState[offset] = mAfterSpace && !isSpace && 268 (aFlags & (offset == 0 ? BREAK_ALLOW_INITIAL : BREAK_ALLOW_INSIDE)); But in this case aFlags is 0, so we don't break at offset 0. In this case, the initialBreakController in BuildTextRunsScanner::SetupBreakSinksForTextRun (which calls AppendText) is nsTextFrame for the non-breaking whitespace before this word. Which is why we don't end up with a break here. That said, I don't quite understand why we end up with a break after the second word... Yet we do. The inconsistency needs to be fixed no matter what.
> Won't that make you fail Hixie's whitespace tests? Which test(s)? CSS 2.1 does not seem to specify whether a collapsed space can still induce a line break opportunity. (It doesn't say much about line breaking at all.)
I'll test that, but I can't make head or tail of it looking at it by hand.
What's happening here is that we only have a break opportunity at the end of a run of spaces, and in this case the end of the run of spaces is in a nowrap, so we don't allow the break. Probably we should modify this so that we allow breaking at the end of each run of *breakable* spaces. I've created a systematic reftest for whitespace collapsing and breaking that shows some other bugs. I'll see if I can get it all fixed up with a better spec for where we allow line break opportunities.
Attached patch fix and tests (obsolete) — — Splinter Review
Okay, here's the fix plus some systematic tests of combinations of white-space properties, testing both collapsing and line-breaking. The tests assume what I suggested in comment #21: breaks are allowed at the end of each run of breakable whitespace. (The patch also fixes the bug here, of course.) The fix has several parts: -- Modify nsLineBreaker so that we have a break opportunity at the end of each run of breakable whitespace. This requires passing a parameter to AppendInvisibleWhitespace to indicate whether it's breakable or not. It also requires one bit of extra state in nsLineBreaker because we might need to allow a break at the start of a chunk even if that chunk disallows all breaks. -- Currently there's an optimization to pass a null break-sink when we know there can't be any breaks in a chunk of text. We can't do that anymore because there could be an initial break pending as mentioned above. So I move that optimization down into nsLineBreaker by providing a flag to indicate whether the non-null sink passed is guaranteed to have no break opportunities in it. -- nsTextFrame::GetTrimmedOffsets should trim nonbreakable whitespace (as long as it's not preformatted) -- Likewise, canTrimTrailingWhitespace should be set for all non-preformatted whitespace -- Reorganize the post-text-measurement trimming analysis to make it more clear what's going on. Also, undo trimming in more situations: in particular, we used to leave the text pre-trimmed when we reached the end of the frame, the text ends in trimmable space(s) and the text fits in the available width with trimming, but does not fit in the available width without trimming. This is incorrect now because the text could be followed by a pre-wrap space, in which case this text won't be at the end of the line so we must not trim it. -- There is no longer a need to explicitly record a break opportunity when a frame ends with breakable whitespace; in fact, in the above situation it is incorrect to record such an opportunity. So we remove that.
Attachment #279027 - Flags: review?(smontagu)
Comment on attachment 279027 [details] [diff] [review] fix and tests I think this has a problem when untrimmed, but trimmable, whitespace overflows the available width --- nsLineLayout might conclude that the frame doesn't fit. To fix this we need to track in nsLineLayout how much width could be trimmed (similar to what the minwidth/prefwidth calculation does).
Attachment #279027 - Flags: review?(smontagu)
Attached patch fix v2 (obsolete) — — Splinter Review
This pulls in the patch from bug 392829 to clean up nsLineLayout before I whack it again here. I need dbaron review for the nsLineLayout changes. The non-cleanup change to nsLineLayout is just a way to record how much space could be trimmed from a textframe --- see comment #23 for why this is needed --- we basically have to return both the width with trimming and the width without trimming, the former to see if the textframe fits assuming it's at the end of the line, the latter in case there is something after the textframe on the line.
Attachment #279027 - Attachment is obsolete: true
Attachment #279429 - Flags: superreview?(dbaron)
Attachment #279429 - Flags: review?(smontagu)
Attached patch fix v3 (obsolete) — — Splinter Review
This has better min-width tests; putting a border around the table cell so we can check that the min-width is actually correct. This showed a bug in the previous patch (and on trunk): the min-width and pref-width computation code was setting aData->skipWhitespace for all white-space, not just collapsible whitespace; this meant that min and pref width for preformatted white-space followed by regular white-space was calculated as if they could collapse together, when in fact they can't.
Attachment #279429 - Attachment is obsolete: true
Attachment #279692 - Flags: superreview?(dbaron)
Attachment #279692 - Flags: review?(smontagu)
Attachment #279429 - Flags: superreview?(dbaron)
Attachment #279429 - Flags: review?(smontagu)
Attachment #279692 - Flags: review?(smontagu) → review+
Did you want me to review the whole patch, or just bug 392829, or something else? The constant BREAK_NONE_SET doesn't make a whole lot of sense to me -- I can't tell what it means from the name. Do you still pass http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ?
(In reply to comment #26) > Did you want me to review the whole patch, or just bug 392829, or something > else? Everything except for nsTextFrameThebes and nsLineBreaker ... that's mostly bug 392829 plus some extra changes. > The constant BREAK_NONE_SET doesn't make a whole lot of sense to me -- I can't > tell what it means from the name. Did you understand the comment in nsLineBreaker.h? How about calling it BREAK_SUPPRESS_SETTING_NO_BREAKS? > Do you still pass > http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ? No. But I think that testcase is broken. In the fifth line of the test div, which starts with "x<span class="normal"> <span class="pre"> </span>"..., I count 20 spaces which should be displayed between the first x and the next x, and the testcase seems to expect 17. What do you think? I have no idea how the count of 17 is supposed to be derived (one reason why I think my testcase is better...) I did find a bug when I was testing this, though, so I'll attach a new patch.
Attached patch fix v4 (obsolete) — — Splinter Review
Same as before except that in nsLineBreaker.cpp, if we do the "skip to the end of the text because there can be no breaks in this segment and the sink already has no line breaks" optimization, we need to avoid calling SetLineBreaks on the sink with uninitialized data...
Attachment #279692 - Attachment is obsolete: true
Attachment #281776 - Flags: superreview?(dbaron)
Attachment #281776 - Flags: review+
Attachment #279692 - Flags: superreview?(dbaron)
Attachment #281776 - Attachment is patch: true
Attachment #281776 - Attachment mime type: application/text → text/plain
(In reply to comment #27) > > The constant BREAK_NONE_SET doesn't make a whole lot of sense to me -- I can't > > tell what it means from the name. > > Did you understand the comment in nsLineBreaker.h? How about calling it > BREAK_SUPPRESS_SETTING_NO_BREAKS? I don't even understand the comment before BREAK_ALLOW_INSIDE. Does it mean that breaks are allowed anywhere, or just at whitespace (in which case I think you don't want "Also"). But I think BREAK_SUPPRESS_SETTING_NO_BREAKS is better -- it's less close to the other names, which I think is good since it's not particularly related. > > Do you still pass > > http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ? > > No. But I think that testcase is broken. I think the test is broken for the reasons stated in bug 191699 comment 17, but those are all related to breaking opportunities, not amount of space. > In the fifth line of the test div, > which starts with "x<span class="normal"> <span class="pre"> </span>"..., I > count 20 spaces which should be displayed between the first x and the next x, > and the testcase seems to expect 17. What do you think? I have no idea how the > count of 17 is supposed to be derived (one reason why I think my testcase is > better...) Hrm. We never had problems with that part of the test before. I count: 1 in the span class="normal" 1 in the span class="pre" 1 back in the span class="normal" 1 in the next span class="pre" 1 back in the span class="normal" 1 in the next span class="pre" 1 in the span class="nowrap" (the spaces before and after the newline are removed, the newline is converted to a space) 0 back in the span class="normal" (collapsed with previous) 2 in the div class="test pre" (I think that's the parent) 1 in the span class="pre" 1 in the span class="normal" 3 in the div class="test pre" (I think that's the parent) 1 in the span class="nowrap" (the spaces before and after the newline are removed, the newline is converted to a space) 0 in the span class="normal" 2 in the div class="test pre" (I think that's the parent) That adds up to 17.
(In reply to comment #29) > I don't even understand the comment before BREAK_ALLOW_INSIDE. Does it mean > that breaks are allowed anywhere, or just at whitespace (in which case I think > you don't want "Also"). It means breaks are allowed after whitespace and wherever nsILineBreaker says. I guess that would be clearer if these were BREAK_SUPPRESS_INITIAL and BREAK_SUPPRESS_INSIDE? > But I think BREAK_SUPPRESS_SETTING_NO_BREAKS is better -- it's less close to > the other names, which I think is good since it's not particularly related. OK. If I change the others to BREAK_SUPPRESS, how about calling this one BREAK_SKIP_SETTING_NO_BREAKS? > Hrm. We never had problems with that part of the test before. > > I count: > > 1 in the span class="normal" > 1 in the span class="pre" > 1 back in the span class="normal" > 1 in the next span class="pre" > 1 back in the span class="normal" > 1 in the next span class="pre" > 1 in the span class="nowrap" (the spaces before and after the newline are > removed, the newline is converted to a space) Actually I count 1 before this span and 0 inside it.... > 0 back in the span class="normal" (collapsed with previous) > 2 in the div class="test pre" (I think that's the parent) > 1 in the span class="pre" > 1 in the span class="normal" > 3 in the div class="test pre" (I think that's the parent) > 1 in the span class="nowrap" (the spaces before and after the newline are > removed, the newline is converted to a space) > 0 in the span class="normal" > 2 in the div class="test pre" (I think that's the parent) > > That adds up to 17. Actually I think you're right, I must have miscounted before. The testcase could be clearer...
Suppress seems like the opposite of allow. How are they the same?
I mean I'll call them "SUPPRESS" and invert the usage.
Okay. The reason Hixie's testcase fails with the patch is not due to the space count at all (it counts 17 remaining spaces, which we both currently agree is correct). What happens is the second item of your bug 191699 comment #17: the last span class="normal" in the line is treated by us as a break opportunity (even though it collapses away). I think this is permitted by CSS2.1, which specifically says it mostly does not define where line break opportunities occur. I've vacillated about the desirability of allowing break opportunities due to collapsed spaces, but currently I think that it's counter-intuitive for a white-space:normal span to appear in content but for there to be no break opportunities in that content, so my code specifically allows for break opportunities due to collapsed spaces, hence nsLineBreaker::AppendInvisibleWhitespace.
Attached patch fix v5 (obsolete) — — Splinter Review
with new and improved flag names
Attachment #281776 - Attachment is obsolete: true
Attachment #282241 - Flags: superreview?(dbaron)
Attachment #282241 - Flags: review+
Attachment #281776 - Flags: superreview?(dbaron)
The diff doesn't seem to contain the new files (reftests); it probably should.
Attached patch fix v6 (obsolete) — — Splinter Review
#!*()!! CVS
Attachment #282241 - Attachment is obsolete: true
Attachment #282331 - Flags: superreview?(dbaron)
Attachment #282331 - Flags: review+
Attachment #282241 - Flags: superreview?(dbaron)
Attached patch fix v7 (obsolete) — — Splinter Review
I checked in the nsLineLayout cleanup separately, so here is an updated patch.
Attachment #282331 - Attachment is obsolete: true
Attachment #283131 - Flags: superreview?(dbaron)
Attachment #283131 - Flags: review+
Attachment #282331 - Flags: superreview?(dbaron)
Whiteboard: [needs review]
Attachment #283131 - Attachment is obsolete: true
Attachment #285018 - Flags: superreview?(dbaron)
Attachment #285018 - Flags: review?(dbaron)
Attachment #283131 - Flags: superreview?(dbaron)
Comment on attachment 285018 [details] [diff] [review] v8 (updated to trunk) ignore test_bug363146.html, that crept in by mistake
Attachment #285018 - Flags: review+
Whiteboard: [needs review] → [needs review][dbaron-1.9:RwCr]
Comment on attachment 285018 [details] [diff] [review] v8 (updated to trunk) You should remove LL_HASTRAILINGTEXTFRAME since you're making it unused. r+sr=dbaron; sorry for the huge delay getting back to this
Attachment #285018 - Flags: superreview?(dbaron)
Attachment #285018 - Flags: superreview+
Attachment #285018 - Flags: review?(dbaron)
Attachment #285018 - Flags: review+
Requesting M9 blocking status. This is a major layout regression that would be good to have fixed for beta. It fixes or blocks a number of my blocker bugs.
Target Milestone: --- → mozilla1.9 M9
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [needs review][dbaron-1.9:RwCr] → [dbaron-1.9:RwCr]
Flags: in-testsuite?
Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre and Vlad's test case in Comment 1, it seems that the page still renders quite a bit wider on Firefox than in IE 7. I am using a Win XP VM and have both browser windows sized the same. In the case of the Sony URL, both pages look the same.
Chris, can you check comment #44 and try to narrow down why the page is wider on trunk? A reduced testcase would be good.
(In reply to comment #44) > Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) > Gecko/2007102504 Minefield/3.0a9pre and Vlad's test case in Comment 1, it seems > that the page still renders quite a bit wider on Firefox than in IE 7. I am > using a Win XP VM and have both browser windows sized the same. In the case of > the Sony URL, both pages look the same. > It looks to me the rendering difference is caused by IE7 having a different default margin-right to Minefield's. This screen shot compares Minefield/IE7 with the test case, and with the testcase <body> with style margin-right:0. My guess is that IE has a bit more margin by default, and so it's wrapping the lines earlier to preserve its margin-right.
Blocks: 390015
Depends on: 402345
Depends on: 403192
Depends on: 403426
Depends on: 412607
Depends on: 457398
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: