Closed Bug 22480 Opened 25 years ago Closed 15 years ago

Unclosed <p> causes <form> to remain open

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: neilconway, Unassigned)

References

(
URL
)

Details

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

Attachments

(3 files, 3 obsolete files)

attached the test case for your convenience
134 bytes, text/html
Details
Testcase #2
1.34 KB, text/html
Details
Screenshot of Testcase #2 with Patch2 applied
22.09 KB, image/png
Details
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: nsbeta3 → mozilla0.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: nsbeta1 → nsbeta1-
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 ago → 15 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: