Closed Bug 289537 Opened 19 years ago Closed 19 years ago

Blank page - <script> after <img> in <head> should also move to body?

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: mrbkap)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(4 files, 2 obsolete files)

Testcase
449 bytes, text/html
Details
results for IE6
1.97 KB, text/plain
Details
cleaned up version
12.68 KB, patch
jst
: review+
rbs
: superreview+
asa
: approval1.8b3+
Details | Diff | Splinter Review
followup fixes
4.46 KB, patch
jst
: review+
jst
: superreview+
benjamin
: approval1.8b3+
Details | Diff | Splinter Review 
The site sometimes shows up blank after a short while. 
This happens particularly when reloading or going back to the page.
Basically, I think this happens because an <img> tag is inside the head and a
<script> tag in the <head> tries to access that <img>.
When that fails, the same function is called but with a timer of 1500ms.
So when it is called the second time, the <img> can be accessed. But then the
document.write code is being carried out and that's why the page is becoming
blank, eventually.
I think this would mean, it should fail every time, I haven't figured out why
Mozilla manages to display the page sometimes.

I'll attach a testcase, which shows -I think - essentially the difference
between the browsers.
Attached file Testcase (obsolete) — 
DOM tree Mozilla:
html
  head
    title
    script
  body
    img

DOM tree IE6:
html
  head
    title
  body
    img
    script

So IE6 is moving the <script> also into the <body> when an <img> is found in
the <head> (which is also moved into the <body>). It doesn't do that with a
<div> for instance. If you like, I can look which elements in IE6 do the same
as <img>.

Opera7.54 moves the <script> into the body on both occasions: when an <img> is
found in the head or when a <div> is found in the <head>
You may want to modify the text "Testcase bug 289537 - Blank page - <script>
after <img> in <head> should also move to body?" where "<" and ">" be changed to
HTML code where the browser can parse them as "<" or ">" since you meant them to
be texts for viewing, not something for the the web browser to parse those tags.

That way, we'll have a better testcase once you make one on why it work or not
work with variety of browsers.
Attached file Testcase — 
Oops! You're totally right. I missed that completely.
Attachment #180037 - Attachment is obsolete: true
Martijn, if you could come up with a list, that would be totally awesome!
Assignee: parser → mrbkap
OS: Windows 2000 → All
Hardware: PC → All
Attached file results for IE6 — 
Ok, these are my results for IE6. Some explanation:
not indented means the tag is moved into the <body>, but the <script> not.
Indented means the tag and the <script> got moved into the <body>, unless I
stated that it stayed in the <head>, then the <script> also stays in the
<head>.

Another observation: any stray/loose text in the head is moved into the <body>.
<script> tags after those text also get moved to the <body> in IE6.
So my test results are specifically for empty tags. When there is text in the
tag, then it gets moved into the <body> (unless stated otherwise), and the
<script> with it.
Status: NEW → ASSIGNED
Attached patch patch (untested) (obsolete) — — Splinter Review 
This patch should make us act like IE in these sorts of situations. Once I have
some other changes out of my tree, I'll be able to actually test it. The idea
here is to use the existing kRequiresBody special property, which opens up a
body when the element is encountered on _all_ of the elements IE opens it for.
This uses the table for bug 180157 as a basis. The problem was simply that not
enough elements had the kRequiresBody. When we encounter an element with this
flag in the <head> section of a document, we open a body tag (so that any
scripts, along with whitespace and userdefined tags are pushed into the body)
which is the correct behavior for these tags.
Attachment #184150 - Attachment is obsolete: true
Attachment #184339 - Flags: review?(jst)
Comment on attachment 184339 [details] [diff] [review]
cleaned up version

>-    /*special parents,kids,skip*/       &gInForm,&gContainsText,eHTMLTag_textarea,
>+    /*special parents,kids,skip*/       &gInForm,&gContainsText,eHTMLTag_unknown,

Please ignore this change, it's part of another bug.
Blocks: 293162
No longer blocks: 293162
Comment on attachment 184339 [details] [diff] [review]
cleaned up version

r=jst
Attachment #184339 - Flags: review?(jst) → review+
Attachment #184339 - Flags: superreview?(rbs)
Comment on attachment 184339 [details] [diff] [review]
cleaned up version

sr=rbs, I never cease to be amazed by these parser trickeries to deal with tag
soup.

>This uses the table for bug 180157 as a basis.

typo on that bug number, it doesn't lead to a table.
Attachment #184339 - Flags: superreview?(rbs) → superreview+
Comment on attachment 184339 [details] [diff] [review]
cleaned up version

This might fix a few pages that rely on us opening the <body> implicitly for
certain elements. It should be about medium-risk.
Attachment #184339 - Flags: approval1.8b3?
I didn't mean bug 180157, but attachment 180157 [details], sorry.
Attachment #184339 - Flags: approval1.8b3? → approval1.8b3+
Fix checked in. I'm leaving this bug open because I think I may have
misinterpreted a couple of entries in the table.
Ok, I've only tested the indented tags from attachment 180157 [details], here are my results:

First off all, I see no regressions from this checkin.
But I see probably an earlier regression, probably from fixing bug 285250:
bgsound - this stayed in the head in Mozilla1.7, but is moved to the body in
current trunk builds.

I see improvements/fixes for:
br, dd, embed, hr, img, li, marquee, pre, select, table (but IE also generates
tbody with it), wbr

