Closed Bug 22480 Opened 25 years ago Closed 14 years ago

Unclosed <p> causes <form> to remain open

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neilconway, Unassigned)

References

()

Details

(Keywords: testcase, Whiteboard: [fixed by the HTML5 parser])

Attachments

(3 files, 3 obsolete files)

Here is the test case - found using Linux 2.2, glibc 2.1.2, M12.
--
<html>
<body>
<form>
<input type=text><p>
<input type=submit>
</form>This text block should be on the next line
</body>
</html>
--

When viewed from Netscape 4.7 and IE 5.5, the "this text...." block is on the
next line - as though a <p> were automatically inserted. However, in Mozilla it
is not, and the "this text" block is on the same line as the Submit button.

Since both major browsers render it the same way, I assume the "This text..."
block SHOULD be on the next line - however, I couldn't figure out from the HTML
4 spec if this is correct or not.
Assignee: karnaze → rods
Reassigning to Rod.
Assignee: rods → rickg
parser issue
I think this is a layout problem, not a parser problem. The parser doesn't
actually care that the form is open.
Assignee: rickg → kipp
Summary: Submit control, no subsequent paragraph → [BLOCK] Submit control, no subsequent paragraph
Target Milestone: M15
added [BLOCK] M15.
mine! mine mine mine!  all mine!  whoo-hoo!
Assignee: kipp → buster
moving all buster m15 bugs to m16.
Target Milestone: M15 → M16
moving non-critical bugs to M17
Target Milestone: M16 → M17
Changing component from "HTML form controls" to "layout".
Component: HTML Form Controls → Layout
I think this is a parser bug.
Harish, here's a content dump.  Notice the second input and text are both inside 
the P. The text should not be.

html@02363FC8 refcount=3<
  head@02365968 refcount=2<
  >
  Text@0254EB40 refcount=3<\n>
  body@025495D8 refcount=3<
    Text@02500F60 refcount=3<\n>
    form@025006CC refcount=3<
    >
    Text@02500360 refcount=3<\n>
    input@0250022C type=text refcount=4<>
    p@025055E8 refcount=3<
      Text@02505570 refcount=3<\n>
      input@025054CC type=submit refcount=4<>
      Text@02505260 refcount=3<\n>
      Text@02507FB0 refcount=3<This text block should be on the next line\n>
    >
  >
>

should be a trivial fix.
Assignee: buster → harishd
OS: other → All
Priority: P3 → P2
Hardware: Other → All
Marking 4xp. dbaron, what is the Right Thing here per the standards? Clearly 
this is lousy HTML, but we may wish to be b.c. as a navquirk, and want to do the 
Right Thing in strict. 

This bug is also a promising candidate to FUTURE however as it's bad HTML and 
I'm not sure it's widely used, and there's a workaround if we're not b.c.: "Fix 
your HTML!" 
Keywords: 4xp
This is actually valid HTML (or trivially turned into it, by adding a TITLE
element and giving the FORM element some required attributes).
Hm, why is the FORM element empty in the content dump?

Could someone attach the content dump of the page as it shows when there is
a strict DOCTYPE at the top? If the content is then in the form, then it should
be fixed in strict mode since FORM has "display:block". I'm not sure how it 
would get fixed in Quirks mode though.
Blocks: html4.01
Sorry guys, you're mistaken. The parser is doing the right thing with this 
(legal) markup. The contentSink should be handling the container-ship of the 
form, not the parser (which is sending the correct commands). 
No excuse for not handling valid markup. Nominating for nsbeta3.

Btw, as rickg mentioned..this is nothing to do with the parser.  This ought to 
be fixed on the sink/layout.  
Status: NEW → ASSIGNED
Keywords: nsbeta3
Target Milestone: M17 → M18
correctness, cross-browser compat, backward compat.
Keywords: correctness
Unless this is a problem that is visible on highly trafficked sites, we are 
inclined to fix this post rtm.  So, marking future.  Please comment if you 
disagree.
Target Milestone: M18 → Future
This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 
Keywords: nsbeta3
Removed nsbeta3 nomination from futured bugs.
Should not remove nsbeta3 nomination.  Thanks to dbaron and ekrock for the heads
up.  Re-adding the nomination and marking nsbeta3-.
Keywords: nsbeta3
Whiteboard: [nsbeta3-]
Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that
do not have the "testcase" keyword and yet have an attachement with the word
"test" in the description field. Apologies for any mistakes.
Keywords: testcase
Target Milestone: Future → mozilla0.9
Keywords: nsbeta3mozilla0.9, nsbeta1
Whiteboard: [nsbeta3-]
This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 
Target Milestone: mozilla0.9 → Future
->gerardok
QA Contact: ckritzer → gerardok
QA Contact: gerardok → petersen
Keywords: nsbeta1nsbeta1-
Target Milestone: Future → mozilla0.9.1
nsbeta1-, but still moving to 0.9.1 in the hopes that it can be fixed in time.
It would be nice to have 100% HTML 4.01 compliance.
Not yet highly visible.


