54571 - on moviefone.com, only the ad and tab bar display--page body is blank

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[ekrock's old account (dead)]

Using Commercial 2000092808 on WinNT 4.0 SP4.

To repro, compare display of http://moviefone.com/ in the current commercial
build and earlier ones, Nav4, or IE. The page body is blank in today's
commercial build.

This is a recent regression. A definite RTM stopper--moviefone.com is a popular
web site and a major AOL brand.

Comment 1

[ekrock's old account (dead)]

Nominating rtm, marking regression.

Comment 2

[ekrock's old account (dead)]

4xp, P1. This is a high profile AOL brand and high profile backwards 
compatibility.

Comment 3

[ekrock's old account (dead)]

Reassigning to karnaze for initial triage and analysis so this doesn't languish 
on Clayton's plate.

Comment 4

[ekrock's old account (dead)]

Nominating nsbeta3 as this might even qualify for stop-ship nsbeta3.

Comment 5

[karnaze (gone)]

jst has agreed to take a look. When javascript is turned off, much of the 
content shows up (although not 100% correct).

Comment 6

[karnaze (gone)]

I'm reassigning this to harishd and ccing rickg, since ekrock thinks this is a 
serious bug (I'm also marking it nsbeta3+, rtm+).

from jst:

I had a look at the page and I know what's causing the bad layout, it's 
basically due to the fact that the page contains things like:

<noscript>
<table>
...
</noscript>
</table>

I.e. ill-formed HTML, could you reassign the bug to me so that I can 
comment on it and forward it to Harish?

Comment 7

[karnaze (gone)]

Eric, I just spoke to Rick and he promised to look at it tomorrow (so I'm 
reassigning it to him). I'm also removing the nsbeta3+ that I added.

Comment 8

[rickg]

In fact I'm trying to recreate the problem now; then I'll determine what IE and 
nav do in such cases. Hold please...

Comment 9

[harishd]

Rickg, feel free to assign this bug to me. The problem is that when SCRIPT is 
enabled the NOSCRTIP contents are not autoclosed.

Comment 10

[rickg]

Using an enlarged testcase (attached), neither 4x nor IE render this (error) 
case as a table. The behavior is that containers opened in the <noscript> don't 
propagate out (as you'd expect) when the </noscript> appears. 

The solution is to autoclose containers upon the </noscript> and handle the 
broken markup that follows as usual. I should have a patch sometime early 
tomorrow.  

Comment 11

[rickg]

Ok -- I have a short (6 line) patch which seems to fix the problem. I'll run the 
top 100, and should be ready with this for tomorrow, if there's any hope of 
landing for pr3.

Comment 12

[rickg]

Even simpler: I have it down to a 1 line change.

Comment 13

[Michael La Guardia]

Marking "needinfo". This seems like a bad thing, but we need a patch, review and
super review to clear for checkin.

Comment 14

[ekrock's old account (dead)]

We should accept this for RTM. It is backward compatibility with poorly-formed
HTML 3.2 on the web--there's a heck of a lot of that on the web the last time I
checked! moviefone is a high profile AOL site. Also, if movefone.com screwed
this tag order up, others likely did too, and I'd rather not find out how many
the hard way.

Comment 15

[rickg]

Here's the patch...

Index: CNavDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v
retrieving revision 3.317
diff -r3.317 CNavDTD.cpp
1635c1635,1636
<       if(thePrevTag==aContext[theChildIndex]){
---
>       if((thePrevTag==aContext[theChildIndex]) ||
>          (eHTMLTag_noscript==aCurrentTag)){  //bug 54571

Comment 16

[harishd]

Looking at the patch I assume that we're autclosing noscript contents regardless
of whether script is ON or OFF. If my assumption is correct then I'd argue that
noscript contents *should not* be autoclosed when script is turned OFF.

Comment 17

[rickg]

Harish is right. I left out the code that caches the javascript (ON|OFF) state. 
Harish: I'm giving this back to you, since you wrote the code returns state 
(from the sink) indicating scripting status. Your comments show that we get 
NS_HTMLPARSER_ALTERNATECONTENT if js is enabled, but it's not clear how we know 
JS disabled. If you know, then cache the state with the DTD (I'll assume 
mScriptDisabled), and change my patch thusly:

Index: CNavDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v
retrieving revision 3.317
diff -r3.317 CNavDTD.cpp
1635c1635,1636
<       if(thePrevTag==aContext[theChildIndex]){
---
>       if((thePrevTag==aContext[theChildIndex]) ||
>          (mScriptDisabled && (eHTMLTag_noscript==aCurrentTag))){  //bug 54571

Comment 18

[harishd]

When JS is enabled we get a message from the sink NS_HTMLPARSER_ALTERNATECONTENT
( meaning that NOSCRIPT content should not be treated as a regular content ).
When JS is disabled sink would send an NS_OK message ( meaning that the NOSCRIPT
content should be treated as a regular content, i.e., as though NOSCRIPT did not
exist ). The message from the sink is stored in mDTDState.  I'll modify rickg's
patch accordingly.

Btw, I think the white board should be changed from [rtm+] to [rtm need info].

Comment 19

[Phil Peterson]

PDT wants this fix, but can't tell r=who? or sr=who? Can you provide that info
and then we'll get it in.

Comment 20

[harishd]

Attachment: Proposed patch

Comment 21

[Phil Peterson]

PDT marking [rtm need info]. Need reviewers.

Comment 22

[David Hyatt]

a=hyatt.  i use moviefone regularly, so i have a vested interest in getting this 
in. :)

Comment 23

[harishd]

Thanx for the approval hyatt!

Comment 24

[harishd]

Attachment: Proposed patch...

Comment 25

[Nisheeth Ranjan]

This bug will be fixed when Harish checks in his fix for bug 56501 into the
branch.  That fix is in the trunk right now and will get checked into the branch
once the PDT blesses bug 56501 rtm++.

Marking rtm-.

Comment 26

[harishd]

56501 changes are in. This should be fixed now.

Comment 27

[Chris Petersen]

Verified fixed in the 2001010209 build.