Here the tags that are still wrong:

These are tags that just get removed from the dom (which Mozilla always did):
area, caption, col, colgroup, frame, option, param, tbody, td, tfoot, th, thead, tr
That's not what IE6 is doing. In IE6, these elements stay in the head.

"It stays in the head in IE6"-tags:
form, frame, input type=hidden, map, multicol
optgroup (also creates form and select tag in Mozilla)

"tag and the <script> get moved into the <body> in IE6"-tags:
isindex, iframe, input

This one is special:
title - the first title is used, a second one is removed from the dom, but the
id attribute is placed to the first one in IE6, Mozilla just throws the whole
tag away.
Maybe it's worth to file a new bug for this?
(In reply to comment #14)
> First off all, I see no regressions from this checkin.
> But I see probably an earlier regression, probably from fixing bug 285250:
> bgsound - this stayed in the head in Mozilla1.7, but is moved to the body in
> current trunk builds.

Doh! I managed to not fix that bug. I'll attach a new patch here and try to get
that in today.

> These are tags that just get removed from the dom (which Mozilla always did):
> area, caption, col, colgroup, frame, option, param, tbody, td, tfoot, th,
thead, tr
> That's not what IE6 is doing. In IE6, these elements stay in the head.

I'm going to leave these tags alone for now. We "omit" these tags because they
require another tag (such as <map>, in the case of <area> to be present before
we open them) and they don't make sense without some special parent tags. Also
note that most of these are tags that live inside of <table>.

> 
> "It stays in the head in IE6"-tags:
> form, frame, input type=hidden, map, multicol
> optgroup (also creates form and select tag in Mozilla)

Note that <multicol> is a Netscape 4.x thing only. IE is probably treating it as
a userdefined tag.

I'm worried about allowing <form> in head because I suspect it would be
impossible to submit such a form (any elements in it would be invisible anyway)
and that seems like it'd be a bigger bug than this anyway. Perhaps we'll be able
to come up with something better for 1.9.

> "tag and the <script> get moved into the <body> in IE6"-tags:
> isindex, iframe, input

I see the problem with <isindex> (which, according to the standard, should be
allowed in the <head>, but we don't allow there because the isindex then fails
to appear), but we should be moving <iframe> and <input type!=hidden> into the
body along with the script now.

> 
> This one is special:
> title - the first title is used, a second one is removed from the dom, but the
> id attribute is placed to the first one in IE6, Mozilla just throws the whole
> tag away.

I think this should be fixed with the big patch in bug 272702. Since we're so
close to the branch point, we should probably just wait until that patch goes in.

I'll attach a patch to fix the problems noted.
Attached patch followup fixes — — Splinter Review 
This fixes <bgsound> and <isindex>. There's also a fix in CNavDTD so that the
HTML document "<bgsound>" causes a <bgsound> in the head instead of the body,
since that's what I think would be expected.
Attachment #187436 - Flags: superreview?(jst)
Attachment #187436 - Flags: review?(jst)
Comment on attachment 187436 [details] [diff] [review]
followup fixes

	   isExclusive = !(mFlags & NS_DTD_FLAG_HAD_BODY);
	   mFlags |= NS_DTD_FLAG_HAS_OPEN_SCRIPT;

	 default:
	     break;
       }//switch

       if(!isTokenHandled) {
	 if(theHeadIsParent &&
-	     (isExclusive || (mFlags & NS_DTD_FLAG_HAS_OPEN_HEAD))) {
+	     (isExclusive || !(mFlags & NS_DTD_FLAG_HAD_BODY))) {


looking quickly at how isExclusive is set above, are you thus having a
redundant test?
(In reply to comment #17)
> looking quickly at how isExclusive is set above, are you thus having a
> redundant test? 
> 

That's only in the case that aChildTag is eHTMLTag_script. In most cases
isExclusive is returned from a call to IsChildOfHead(). Unfortunately,
IsChildOfHead() is not smart enough to return the correct answer for <script>,
so I hack it in the DTD.
Comment on attachment 187436 [details] [diff] [review]
followup fixes

r+sr=jst
Attachment #187436 - Flags: superreview?(jst)
Attachment #187436 - Flags: superreview+
Attachment #187436 - Flags: review?(jst)
Attachment #187436 - Flags: review+
Attachment #187436 - Flags: approval1.8b3?
Attachment #187436 - Flags: approval1.8b3? → approval1.8b3+
The followup fix has been checked in. Marking this as FIXED, even noting the
problems pointed out in comment 14. Thanks again Martijn!
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> I see the problem with <isindex> (which, according to the standard, should be
> allowed in the <head>, but we don't allow there because the isindex then fails
> to appear), but we should be moving <iframe> and <input type!=hidden> into the
> body along with the script now.
<isindex> is working now as IE6.
But with <input type="hidden">, the script isn't moved into the body (so if you
would fix it, it would act like the rest of the <input> tags).
This is also the case with <iframe>, for which you specifically said in comment
15 that the script would move along into the body.
So you might want to fix those two tags.

For the rest, you're probably right to leave those differences alone.
Thank you!
I filed bug 299268 on the problem with <iframe>. <input type=hidden> is
deliberatly doesn't open a <body> in order to not regress bug 66985.
This caused bug 307122.
Depends on: 307122
This also caused bug 318365.
Depends on: 318365
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: