54005 - totally wrong layout

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Gregory Tallur]

Many layout problems on this webpage. Although the layout is perfect with IE 4
or 5, the page is almost empty with Mozilla build ID 2000092420 under win95.

Comment 1

[Claude Gohier]

I see the same problem with 2000092505 winNT.

Comment 2

[karnaze (gone)]

Attachment: reduced test case

Comment 3

[karnaze (gone)]

Buster, the max element size is too big. You can remove some entries from the 
test case. I left enough of them in so that the table would be unreasonably 
wide.

  TO::Rfl en 00E52DB8 rea=0 av=(8940,UC) comp=(0,0) count=0
     T::Rfl en 00E52E0C rea=0 av=(8940,UC) comp=(8016,UC) count=1
       TRG::Rfl 00E52E74 rea=0 av=(UC,UC) comp=(UC,UC) count=2
         TR::Rfl en 00E52EB8 rea=0 av=(UC,UC) comp=(UC,UC) count=3
           TC::Rfl 00E52F08 rea=0 av=(UC,UC) comp=(UC,UC) count=4
             Area::Rfl en 00E52F68 rea=0 av=(UC,UC) comp=(UC,UC) count=5
             Area::Rfl ex 00E52F68 des=(13275,2520) maxElem=(13275,180)
          TC::Rfl ex 00E52F08 des=(13305,2550) maxElem=(13305,210)
        TR::Rfl ex 00E52EB8 des=(13305,2550) maxElem=(13305,210)
      TRG::Rfl ex 00E52E74 des=(UC,2550) maxElem=(13305,210)
      TRG::Rfl 00E52E74 rea=2 av=(13305,UC) comp=(13305,UC) count=6
         TR::Rfl en 00E52EB8 rea=2 av=(13305,UC) comp=(13305,UC) count=7
           TC::Rfl 00E52F08 rea=2 av=(13305,UC) comp=(13275,UC) count=8
             Area::Rfl en 00E52F68 rea=2 av=(13275,UC) comp=(13275,UC) count=9
             Area::Rfl ex 00E52F68 des=(13275,2520)
          TC::Rfl ex 00E52F08 des=(13305,2550)
        TR::Rfl ex 00E52EB8 des=(13305,2550)
      TRG::Rfl ex 00E52E74 des=(13305,2550)
    T::Rfl ex 00E52E0C des=(13335,2580)
  TO::Rfl ex 00E52DB8 des=(13335,2580)

Comment 4

[buster]

Attachment: same test case, but with the inline end tags in the correct order, lays out correctly

Comment 5

[buster]

Attachment: same test case (end tags out of order), but HTML written in-line instead of using document.write. Lays out correctly

Comment 6

[buster]

The original test case contains lots of invalid HTML of the form:

  document.write('
    <b><font face="arial" size=1 color=0>&nbsp;VTR-Hardware</b></font><br>
  ');

(I added the line breaks to try to make it a little more readable here.)
Notice the end tags for b and font are out of order.
There is some odd interaction between the fix-up we do for invalid HTML and
document.write.
See the test cases I just attached to this bug:
a) if the end tag ordering problem is corrected, the page lays out correctly
b) if the HTML is written out directly instead of using document.write, leaving
the end tag ordering problem alone so they are still out of order, the page lays
out correctly.

cc-ing the parser and javascript guys in case this rings any bells.

tempted to future this one as a rare case of invalid markup, but Nav4 and IE
reportedly handle it correctly.

Comment 7

[buster]

Attachment: frame dumps for the 3 test cases

Comment 8

[rickg]

After further review, buster and I have concluded that the only difference in 
the <script> version (vs. plain html version) is that the script version is 
*not* getting newlines. 

We think that the text measurement code is ignoring the <BR>'s, and using the 
full length of the leaf elements for the cell width. Over to EVDP for further 
review.

Comment 9

[Hixie (not reading bugmail)]

cc'ing waterson who has done some line layout stuff recently.

It only seems to occur where there are no spaces involved. See new testcase.
Note that for me, the reduced test case actually works (Win2K Comm branch).

Comment 10

[Hixie (not reading bugmail)]

Attachment: Test case - there should be no red

Comment 11

[Chris Waterson]

Attachment: simpler test case; no <script>

Comment 12

[Chris Waterson]

I'm not sure what's going on here, but this is related to removal of nsTextRun.
The <br> frame should be interrupting the contiguous text run; not sure why it
isn't. I'll look at it...

Comment 13

[Chris Waterson]

Attachment: fix logic in nsLineLayout::FindNextFrame()

Comment 14

[Chris Waterson]

Yep, mea culpa. The logic was a bit off in FindNextFrame(); specifically, we
need to always check "top" (not next) to see if the frame CanContinueTextRun().
I'm experimenting with the patch now.

Comment 15

[Chris Waterson]

Attachment: fix logic in nsLineLayout::FindNextFrame()

Comment 16

[Chris Waterson]

Ignore previous (10/09/00 12:53) randomly generated patch. The second "fix logic
in nsLineLayout::FindNextFrame()" patch is what I meant to attach.

Comment 17

[buster]

a=buster. bumped to P2.  Without this fix, some pages will lay out very poorly
in ways that are hard to document.  This will cause page authors a lot of
unnecessary grief.

Comment 18

[Chris Waterson]

Also have r=karnaze in a private email; nominating for rtm+.

PDT, here are the details. This fix fixes an incorrectly located loop
terminating condition, and is essentially equivalent to changing a pre-increment
into a post-increment. Specifically, I bungled the loop logic in
nsLineLayout::FindNextFrame() when I removed nsTextRun about two months ago.

FindNextFrame() does a crawl of the frame tree, looking for the next
"continguous" piece of text. It stops as soon as it either 1) bangs into a frame
that would interrupt the text (i.e., CanContinueTextRun() returns false), or 2)
it finds a text frame. The bug is, we don't actually look at the frame that
we've just pulled off the top of the stack; we advance to its sibling first.
Although this'll tend to work the first time through the loop (because we'll
always be started off with a text frame), if we ever fall through through to the
tree-grovelling logic below, we'll always skip the first child frame, and
instead mosy on to its next sibling. The fix is to just do the
CanContinueTextRun() test before advancing.

This problem tends to be masked by the "whitespace frames" that get created
whenever you have newlines or whitespace between elements; however, in the test
cases, this whitespace was removed.

With respect to risk assessment, I'm very confident that this is the right way
to fix the problem we're seeing in this bug. I believe that the primary risk
here would be exposing other bugs that had been previously masked, and I think
the probabilty of /this/ masking problems would be small.

But let's say we did get this wrong, and a bug was exposed. It would likely
manifest itself as a botched word or line break, or, like this bug, a screwed up
div or table cell width computation. In this case, I feel pretty good about
erring on the side of fixing a known problem vs. exposing an unknown problem.

Comment 19

[Chris Waterson]

Fix checked in on trunk.

Comment 20

[Michael La Guardia]

marking rtm++

Comment 21

[Chris Waterson]

fixed on the branch

Comment 22

[Chris Petersen]

Fixed in the Sept 12 builds (Windows, Linux) and the Sept 11 Mac build.

Comment 23

[Doron Rosenberg (IBM)]

page loads (albeit some minor problems, but probably due to the bad html) in
win98 2000101704 trunk.

removing vtrunk keyword per asa, verified