This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Target Milestone: mozilla0.9.1 → Future
Attached patch Proposed patch (obsolete) — Splinter Review
This does the right thing, methinks... All reasonable tag closures other than
form call CloseContainer() on the sink context unconditionally; </form> only
does so if the container currently open is a form.  I don't know whether we can
make </form> unconditionally close forms (assuming there is a form open, of
course; the problem is that the DTD does not fixup forms, so we can have wildly
malformed stuff coming through), but closing out an open <p>, if any, should be
pretty safe....
Taking...
Assignee: harishd → bzbarsky
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.4beta
Summary: [BLOCK] Submit control, no subsequent paragraph → [FIX][BLOCK] Submit control, no subsequent paragraph
Comment on attachment 120376 [details] [diff] [review]
Proposed patch

Harish?  Heikki?  Thoughts?
Attachment #120376 - Flags: superreview?(heikki)
Attachment #120376 - Flags: review?(harishd)
Comment on attachment 120376 [details] [diff] [review]
Proposed patch

>   if (mCurrentForm) {
>+    // If there is a <p> tag on the container stack, close it
>+    if (mCurrentContext->IsCurrentContainer(eHTMLTag_p)) {
>+      result = mCurrentContext->CloseContainer(eHTMLTag_p);
>+    }
>+ 

What about tags other than P? What if there was DIV? May be you should check if
the last tag is a container and if it is then close the container. Still, I
don't like this fix up to be in the sink.
Sure, I could do a container-check instead.  I'm not sure we can move this stuff
down into the DTD, because the sink is already handling so much form fixup.... 
In particular, making the DTD enforce containership for forms by fixing things
up would likely break things all over... :(

Oh, I recall now why I only did <p>.  It's because it has an optional end tag. 
So in that case we're breaking completely valid markup. If someone forgets to
close a <div> before closing a <form>, they deserve what they get, imo.
In any case, if people want me to do anything bigger than that patch (and I'm
loath to do that), this will not be happening for 1.4.
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Closing <div> when you close <form> would give parity with the other tags (i.e.
do whatever we do in normal CloseContainer()) and might give us IE parity on
attachment 82130 [details].  I can't recall why we didn't do that before, or whether we
discussed the issue.  But closing all tags with </form> just before this major
release is askin' for trouble--1.5 *would* make sense.

This patch seems reasonable to me for 1.4, but we need to file a bug to decide
on the issue of all tags.
Comment on attachment 120376 [details] [diff] [review]
Proposed patch

Add a comment explaining why you're only checking for P tag. With that
r=harishd.
Attachment #120376 - Flags: review?(harishd) → review+
Comment on attachment 120376 [details] [diff] [review]
Proposed patch

sr=heikki, please land during 1.5alpha (as this is currently scheduled). There
don't seem to be any real word sites listed here and there are no duplicates,
so there is no reason to take the regression risk for 1.4 at this point.
Attachment #120376 - Flags: superreview?(heikki) → superreview+
Summary: [FIX][BLOCK] Submit control, no subsequent paragraph → [FIXr][BLOCK] Submit control, no subsequent paragraph
Patch checked in, bug 209434 filed per comment 33.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
checkin here resulted in bug 209504
Reopening; I backed this patch out because of bug 209504 and duplicates.

The problem there was that given this markup:

<body>
  <form><p>
  </form>
</body>

CNavDTD sends the following to the sink, in order:

OpenBody()
OpenForm()
OpenContainer(eHTMLTag_p /* parser node, actually */)
CloseForm()
CloseContainer(eHTMLTag_p)
CloseBody()

That CloseContainer() comes from the CloseContainersTo(mBodyContext->Last()).

So the problem is that the sink has no way to let the DTD know that it's closed
out the <p>, and the sink context cannot reasonably enforce the fact that the
tag being closed corresponds to what's currently on the stack (since I suspect
that we have cases when we will in fact be closing one tag, while another is on
the context stack and "correct" behavior will rely on closing whatever is on the
context stack....).  If this suspicion is wrong, then one could try to fix this
bug by doing what I did and adding a check in SinkContext::CloseContainer to
make sure that the tag on stack and the argument match, doing nothing if they do
not.

In any case, I am somewhat out of my depth at this point, so punting back to
default owner...
Status: RESOLVED → REOPENED
Priority: P2 → --
Resolution: FIXED → ---
Summary: [FIXr][BLOCK] Submit control, no subsequent paragraph → [BLOCK] Submit control, no subsequent paragraph
Target Milestone: mozilla1.5alpha → ---
This is totally a parser bug.
Assignee: bzbarsky → harishd
Blocks: 209434
Status: REOPENED → NEW
Component: Layout → Parser
QA Contact: petersen → dsirnapalli
Attached file Testcase #2
Attached patch Patch2 (obsolete) — Splinter Review
Here is a patch for CNavDTD worth considering perhaps, it closes out the
nearest
container that has an optional end tag and can be contained by <form>.
(it matches <p>, <dd>, <dt> and <li>).	It's a compromise but I think it works
well for the problem we are trying to fix.
Attachment #120376 - Attachment is obsolete: true
Whiteboard: [patch]
Attachment #126404 - Flags: review?(harishd)
Attached patch Alternative patch? (obsolete) — Splinter Review
This patch will auto close any container inside FORM. 

Mats, what you think?

Note: I have not tested this patch for regressions...yet.
Nice idea, but I think it's a bit too aggressive.
I ran it through Choffman's list and of 266 URLs it changed 70.  Most of them
probably harmless, but a few were really broken, just to mention a couple:
http://www.infospace.com/
http://hotjobs.com/

I added a test so root-elements terminated the loop, that helped
http://www.infospace.com/ so I guess they have a tag soup like:
<form><table><tr><td>.... </form>  <td> ... </table>

Still, there is something important missing here, the saved index for the
<form> can be invalid at the point we find a </form>.  An example:

<html><body>
     <div><div><div><p> <form></div></div></div>
               <div><p></form></div>

When we see </form> we have NS_DTD_FLAG_HAS_OPEN_FORM and mFormIndex=5 but
the stack size is 4.  So we need a check when we pop the stack so that if
the size gets below the current mFormIndex it has to be reset to -1.
The problem with that is that we would miss to close the <p> before the
</form> above - unless we have something like my patch as a fallback.

We should also remember that NS4/IE seems to view stray </form>s as rather
weak creatures that do not close any containers at all (merely introduce
a line break).  So, maybe we should limit closing containers based on
mFormIndex to Standard mode and have what I suggested as a fallback
when mFormIndex=-1 and for Quirks mode?
A safer way of "marking" the stack position where the <form> was found would
be to introduce a bit on the parse node itself, so the bit disappears when
that tag is popped.  I actually tried that at some point and could revive the
code if I still have it around...  what do you think?
>When we see </form> we have NS_DTD_FLAG_HAS_OPEN_FORM and mFormIndex=5 but
>the stack size is 4.  So we need a check when we pop the stack so that if
>the size gets below the current mFormIndex it has to be reset to -1.

Good catch.

>A safer way of "marking" the stack position where the <form> was found would
>be to introduce a bit on the parse node itself, so the bit disappears when
>that tag is popped.

May be. But that will bloat ( not much though ) the parser node just to handle
the FORM case.

Anyway, I'm not sure if it is worth it to shake up the parser to address this
bug :-). 
> But that will bloat ( not much though ) the parser node

