P should not contain TABLE (block-level elements) in strict mode

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
HTML: Parser
P3
normal
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: harishd, Assigned: mats)

Tracking

Trunk
mozilla1.5alpha
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
A spin off from bug 91468.

Ex. <p><table></table> should become <p></p><table></table> in strict mode.

Updated

17 years ago
OS: Windows 2000 → All

Updated

17 years ago
Keywords: correctness

Comment 1

17 years ago
The testcases for this are attached to bug 91468.

Updated

17 years ago
Blocks: 7954
(Reporter)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.4
(Reporter)

Updated

17 years ago
Priority: -- → P2
(Reporter)

Updated

17 years ago
Priority: P2 → P1
(Reporter)

Comment 2

17 years ago
--> 0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Reporter)

Updated

17 years ago
Priority: P1 → P3

Updated

16 years ago
QA Contact: bsharma → moied
(Reporter)

Comment 3

16 years ago
--> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Reporter)

Comment 4

16 years ago
Out of time :-( Moving to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Reporter)

Comment 5

16 years ago
--> 0.9.9
Target Milestone: mozilla0.9.7 → mozilla0.9.9
(Reporter)

Updated

16 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
(Reporter)

Comment 6

16 years ago
Mass moving bugs to 1.1
Target Milestone: mozilla1.0 → mozilla1.1

Comment 7

16 years ago
*** Bug 109806 has been marked as a duplicate of this bug. ***

Comment 8

16 years ago
I don't see this bug (lays out properly, what other problem is there?) (moz
1.1alpha).  Mark fixed?
Created attachment 91247 [details]
simple testcase
[was 1.1alpha]
Target Milestone: mozilla1.1alpha → Future
The duplicate says this is a regression from bug 43678.
Is this TABLE-only, or not?  I suspect it may be.
Summary: P should not contain block-level elements in strict mode → P should not contain TABLE (block-level elements) in strict mode
*** Bug 208703 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 14

15 years ago
How about <p><table style="display:inline-table"> - do we still want to close
the <p> in standards mode?  If yes, then I have a 3 line patch for CNavDTD.cpp
that fixes this bug.
Yes; CSS should in no way affect parsing.
(Assignee)

Comment 16

15 years ago
Created attachment 125231 [details] [diff] [review]
Patch rev. 1 (naiv)

OK, good.  I realize now that my first attempt probably isn't general enough
since |CanContain(...)| is called in more places.  I'm attaching it anyway if
you want to comment on it.
Instead, I'm now going to add a |nsDTDMode| parameter to |CanContain| which I
think is the right approach.
(Assignee)

Comment 17

15 years ago
Created attachment 125252 [details] [diff] [review]
Patch rev. 2

This patch removes TABLE from set of tags generally allowed in P (from |gInP|)
and instead dynamically allows it in |nsHTMLElement::CanContain(...)| for
Quirks mode only.

All parser regression tests pass, except "43678.html" - this is because this
file triggers Standards mode and has a <p><table> in it, so this is OK.

All layout/block regression tests pass, except "table/bugs/bug104898.html" -
for the same reason as above.
Attachment #125231 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #125252 - Flags: review?(harishd)
Comment on attachment 125252 [details] [diff] [review]
Patch rev. 2

>-static const TagList  gInP={2,{eHTMLTag_span,eHTMLTag_table}}; // added table for bug 43678, removed FORM bug 94269
>+// gInP: nsHTMLElement::CanContain() also allows table in Quirks mode for bug 43678, removed FORM bug 94269
>+static const TagList  gInP={2,{eHTMLTag_span}};

This will cause reading past the end of the array, since you didn't change the
length.  Is there a bug on making these arrays not have internal lengths, but
instead use NS_ARRAY_LENGTH (a pair of |sizeof|s)?

Also, does eHTMLTag_span mean all phrasal elements, or is SPAN special?  (If
the latter, why?)
(Assignee)

Comment 19

15 years ago
Created attachment 125254 [details] [diff] [review]
Patch rev. 3

Corrected the length argument.

Maybe this answers your question regarding eHTMLTag_span:

    Initialize( 
    
	  // I made span a special% tag again, (instead of inline).
	  // This fixes the case:  <font color="blue"><p><span>text</span>

      /*tag*/				  eHTMLTag_span,
      /*req-parent excl-parent*/	  eHTMLTag_unknown,eHTMLTag_unknown,
	    /*rootnodes,endrootnodes*/		&gRootTags,&gRootTags,	
      /*autoclose starttags and endtags*/ 0,0,0,0,
      /*parent,incl,exclgroups*/	  kSpecial,
(kInlineEntity|kSelf|kFlowEntity), kNone,   
      /*special props, prop-range*/	  0,kDefaultPropRange,
      /*special parents,kids,skip*/	  0,0,eHTMLTag_unknown);
Attachment #125252 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #125252 - Flags: review?(harishd)
(Assignee)

Updated

15 years ago
Attachment #125254 - Flags: review?(harishd)
Re comment 18: I filed the separate bug as bug 208846.
(Reporter)

Comment 21

15 years ago
Mats: Could you please post a patch integrated with dbaron's patch ( bug 208846 ).
I'll take a look into your Patch rev.3 today.
There's no need to merge until one of the two is checked in -- they really don't
conflict except for the one line.
(Reporter)

Comment 23

15 years ago
Comment on attachment 125254 [details] [diff] [review]
Patch rev. 3

Looks fine. However, to be on the safe side please go thro' the top50-100 sites
and run parser regression test ( rudimentary script but good enough to detect
problems ). Also, make sure to test composer.
With that r=harishd.
Attachment #125254 - Flags: review?(harishd) → review+
(Reporter)

Comment 24

15 years ago
--> Mats. Since he has done all the work :)
Assignee: harishd → mats.palmgren
Status: ASSIGNED → NEW
(Assignee)

Comment 25

15 years ago
Created attachment 125356 [details] [diff] [review]
Patch rev. 4

I have updated to the tip so this patch is now merged with David's patch
for bug 208846.  There are no other changes since Patch rev. 3.

> harishd wrote in comment 23:
> go thro' the top50-100 sites
    I will make a separate comment on this that follows.
> run parser regression test
    I did, with good results; see comment 17.
> make sure to test composer
    Sorry, I don't know what I'm expected to do here, please explain.
Attachment #125254 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #125356 - Flags: review?(harishd)
(Assignee)

Comment 26

15 years ago
By the "top50-100 sites" I assume you mean those on "Chofmann's list" under
the Debug menu.  Here's what I did - I saved that list to a file and then
pulled all those sites using 'wget'.  I then ran 'TestParse' on those.
From 219 files that were successfully pulled by 'wget' (28 failed for
some reason and I ignored those), here are the results:
203 were Quirks mode - they passed.
3   were Full Standards mode - they passed.
13  were Almost Standards - 12 passed, 1 had a closed <p> that this patch caused.

The site that had a different parse tree is http://www.planetquake.com/ -
the (one) difference is at the far bottom of the page - the gap between
the last text line "(irc.enterthegame.com #planetquake)" and the
light-brown footer box is going to be 1em less than it is now.
This is because big table that contains all the text is preceeded by a <p>
that will auto-close and thus margin-collapse with itself (no change at
the top) and it's bottom-margin after the table will disappear.

I also went through a few other popular sites manually and saw no difference.
Status: NEW → ASSIGNED
Target Milestone: Future → ---
(Reporter)

Comment 27

15 years ago
> run parser regression test
> I did, with good results; see comment 17.

doh!. Ignore my comment then. :)

> make sure to test composer
> Sorry, I don't know what I'm expected to do here, please explain

Just try to edit any document that contains tables.

>By the "top50-100 sites" I assume you mean those on "Chofmann's list" under
>the Debug menu.

Yes. And thanks for doing the necessary testing. Ship it! ;) 
(Reporter)

Comment 28

15 years ago
Comment on attachment 125356 [details] [diff] [review]
Patch rev. 4

Nice job Mats. 

r=harishd
Attachment #125356 - Flags: review?(harishd) → review+
(Assignee)

Comment 29

15 years ago
Thanks.  I have now done some testing with Composer as well and the results
are as I would expect them to be.  There is no change in behaviour if the
document is in Quirks mode (this seems to be default).  In Standards mode
<p> is auto-closed as expected.  An example:

I started with the following empty Standards mode document, which is
close to the attached "simple testcase":

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<title>Testcase, bug 91927</title>
<p style="color: red;border:1px solid red;">
text
<table>
  <tr>
    <td>This should not be red.</td>
  </tr>
</table>

I opened it in Composer and just did "Save As". I have default Composer prefs.

Before the patch I see Quirky layout in the Composer window and the saved
result file is:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
  <title>Testcase, bug 91927</title>
</head>
<body>
<p style="border: 1px solid red; color: red;">text
<table>
  <tbody>
    <tr>
      <td>This should not be red.</td>
    </tr>
  </tbody>
</table>
</p>
</body>
</html>


After the patch I see Standards mode layout in the Composer window and the
saved result file is:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
  <title>Testcase, bug 91927</title>
</head>
<body>
<p style="border: 1px solid red; color: red;">text
</p>
<table>
  <tbody>
    <tr>
      <td>This should not be red.</td>
    </tr>
  </tbody>
</table>
</body>
</html>


IMO, this is the correct behaviour for a Standards mode document.
(Assignee)

Comment 30

15 years ago
I don't have cvs commit rights so you or David have to do this for me.
Attachment #125356 - Flags: superreview+
Fix checked in to trunk, 2003-06-10 21:24 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5alpha

Comment 32

15 years ago
*** Bug 211091 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.