237366 - Page layout is broken when the page was loaded first or super-reloaded.

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

# Sorry the spam.

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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;
}

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Robert:
What do you need in testcase?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> -- 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...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 9

[Sekundes]

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.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> 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.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 13

[Bernd]

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

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

bug 236796 didn't change any code that should affect layout.

What we need here is a minimal testcase that reliably reproduces the problem...

Comment 16

[Hideo Saito]

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

Comment 17

[Hideo Saito]

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

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should be high priority for 1.7

Comment 19

[Hideo Saito]

All/All

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

Comment 20

[Hideo Saito]

Attachment: test patch

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's the rationale behind that patch?

Comment 22

[Hideo Saito]

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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 24

[Hideo Saito]

Attachment: testcase

Comment 25

[Hideo Saito]

Attachment: screen shot

Comment 26

[Andrew Schultz]

Attachment: minimal testcase

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

Comment 27

[Hideo Saito]

(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);

Comment 28

[Bernd]

>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+.

Comment 29

[Hideo Saito]

> >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.

Comment 30

[chris hofmann]

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

Comment 31

[Asa Dotzler [:asa]]

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? 

Comment 32

[Hideo Saito]

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 33

[Hideo Saito]

Comment on attachment 145672 [details] [diff] [review]
test patch

This patch tests whether ParentReflowState has incremental reflow command.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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?

Comment 35

[Boris Zbarsky [:bzbarsky]]

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.

Comment 36

[Hideo Saito]

> 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.

Comment 37

[Boris Zbarsky [:bzbarsky]]

(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.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 41

[Hideo Saito]

Comment on attachment 145672 [details] [diff] [review]
test patch

My idea that this bug can be hidden was wrong.

Comment 42

[chris hofmann]

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?

Comment 43

[Bernd]

no, this  yet another reflow bug (YARB)

Comment 44

[Hideo Saito]

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;
+      }

Comment 45

[Hideo Saito]

Attachment: patch

Comment 46

[Boris Zbarsky [:bzbarsky]]

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.

Comment 47

[Bernd]

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

Comment 48

[Bernd]

Attachment: reflow log for testcase

Comment 49

[Bernd]

Attachment: testcase with border-spacing:0

Comment 50

[Bernd]

Attachment: reflow log for borderspacing 0

Comment 51

[Bernd]

Attachment: testcase with borderspacing 0

Comment 52

[Bernd]

Attachment: reflow log for borderspacing 0

Comment 53

[Bernd]

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 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 55

[Hideo Saito]

Attachment: minimal testcase2

Testcase does not have any empty cell.

Comment 56

[Bernd]

Attachment: patch

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.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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/

Comment 58

[Bernd]

fix checked in

Comment 59

[Bernd]

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 60

[Brendan Eich [:brendan]]

Comment on attachment 150077 [details] [diff] [review]
patch

a=[e-mail] for 1.7final.

/be

Comment 61

[Bernd]

fixed on branch too

Comment 62

[Max Waterman]

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.

Comment 63

[Michael Lefevre]

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

Comment 64

[Kenan]

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