134401 - html of "frame compatible browser is required" message is displayed (noframes)

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Devoti Paolo]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9+) Gecko/20020329
BuildID:    2002032916

Reproducible: Always
Steps to Reproduce:
1. open url http://www2.comex.it/listino.htm

Actual Results:  top of page shows the html of the "frame compatible browser is
required" page

Expected Results:  normal render of page

Comment 1

[Devoti Paolo]

html tags are appearing spread in http://www.amd.com/us-en/
maybe this issue is not frame related

Comment 2

[Boris Zbarsky [:bzbarsky]]

The site seems to be down (it's not responding to HTTP requests)

Comment 3

[Devoti Paolo]

Check it again, maybe better in evening or night CET.
Behaviour of both sites is unchanged

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Confirming, but....

The content is invalid (<noframes> should come _inside_ <frameset>, not before
or after it).  The reason the bug happens is that once we hit the <frameset> tag
we set a "frameset document" flag and ignore <noframes> from that point on.  So
when <noframes> comes inside or after <frameset> things are fine.

When it comes _before_, we render it as CDATA (which is what we parse it as). 
NS4 also shows the <noframes> content on this site (thought it does _not_ show
the frameset).

Possible solutions:
1)  Fix this in the content sink or layout
2)  Just add "noframes { display: none }" to html.css

(I like #2 myself)

Comment 6

[Marc Attinasi]

Hmm. I guess we always support frames, so maybe it is best to hide the NOFRAMES
elements (and all children thereof). If we ever planned to allow an option to
disable frame support then this would be a problem, but that sure seems unlikely ;)

I agree with Boris - simple solution, nuke NOFRAMES in html.css

Comment 7

[Marc Attinasi]

Attachment: PATCH to hide NOFRAMES, since we do not support disabling frames anyway

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 77478 [details] [diff] [review]
PATCH to hide NOFRAMES, since we do not support disabling frames anyway

r=bzbarsky

Comment 9

[Marc Attinasi]

Interestingly, this looks like it is being handled in the frame constructor
already: see
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#4696

I wonder why that is not working in this case. Looking at this and the method
nsDocShell::GetAllowSubframes, it seems like there is at least _some_ support
for disabling frames, though I am not sure if it is tested. I think maybe the
blanket style rule is a bad idea now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm..  Does GetAllowSubframes return true?

Comment 11

[Marc Attinasi]

I haven't debugged it, but a pref is read (browser.frames.enabled) and it looks
like it is set to 'true' in all.js, so it should return true. Maybe I need to
debug it, if my build ever completes I will.

Comment 12

[rods (gone)]

Will not fix, but will help bug 133809, in that not matter how much "text" is
inside the noframes tag it will reflow to a single page in PP and be blank.
Printing has a hack in it that it will only print the first page of an "AsIs"
frameset anyway.

Comment 13

[rods (gone)]

I have nominated this for nsbeta1 because of Print Preview. But it still thinks
it needs to reflow for the noframes case see new Bug 139489

Comment 14

[Kevin McCluskey (gone)]

nsbeta1+

Comment 15

[Joe Hewitt (gone)]

Comment on attachment 77478 [details] [diff] [review]
PATCH to hide NOFRAMES, since we do not support disabling frames anyway

sr=hewitt

Comment 16

[karnaze (gone)]

Taking the bug. We shouldn't leave code in to support this feature (and IE 
doesn't support it). The spec says - "The NOFRAMES element specifies content 
that should be displayed only by user agents that do not support frames or are 
configured not to display frames". Lets not pretend that we can be configured 
not to display frames.

Comment 17

[karnaze (gone)]

Attachment: attinasi's patch + rip out noframes code

This patch is the previous one plus the removal of noframes code from the frame
constructor.

Comment 18

[karnaze (gone)]

As jkeiser points out, if we ever try to support bug 56743, then we may need the 
code that I ripped out. He also points out that there is more code to rip out. 
So, I'm giving this back to him so he can decide what to do. Either we should go 
with the simple patch or rip it all out (and possibly add some comments 
explaining what version removed it just in case we ever decide to use it). 