I think I know a way around that... having a 'eHTMLTag_form_marker' entry on the
stack (no nsCParserNode)... doing some experiments with that, just for fun :-)
*** Bug 166461 has been marked as a duplicate of this bug. ***
Attachment #126404 - Attachment is obsolete: true
Attachment #126404 - Flags: review?(harishd)
Attachment #126838 - Attachment is obsolete: true
Blocks: 211728
Blocks: 182824
Updating summary to reflect the bug here and marking a dependency.
Assignee: harishd → parser
Depends on: 136397
QA Contact: dsirnapalli → mrbkap
Summary: [BLOCK] Submit control, no subsequent paragraph → Unclosed <p> causes <form> to remain open
Whiteboard: [patch]
*** Bug 271885 has been marked as a duplicate of this bug. ***
*** Bug 293300 has been marked as a duplicate of this bug. ***
*** Bug 324770 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7 

Consider the following (broken?) html:

<html>
	<body>
		<div>
		<form name="test" action="test" id="test">
				<input type="text" name="text1" id="text1" />
				<input type="text" name="text2" id="text2" />
				</div>
				<input type="text" name="text3" id="text3" />
				<input type="text" name="text4" id="text4" />
				<input type="text" name="text5" id="text5" />
				<input type="text" name="text6" id="text6" />
		</form>
	</body>
</html>

<script type="text/javascript" > 
function doit() {
	var form1 = document.getElementById("test");
	var elem1 = form1.getElementsByTagName("*");
	var elem1Length = elem1.length;
	var form1ElementsLength = form1.elements.length;
		
	alert("lengths: elem1Length: " + elem1Length + ", form1ElementsLength:" 
	+ form1ElementsLength);
}

doit();

</script>

The above script will print elem1Length: 2 , form1ElementsLength: 6 since getElementByTagName only returns the first two input tags before the closing div but the property form1.elements will contain all six of them.

Is this the expected and correct behaviour? 
Yes, that is the expected and correct behavior.  We should add a regression test for it, in fact.
I checked in that regression test.
Judging from some of the comments above, should this be WONTFIX?
Actually, I believe this is fixed by bug 487949.
Depends on: html5-parsing
Assignee: parser → nobody
QA Contact: mrbkap → parser
Status: NEW → RESOLVED
Closed: 21 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by the HTML5 parser]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: