54651 - <LI> doesn't allow nested <DIV>s

Status: VERIFIED FIXED

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

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE:
   Pass to the parser:
      <UL> <LI> <DIV> text </DIV> </LI> </UL>
   ...or look at the testcase.

EXPECTED RESULTS:
   Parser builds correct content model.

ACTUAL RESULTS:
   Parser doesn't allow DIV to be contained in an LI.

This is a SERIOUS problem with our HTML content sink. Recommend rtm hard stop.
It should be an easy, low risk fix if previous such bugs are anything to go by.

Comment 1

[Hixie (not reading bugmail)]

Attachment: Simple and self explanatory testcase. Output should be green, not red.

Comment 2

[Hixie (not reading bugmail)]

Uriel: This is the bug you found with David's childsel test:
   http://www.people.fas.harvard.edu/~dbaron/css/test/childsel
Good catch.

Comment 3

[rickg]

Odd -- the LI is set up as a block container. Let's see what's going on...

Comment 4

[rickg]

<UL> <LI> <DIV> text </DIV> </LI> </UL> works perfectly for me. 

But this case does not: <DIV><UL><LI><DIV> text </DIV> </LI></UL></DIV>

Let's see why...

Comment 5

[rickg]

Ok -- fixed in my tree. It's an error in the autoclose logic. I'll build a huge 
test suite to look for other cases...

Comment 6

[Hixie (not reading bugmail)]

Taking QA per managerial policy.

Comment 7

[Michael La Guardia]

What sites does this actually break?  Marking rtm- until we get information
about user impact.

Comment 8

[ekrock's old account (dead)]

We should accept this for RTM. The issue is that this kind of error in the
autoclose logic could cause a wide variety of markup sequences to potentially
break. This is backward compatibility with poorly-formed or nested HTML 3.2 on
the web--there's a great deal of that. We don't have time to go out and do an
exhaustive search of the web to turn up examples of this, but knowing that DIVs
are widely mis-nested, I'd prefer to accept the low-risk patch and sleep soundly
rather than find out the hard way *after* ship how many pages this affects.

Comment 9

[Hixie (not reading bugmail)]

PDT, Eric: This is not only a compatability issue. This is an issue with 
_CORRECT_ HTML too! As we encourage people to use structural markup, this
markup will become more and more common. It is probably already very common on 
the web. As I understand it, parser fixes of this kind are remarkably low risk 
and are a _very_ well understood procedure that Rick has done many times before.

Putting back on [rtm+] radar.

Comment 10

[rickg]

Agreed; and this: I'm currently generating an absolutely *HUGE* testset of pages 
to try to catch these before we go out. I'll publish more about it once it's 
working. : )

Comment 11

[rickg]

Here's the patch:

Index: CNavDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v
retrieving revision 3.317
diff -r3.317 CNavDTD.cpp
983,984c983,988
<   PRBool result=PR_TRUE;
<   if(aContext.GetCount()){
---
>   //the changes to this method were added to fix bug 54651...
> 
>   PRBool  result=PR_TRUE;
>   PRInt32 theCount=aContext.GetCount();
> 
>   if(0<theCount){
991c995
<       PRInt32 theBaseIndex=(theRootIndex>theSPIndex) ? theRootIndex : 
theSPIndex;
---
>       PRInt32 theTargetIndex=(theRootIndex>theSPIndex) ? theRootIndex : 
theSPIndex;
993c997,998
<       if((theBaseIndex==theChildIndex) && 
(gHTMLElements[aChildTag].CanContainSelf()))
---
>       if((theTargetIndex==theCount-1) ||
>         ((theTargetIndex==theChildIndex) && 
gHTMLElements[aChildTag].CanContainSelf())) {
995c1000,1018
<       else result=PRBool(theBaseIndex>theChildIndex);
---
>       }
>       else {
>         
>         result=PR_FALSE;
> 
>         PRInt32 theIndex=theCount-1;
>         while(theChildIndex<theIndex) {
>           eHTMLTags theParentTag=aContext.TagAt(theIndex--);
>           if (gHTMLElements[theParentTag].IsMemberOf(kBlockEntity)  || 
>               gHTMLElements[theParentTag].IsMemberOf(kHeading)      || 
>               gHTMLElements[theParentTag].IsMemberOf(kPreformatted) || 
>               gHTMLElements[theParentTag].IsMemberOf(kList)) {
>             if(!HasOptionalEndTag(theParentTag)) {
>               result=PR_TRUE;
>               break;
>             }
>           }
>         }
>       }

Comment 12

[Loco]

Hixie's reasoning seems fairly accurate-if we're trying to get people to do 
things differently (our way?) we should at least have something working when they 
take us up on the offer.

Comment 13

[rickg]

The code has been reviewed and superreviewed by buster.

Comment 14

[Phil Peterson]

Rick, we collided as I was marking need info :-)  [rtm++] since you have the
reviews.

Comment 15

[rickg]

Fixed by small improvement to code that allows a child container to veto 
containment in a parent.

Comment 16

[Asa Dotzler [:asa]]

Verified Fixed on trunk builds testcase was Green
linux 101808 RedHat 6.2
win32 101804 NT 4
mac 101804 Mac OS9
Setting bug to Verified and removing vtrunk keyword