98261 - WRMB:the bullets continue past the text

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Sivakiran Tummala]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.2)
Gecko/20010726 Netscape6/6.1
BuildID:    2001-08-27-06

The bullets for the press releases continue past the text in Netscape  at url:
http://www.state.ny.us/governor/press/year01/press01.html. 



Reproducible: Always
Steps to Reproduce:
load the above url and scroll down to the end of the page.u will see the bullets
which are not supposed to be shown.

Actual Results:  the bullets appear even after the end of the text

Expected Results:  the bullets should not appear

Comment 1

[Dave Barrowman]

Sivakiran, why is this assigned to the Evangelism product?  It looks like it
should be assigned to the browser product, not sure which component. (Assigning
bugs to components is not my strength, but it certainly does not belong to
evangelism.)

Comment 2

[Sivakiran Tummala]

adding myself and mdunn to the cc list, keyword topembed

Comment 3

[David Epstein]

I noticed that the month links (i.e. January) in this page navigate to the blank
bullets. Looks like the monthly content isn't being displayed (the content
displays fine in IE5.5). Maybe this is a page loading issue? cc'ing gagan for
help who this should be assigned to.

Comment 4

[Dave Barrowman]

If you look at the content on the page
(http://www.state.ny.us/governor/press/year01/press01.html) we see that the
bullets break at June 13, 2001, at the exact point where a <br> appears inside
the <ul></ul>.  There appears to be a problem in the way Netscape handles this
HTML - we should see if we can build a reduced testcase.

Setting product to the Browser.

Comment 5

[Sivakiran Tummala]

 it has nothing to do with <BR> because i removed all the <BR>'s and tried to
load the page. it is a page loading issue (layout). 

Comment 6

[Zach Lipton [:zach]]

Get rid of the WRMB, reassign to default owner and qa. When you move 
bugs into a new component, you will almost always want to reassign the 
bug.

Comment 7

[Zach Lipton [:zach]]

Sorry, I spoke too soon on the WRMB bit.

Comment 8

[Marc Attinasi]

sivakiran, could you attach the testcase you made with the BR tags removed? That
would help alot. Thanks.

Comment 9

[Marc Attinasi]

Attachment: URL as a file

Comment 10

[Marc Attinasi]

The problem with the dropped content is an old one: there are *loads* of FONT
tags that are never closed, so we end up with incredibly deeply nested content.
After some nesting level, we just throw the content away. This is a feature, I
believe ;)

Over to harishd for confirmation. Just dump the content and look how deep it
gets at the end (it takes a while!).

Comment 11

[harishd]

The parser does limit nesting level ( 200 to be precise ). However, the problem
we're seeing here may not be related to it. 

FYI: I am able to reproduce the problem even after increasing the nesting level
limit ( 300 ).

Comment 12

[Marc Attinasi]

One obvious problem is that the layout code uses the HTMLContentSink's constant
MAX_REFLOW_DEPTH, but that doesn't work because the ContentSink's notion of
'depth' is not the same as layouts. A content node 20 levels deep will actually
be more like 24 levels deep in frame land, because we have some frames before
anything representing the document: Viewport, GfxScroll, ScrollPortFrame, and
Canvas.

I have a trivial fix that takes into account the non-document-related frames by
setting layout's MAX_REFLOW_DEPTH to the contentSink's MAX_REFLOW_DEPTH+5. It
causes the entire page to be rendered, but we lose the anchors everywhere that
we used to lose the text (from June 15 on in the attached version of the URL).
Harish has some ideas on how to fix that...

Comment 13

[Marc Attinasi]

Attachment: PATCH to reconcile the difference between the ContentSink's notion of depth and Layout's notion of depth

Comment 14

[harishd]

Attachment: patch v1.0 [ Tentative - Need's testing / Have to write a few comments ]

Comment 15

[harishd]

Attachment: patch v1.1

Comment 16

[Marc Attinasi]

Your patch makes sense, but it will, of course, change the document. It is
better than dropping the content though, I guess. Have you tested it much, and
are you ready for reviews?

Comment 17

[harishd]

>Your patch makes sense, but it will, of course, change the document.
The document will change but it will atleast try it's best to maintain the
structure. 

>Have you tested it much?
I ran my patch ( along with Marc's patch ) against testcases in bugs 98261,
49678, 55095, 55980, and 58917 ( huge pengin page ). Other than a few style
differences ( which was expected ) they all seem to render "fine" <-- not
correctly. 

>and are you ready for reviews?
I guess :-/. Marc, can you r=?

Comment 18

[Marc Attinasi]

Comment on attachment 49146 [details] [diff] [review]
patch v1.1

sr=attinasi (also good for r=attinasi if you prefer)

Comment 19

[Marc Attinasi]

Harish, if you decide not to check in my portion of the patch, please let me
know and I'll do it. I don't think it is critical to the fix, but it is more
correct.

Comment 20

[harishd]

Marc: IMO, your fix should go in. Please check it in if you can or let me know
if you want me to land along with my changes.

Comment 21

[harishd]

fixed on the trunk.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Seems like a safe fix with high return (fix topembed), therefore nsbranch+.

Comment 23

[Jaime Rodriguez, Jr.]

+ check this in today per PDT.

Comment 24

[harishd]

fixed.

Comment 25

[Chris Petersen]

Verified on branch builds Mac OS X (2001-09-24-04) and Windows ME (2001-09-25-05).

Comment 26

[Chris Petersen]

Verified on the Sept 25th Linux branch build (2001-09-25-04)