154780 - accessing "document.body.clientHeight" while layout causes layoutproblems

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Henrik Gemal]

this is causing big problems at our portal (largest portal in Denmark)

I've narrowed it down to two small files. Please load and compare these two files:
http://gemal.dk/test/opasia2.html
http://gemal.dk/test/opasia.html
which layout diffently!

The difference in those two files is:
<script>
var i = document.body.clientHeight;
</script>

I dont do anything with clientHeight. I just access it.

It seems that accessing clientHeight *while* the page layouts causes the page to
layout differently. Is it due to some reflow stuff??

If I access clientHeight when the page has finish loading as in:
http://gemal.dk/test/opasia3.html
it layout ok.

2002062713 on Windows 2000

Comment 1

[gabriel]

Confirming the problem with mozilla 1.0 NT

Comment 2

[Boris Zbarsky [:bzbarsky]]

tables bug.  The access causes a flush and reflow to make sure the height is up
to date, and the tables code seems to not deal well..

Comment 3

[Bernd]

Attachment: reflow log

Comment 4

[Bernd]

as visible from the attached reflow log the culprit is the wrong maxElementsize
(MES) reported back from the block layout code. ( the log has been created as
described in
http://lxr.mozilla.org/seamonkey/source/layout/doc/frame_reflow_debug.html)

         cell 02511794 r=1 a=8880,UC c=8850,UC cnt=890 
          block 025138AC r=1 a=8850,UC c=8850,UC cnt=891 
           block 02513BCC r=1 a=8850,UC c=1500,UC cnt=892 
            text 02513CA0 r=2 a=UC,UC c=UC,UC cnt=893 
            text 02513CA0 d=900,285 me=840 
            text 02514514 r=0 a=UC,UC c=UC,UC cnt=894 
            text 02514514 d=0,0 me=0 
            text 02513CA0 r=2 a=1500,UC c=UC,UC cnt=895 
            text 02513CA0 d=900,285 
            text 02514514 r=2 a=600,UC c=UC,UC cnt=896 
            text 02514514 d=0,0 
           block 02513BCC d=1500,300 me=1500 m=840 
           text 02514558 r=0 a=UC,UC c=UC,UC cnt=897 
           text 02514558 d=0,0 me=0 
           text 02514558 r=2 a=8850,UC c=UC,UC cnt=898 
           text 02514558 d=0,0 
          block 025138AC d=8850,300 me=8850 m=8190  !!!!!!! 
         cell 02511794 d=8880,330 me=8880 m=8880  

while on first reflow it returns: 
          block 025138AC d=1500,300 me=1500 
         cell 02511794 d=1530,330 me=1530 

and as usual when the block code spoils the MES calculation table-layout suffers

Comment 5

[Bernd]

Attachment: MES log

Comment 6

[Bernd]

nsBlockFrame::ReflowLine block 02A8B738 line 02A8B90C setting
aLine.mMaxElementWidth to 44611444 

looks pretty much like an unitialized use of mMaxElementSize

Comment 7

[Bernd]

just my next debug step:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#2589
mBounds.XMost() has already the unitialized value.

Comment 8

[Jerry]

how to get MES log?

Comment 9

[Bernd]

Attachment: patch

The patch still needs to be qualified, I did not run the regression tests yet.

Comment 10

[karnaze (gone)]

The first part of the patch looks fine, but the change to nsTableRowFrame is 
covering up a block bug. On cnt=116, the cell block is return m=8130, which is
too big.

   rowG 02FF7D08 r=1 a=8910,UC c=8910,UC cnt=110
    row 02FF7E94 r=1 a=8910,UC c=8910,UC cnt=111
     cell 03005DDC r=0 a=UC,UC c=UC,UC cnt=112
      block 03005E3C r=0 a=UC,UC c=UC,UC cnt=113
      block 03005E3C d=315,300 me=315
     cell 03005DDC d=375,360 me=375
     cell 02FF8060 r=1 a=8850,UC c=8790,UC cnt=115
      block 03004F40 r=1 a=8790,UC c=8790,UC cnt=116
      block 03004F40 d=8790,330 me=1530 m=8130
     cell 02FF8060 d=8850,390 me=1590 m=8190
    row 02FF7E94 d=8910,390
   rowG 02FF7D08 d=8910,390

Comment 11

[karnaze (gone)]

Oh, I take that back. I didn't see the computed value in

cell 02FF8060 r=1 a=8850,UC c=8790,UC cnt=115

which is causing the problem.

Comment 12

[karnaze (gone)]

Attachment: revised bernd's patch and added some comments

The regression tests need to be run.

Comment 13

[Bernd]

taking the bug

Comment 14

[Bernd]

the patch together with the patch for bug 162691 produces a regression when
printing
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/printing/bug153785-1.html

:-(

Comment 15

[Bernd]

If I remove the aTableFrame.NeedStrategyBalance() then the page prints
correctly. I will discuss this with Chris when he is back from vacation.

Comment 16

[karnaze (gone)]

Attachment: revised patch to not skip calculation if the table will be balanced

Comment 17

[kinmoz]

Comment on attachment 97692 [details] [diff] [review]
revised patch to not skip calculation if the table will be balanced

sr=[e-mail]

Comment 18

[Alexandru Savulov]

Comment on attachment 97692 [details] [diff] [review]
revised patch to not skip calculation if the table will be balanced

r= alexsavulov

Comment 19

[karnaze (gone)]

The patch is in.

Comment 20

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

Regarding the change to nsBlockReflowContext -- you should never be seeing
null-unit margins in computed style data.  That explicitly disagrees with the
comment in nsStyleStruct.h (and it also doesn't make sense -- we already allow
the auto unit):

   nsStyleSides  mMargin;          // [reset] length, percent, auto, inherit

Were you actually seeing such margins in the style data?  In what testcase?  (I
think that change is incorrect and should be reversed and the cause of a
null-unit margin should be fixed, or at the very least the purpose of such a
thing should be documented in nsStyleStruct.h.)

Comment 21

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

Filed comment 20 as bug 166588.

Comment 22

[karnaze (gone)]

Attachment: more efficient patch from bug 166577 to be applied over previous one

Comment 23

[karnaze (gone)]

Reopening after moving the better patch over from bug 166577.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 98447 [details] [diff] [review]
more efficient patch from bug 166577 to be applied over previous one

a=[e-mail]

Comment 25

[karnaze (gone)]

The 2nd patch is in.

Comment 26

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

The second patch caused a major regression on many cnn.com pages.  See bug
169620, particularly bug 169620 comment 42.