Comment 19

[John Keiser (jkeiser)]

The Plan: I am checking in the attinasi patch for now.  I acknowledge the
potential need to rip out all frame disabling code, but not until we decide
exactly how bug 56743 is to be implemented--much if not all of that code will be
useful at that point, and it's not really harming us to leave it in while the
decision is made.

Yes, the frame disabling code is screwed up in this case.  Yes, display: none
isn't getting set properly, and this patch is purely wallpaper over that.  But
until we really support noframes in frame disabling, none of that matters one whit.

During investigation of the display: none problem I found that display: none is
getting set in the frame constructor at that point but either the change isn't
sticking or someone is overwriting it with display: 1 later (I dumped style
contexts).

Comment 20

[John Keiser (jkeiser)]

Checked in to trunk.

Comment 21

[Jaime Rodriguez, Jr.]

thanks for the fix, but the better time to take this one is after RC2. adt1.0.0-
[adt2 RTM].

Comment 22

[Marc Attinasi]

Keep in mind that the HTML spec indicates that the NOFRAMES section _should_
show up if the document is shown out of a frameset, like when the user opens a
page by itself due to bookmarking or something.

http://www.w3.org/TR/html401/present/frames.html#edef-NOFRAMES

NOFRAMES may be used, for example, in a document that is the source of a frame
and that uses the transitional DTD. This allows authors to explain the
document's purpose in cases when it is viewed out of the frameset or with a user
agent that doesn't support frames.

So, for now, this is broken (well, it was broken before too - we were showing
the contents of the NOFRAMES container as HTML source instead of HTML elements).

Comment 23

[Kevin McCluskey (gone)]

[e-mail]: Can you verify this fix on the trunk?. Thanks

Comment 24

[Kevin McCluskey (gone)]

[e-mail]: Can you verify on the trunk? Thanks

Comment 25

[scottputterman]

amar, could you verify this on the trunk?

Comment 26

[Christine Wozniak]

Adding custrtm-; no impact on customization.

Comment 27

[Jaime Rodriguez, Jr.]

Mass removing adt1.0.0, and adding adt1.0.1 because, we are now on 1.0.1.

Comment 28

[Amarendra Hanumanula]

 Verified on trunk and branch 20020603 builds. Marking verified. 

Comment 29

[Jaime Rodriguez, Jr.]

Amar - Are you saying this is fixed on the 1.0 branch?  I believe the fix was
only checked into the trunk.

John - Did you land this on the branch and trunk?

Comment 30

[Amarendra Hanumanula]

I will have to take back my above statement since this is fixed only in the 
trunk builds. Still not fixed in the branch builds. I still see the HTML in the 
recent branch builds. Reopening untill its verified in the branch. 

Comment 31

[John Keiser (jkeiser)]

Correct, AFAICT it was given adt1.0.0-.  It's not even approved for branch.

Comment 32

[Kevin McCluskey (gone)]

fixed and verified on the trunk. Resolving as fixed.
*not* fixed on the branch, but it is nominated. 

Comment 33

[Jaime Rodriguez, Jr.]

Marking as verified for the trunk per Comment #32 From Kevin McCluskey.

Comment 34

[Kevin McCluskey (gone)]

ADT: Known sites affected by this http://www2.comex.it/listino.htm, and
print-preview of http://kontek.net/404/frames.html and print preview of
http://www.geocities.com/SiliconValley/1166/

Comment 35

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Drivers' approval. pls check this in asap, the add the "fixed1.0.1" keyword.

Comment 36

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 37

[scottputterman]

Please check this into the branch today.  Or, if it's already been checked in,
add the fixed1.0.1 keyword.  If it's not in tomorrow's build, the adt1.0.1+ may
be removed.

Comment 38

[Kevin McCluskey (gone)]

rods will be checking in the fix to the 1.0 branch today.

Comment 39

[rods (gone)]

fixed on branch

Comment 40

[Amarendra Hanumanula]

 Verified on branch build: 2002-07-11-08. WIN2K and linux 7.1. I do not see the 
HTML on the top of the page.

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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