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

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[harishd]

A spin off from bug 91468.

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

Comment 1

[Jacek Piskozub]

The testcases for this are attached to bug 91468.

Comment 2

[harishd]

--> 0.9.5

Comment 3

[harishd]

--> 0.9.6

Comment 4

[harishd]

Out of time :-( Moving to 0.9.7

Comment 5

[harishd]

--> 0.9.9

Comment 6

[harishd]

Mass moving bugs to 1.1

Comment 7

[Braden]

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

Comment 8

[Eli Friedman]

I don't see this bug (lays out properly, what other problem is there?) (moz
1.1alpha).  Mark fixed?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple testcase

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

[was 1.1alpha]

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The duplicate says this is a regression from bug 43678.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this TABLE-only, or not?  I suspect it may be.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[Mats Palmgren (inactive)]

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.

Comment 15

[Hixie (not reading bugmail)]

Yes; CSS should in no way affect parsing.

Comment 16

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 17

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 19

[Mats Palmgren (inactive)]

Attachment: 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);

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Re comment 18: I filed the separate bug as bug 208846.

Comment 21

[harishd]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's no need to merge until one of the two is checked in -- they really don't
conflict except for the one line.

Comment 23

[harishd]

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.

Comment 24

[harishd]

--> Mats. Since he has done all the work :)

Comment 25

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 26

[Mats Palmgren (inactive)]

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.

Comment 27

[harishd]

> 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! ;) 

Comment 28

[harishd]

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

Nice job Mats. 

r=harishd

Comment 29

[Mats Palmgren (inactive)]

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.

Comment 30

[Mats Palmgren (inactive)]

I don't have cvs commit rights so you or David have to do this for me.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-06-10 21:24 -0700.

Comment 32

[Bill Mason]

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