User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040312 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040312 Layout of above URL page is broken, when I load first or super-reload. This problem is reproduced on later 2004031108/WinXP build. But it isn't reproduced on before 2004031008/WinXP build. Reproducible: Always Steps to Reproduce: See. http://www.mugimugi.com/mugiradioz/index.html Actual Results: following screenshot. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2089&action=view Expected Results: following screenshot. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2091&action=view I tried to create minimize test case. But I cann't create it. Because it isn't reproduced with small HTML file.

# Sorry the spam. This bug came from Bugzilla-jp http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3685

Roc, your checkins look like the only ones in the regression range that may have affected layout... fwiw I can't reproduce with a Linux 2004-03-11-09 build...

following screenshot has border with table and td elemnts. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2090&action=view table{ border: black 1px solid; } td{ border: blue 5px dashed; }

There must be some way to reduce this testcase. Please!

Robert: What do you need in testcase?

We need a page that shows the incorrect layout and -- doesn't use any Javascript -- preferably, has no external images or style sheets (completely self-contained) -- is as small as possible (i.e., if you take *anything* out, the page works correctly)

> -- doesn't use any Javascript This page's script is not related. It is checked. > -- preferably, has no external images or style sheets (completely self-contained) Misfortune, if rendering the page is faster, this problem isn't reproduce. e.x., On lacal HDD, it can't be reproduced when first row of the most outer table was deleted. > -- is as small as possible (i.e., if you take *anything* out, the page works > correctly) same. So, I couldn't create a simple testcase. But the date of regression is clearly. Therefore, I thought that cause may become clearly from the source code...

Okay. Thanks, that's useful to know. Bernd: looks like some table layout wackiness. Care to look at this list of checkins and guess which one caused the regression? http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=3%2F10%2F2004&maxdate=3%2F12%2F2004&cvsroot=%2Fcvsroot

Hi, I can reproduce with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040309 Firefox/0.8.0+ build, which is before the reporter mentioned. But I can't reproduce with win32 2004-03-11-08 mozilla build. Actually, I've encountered similar table issue about this. I doubted this bug and mine also related to bug 4510. I'll tested these bugs again when Mozilla 1.7b released and see what happen.

Here is adding infomation. When tab bar is hidden, I can't reproduce it. But when tab bar is shown, I can reproduce it(number of tabs is not related). # with 2004031309-trunk/WinXP.

> Hi, I can reproduce with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; > rv:1.7b) Gecko/20040309 Firefox/0.8.0+ build, which is before the reporter > mentioned. really? > When tab bar is hidden, I can't reproduce it. > But when tab bar is shown, I can reproduce it(number of tabs is not related). I see this. This bug is related the height of Viewport. When height of Viewport is short, this bug is reproduced.

With 20040307(suite), 20040308(suite), 20040309(suite), and 20040310(suite), I can't reproduce this bug... On 2004031309/WinXP, It isn't reproduced when if Viewport height is too short... # This bug is reoproduced with certain range of Viewport height?

This regressed between 2004/03/10 and 2004/03/11 assuming that only changes in content and layout could cause this. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Flayout+mozilla%2Fcontent&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=03%2F10%2F2004+08%3A00%3A00&maxdate=03%2F11%2F2004+08%3A00%3A00&cvsroot=%2Fcvsroot

This regression comes from bug 236796. I tested by to build.

bug 236796 didn't change any code that should affect layout. What we need here is a minimal testcase that reliably reproduces the problem...

It seems that the following patch of attachment 140837 [details] [diff] [review] of bug 229654 causes this bug. --- nsBlockReflowContext.cpp 27 Jan 2004 06:46:33 -0000 1.113 +++ nsBlockReflowContext.cpp 8 Feb 2004 01:36:53 -0000 - if (NS_UNCONSTRAINEDSIZE != mSpace.width) { + if (NS_UNCONSTRAINEDSIZE != mSpace.width && + NS_UNCONSTRAINEDSIZE != mOuterReflowState.mComputedWidth) { This problem is confirmed by mozilla-1.7a/Trunk/20040209/WinXP, but not mozilla-1.7a/Trunk/20040208/WinX. This problem is reproducible on WinXP or WinNT, but not Win98. my steps to reproduce: 1. see http://www.mugimugi.com/mugiradioz/index.html 2. excute following program on shell (ex. forever.exe: main(){for(;;);} ) 3. reload, not super-reload

It seems that the following patch of attachment 140837 [details] [diff] [review] of bug 229654 causes this bug. --- nsBlockReflowContext.cpp 27 Jan 2004 06:46:33 -0000 1.113 +++ nsBlockReflowContext.cpp 8 Feb 2004 01:36:53 -0000 - if (NS_UNCONSTRAINEDSIZE != mSpace.width) { + if (NS_UNCONSTRAINEDSIZE != mSpace.width && + NS_UNCONSTRAINEDSIZE != mOuterReflowState.mComputedWidth) { This problem is confirmed by mozilla-1.7a/Trunk/20040209/WinXP, but not mozilla-1.7a/Trunk/20040208/WinX. This problem is reproducible on WinXP or WinNT, but not Win98. my steps to reproduce: 1. see http://www.mugimugi.com/mugiradioz/index.html 2. excute following program on shell (ex. forever.exe: main(){for(;;);} ) 3. reload, not super-reload

This should be high priority for 1.7

All/All I checked also on PowerPC/Linux using own build of released mozilla-1.7b.

Page layout of the following page is broken, it is reported at http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3730. http://tech.firebird.gr.jp/firebird/index.php?firebird_xsite=81 However if test patch is applied, its layout is right.

What's the rationale behind that patch?

I guess that the order of block's layout differs from ordinal condition, when they are laid out from timer event at PresShell. In that case, a |mComputedWidth| of parent's ReflowState has NS_UNCONSTRAINEDSIZE value.

Using the steps to reproduce in comment 16, I see different font sizes for the bottom line of text depending on whether I Reload or Shift-Reload. However, I don't see the layout differences shown in the screenshots in comment 0. That suggests to me that a Parser bug might be involved here. I'd really like to see a simplified testcase for this bug.

the second inner table should be centered, but is flush left.

(In reply to comment #25) > Created an attachment (id=145905) > screen shot > Testcase shows another bug which has height even if the cell is empty. The height should be re-calculated when the cell is empty but |contentEmptyBeforeReflow| is false at nsTableCellFrame::Reflow in ./layout/html/table/src/nsTableCellFrame.cpp. // see testcase "emptyCells.html" if ((0 == kidSize.width) || (0 == kidSize.height)) { // XXX why was this && SetContentEmpty(PR_TRUE);

>Testcase shows another bug which has height even if the cell is empty. I hope it is not necessary to touch the empty cell pandora box. If yes, then a lot of the reflow in the table cell reflow needs to be changed, as the whole logic is completely broken. I plan to do this as either as part of bug 226637 or in a seperate bug. However my reading of the code makes me believe that there isn't a small change which will not cause another bunch of regressions. Especially as some special code is nessary as a quirk. So this needs IMHO to be done either right and completely or not touched. However I fail to see how this would fit into the 1.7 schedule. I doubt that this bug deserves a 1.7+.

> >Testcase shows another bug which has height even if the cell is empty. > > I hope it is not necessary to touch the empty cell pandora box. An empty cell is not necessary in order to reproduce this bug that is |Page layout is broken|. Although it explained since other bugs were found by chance, a new bug should be created.

need to figure out what to do on this one for 1.7... any suggestions?

Is it likely that this regression is going to impact high-profile sites? Is shipping the regression preferable to backing out the change that broke things?

I think that this bug has a same cause as Bug 244117 that is incremental reflow bug. This problem is reproduced by timing of flushing a content.

Comment on attachment 145672 [details] [diff] [review] test patch This patch tests whether ParentReflowState has incremental reflow command.

Comment on attachment 145672 [details] [diff] [review] test patch Doesn't this re-break bug 229654 when the current object is the target of the reflow command? What sort of testing have you done on this patch?

More to the point, isn't the problem that some reflows are just being optimized away somewhere here? As far as I can tell, the patch tries to make an unconstrained reflow "work" where a constrained reflow is really needed.

> Doesn't this re-break bug 229654 when the current object is the target of the reflow command? This patch does not re-break bug 229654 since the problem is not concerned with incremental reflow. > What sort of testing have you done on this patch? I tested on WinXP and PowerPC/Linux. The load applied to the system was changed variously for actual page and testcase. > isn't the problem that some reflows are just being optimized away somewhere here? This patch affects in the case |mComputedWidth| of parent's ReflowState has NS_UNCONSTRAINEDSIZE value and the object has incremental reflow command.

(In reply to comment #36) > This patch does not re-break bug 229654 since the problem is not concerned > with incremental reflow. I didn't mean that it re-breaks the precise testcase in bug 229654 but it restores the conditions that would lead to that bug in the first place (a frame trying to do auto margin stuff when the parent computed width is unconstrained). > I tested on WinXP and PowerPC/Linux. The load applied to the system was > changed variously for actual page and testcase. I was more interested in how you verified that this patch doesn't cause any other problems, not in how you checked that it fixes this bug.... > This patch affects in the case |mComputedWidth| of parent's ReflowState has > NS_UNCONSTRAINEDSIZE value and the object has incremental reflow command. I realize that. That's obvious from the patch. That wasn't what I was asking about. Again, it looks to me like you're wallpapering symptoms, and probably introducing other symptoms in the process, whereas what really needs fixing is the root cause, which I suspect is as described in comment 35.

It really seems like the table with no rows is needed to reproduce the bug. At least, I can't make a simple modification of the testcase without that table that still shows the bug. Is there a failure return value being propagated somewhere that causes part of the reflow process to be skipped?

It looks like whether the first inner table has rows or not controls whether the incremental reflow of the outer cell has a constrained (has rows, no bug) or unconstrained (no rows, shows bug) width. I have no idea why that would happen.

In my previous comment, I was refering to the computed width (not the available space).

Comment on attachment 145672 [details] [diff] [review] test patch My idea that this bug can be hidden was wrong.

seems like the regression is going to be low-visibilty and we have almost run out of time for fixing in 1.7... suggest that we minus this bug. anyone have other thoughts?

no, this yet another reflow bug (YARB)

For the following program, |nsTableRowFrame::IR_TargetIsChild()| is called from |IncrementalReflow()|. |kidRS.mComputedWidth| is set |NS_UNCONSTRAINEDSIZE| because |resetComputedWidth| is true. |kidRS.mComputedWidth| is not changed by |ReflowChild()|, then a child block can not be reflowed correctly. nsTableFrame::Reflow() at layout/html/table/src/nsTableFrame.cpp case eReflowReason_Incremental: NS_ASSERTION(HadInitialReflow(), "intial reflow not called"); rv = IncrementalReflow(aPresContext, aReflowState, aStatus); nsTableRowFrame::IR_TargetIsChild() at layout/html/table/src/nsTableRowFrame.cpp PRBool resetComputedWidth = aTableFrame.NeedStrategyInit() || aTableFrame.NeedStrategyBalance(); InitChildReflowState(*aPresContext, cellAvailSize, aTableFrame.IsBorderCollapse(), p2t, kidRS, resetComputedWidth); rv = ReflowChild(aNextFrame, aPresContext, cellMet, kidRS, cellOrigin.x, 0, 0, aStatus); For example, in order to reflow again, is the following patch correct? case eReflowReason_Incremental: NS_ASSERTION(HadInitialReflow(), "intial reflow not called"); rv = IncrementalReflow(aPresContext, aReflowState, aStatus); nextReason = eReflowReason_Resize; + if (NeedStrategyInit() || NeedStrategyBalance()) { + nextReason = eReflowReason_StyleChange; + }

I don't know the table code well enough to review this in any reasonable length of time (that is, certainly not until July 11; even then reviewing this change would take me a week or more....). You may want to ask bernd for review first.

I will look on the patch in detail over the weekend.

Comment on attachment 149927 [details] [diff] [review] patch I don't think that the patch does the right thing. It should follow Davids comment 23. If one looks at the reflow log for the empty table one sees: tblO 02370C5C r=0 a=UC,UC c=UC,UC cnt=872 tbl 02370D64 r=0 a=UC,UC c=UC,UC cnt=873 rowG 02370E38 r=0 a=UC,UC c=UC,UC cnt=874 rowG 02370E38 d=UC,0 tbl 02370D64 d=0,24 me=0 tblO 02370C5C d=0,24 me=0 This raises the question how a empty table can get 0 width and 24twips height. The only thing that comes into mind is the cellspacing that is added even if there is no table cell at all. But below things go also bad: tblO 02370C5C r=2 a=1128,UC c=0,UC cnt=883 tbl 02370D64 r=2 a=1128,UC c=UC,UC cnt=884 rowG 02370E38 r=2 a=-24,UC c=0,UC cnt=885 rowG 02370E38 d=-24,0 o=(0,0) 0 x 0 tbl 02370D64 d=0,24 tblO 02370C5C d=0,24 What is a negative available width? Looks like the cellspacing is substracted twice even if there is no cell. The next idea that comes into mind is to change the borderspacing from its default value to say 10 px and it becomes obvious that no other browser (opera, IE) does that weird vertical borderspace addition. So what happens if borderspacing is 0? The testcase is rendered as other browser do (see attachment 150014 [details]). If we look further down the reflow log one sees that the cell as result of incr. reflow ( the target is the block below) block 0237096C d=1104,360 me=144 m=144 cell 0237090C d=1152,408 me=192 m=192 row 02363CC4 d=1152,408 rowG 02363B68 d=1152,468 rowG 02363B68 r=2 a=1152,UC c=1152,UC cnt=927 row 02363CC4 r=2 a=1152,UC c=1152,UC cnt=928 row 02363CC4 d=1152,408 row 02372EAC r=2 a=1152,UC c=1152,UC cnt=929 row 02372EAC d=1152,48 rowG 02363B68 d=1152,468 The cell has already the correct size so there is no need to reflow its content again. When one compares that to attachment 150016 [details] the reflow log for border-space:0 then one sees: block 0251E974 d=1152,312 me=120 m=120 cell 0251E914 d=1200,360 me=168 m=168 row 02511F8C d=1176,360 rowG 02511E30 d=1176,408 rowG 02511E30 r=2 a=1176,UC c=1176,UC cnt=927 row 02511F8C r=2 a=1176,UC c=1176,UC cnt=928 cell 0251E914 r=2 a=1176,UC c=1128,UC cnt=929 block 0251E974 r=2 a=1128,UC c=1128,UC cnt=930 that the cell doesnt have the right size so it needs to be reflown. What the patch in attachment 149927 [details] [diff] [review] does, it forces simply another reflow to give the block code a chance to correct the wrong alignment from the incr. reflow. It also disables nearly a good part of the performance optimization in table reflow. So for me the questions that need to be addressed in a patch are: 1. Why does that empty table have a nonzero size (height)? 2. That negative available width seems to be wrong and should be corrected. 3. Why does the block code not align the second table correctly in the incr. reflow? 4. Is that a result from a 0 width 24twips high table?

Comment 38 suggests that this is unlikely to be common on the web, so changing to blocking1.7-.

Testcase does not have any empty cell.

The table code is violating the contract. In the scenario highlighted by this bug it reflows the block with a unconstrained computed size, but doesn't ensure that it will reflow the block with a constrained computed width. Due to another bug in table cell code the resulting table cell size differs frequently from the available size (see attachment 150016 [details]) which hides this bug for most pages. The other bugs shown in my previous comment needs to be fixed too, but this one is rather serious. The attached patch passes the regression tests. It is less intrusive than Saito's patch and relies on already existing infrastructure.

The original page is gone, because the radio finished. # However, the page will be back soon. The original page is copied and uploaded to my server. http://www.d-toybox.com/mozilla/testpages/bug3685/

fix checked in

Comment on attachment 150077 [details] [diff] [review] patch this bug was already marked as 1.7 and got minused only yesterday. The patch seems to be low risk but fixes a serious breach of the reflow rules, that is when a frame reflows a child unconstrained its the duty of the frame to reflow that child again constrained.

Comment on attachment 150077 [details] [diff] [review] patch a=brendan@mozilla.org for 1.7final. /be

fixed on branch too

It seems that the fix for this bug also fixes 239780 and 239778. Can someone confirm that this is the case (we can confirm they are fixed, just not sure this fix is the reason)? Also, will this fix get into firefox? Thanks. Max.

Max - yes, this fix was checked into the Firefox branch as well.

*** Bug 246553 has been marked as a duplicate of this bug. ***