Last Comment Bug 167493 - eHTMLTag_userdefined should be included as a valid head kid
: eHTMLTag_userdefined should be included as a valid head kid
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.4beta
Assigned To: harishd
: Moied
Mentors:
: 170413 (view as bug list)
Depends on:
Blocks: 170413 195260
  Show dependency treegraph
 
Reported: 2002-09-09 06:50 PDT by Thu-Van Luu
Modified: 2011-08-05 21:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (563 bytes, patch)
2002-09-09 07:56 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Review
Patch v1.1 (1.89 KB, patch)
2003-05-01 16:10 PDT, harishd
no flags Details | Diff | Review
Patch v1.2 (2.29 KB, patch)
2003-05-02 11:22 PDT, harishd
nisheeth_mozilla: review+
jst: superreview+
sspitzer: approval1.4b+
Details | Diff | Review

Description Thu-Van Luu 2002-09-09 06:50:10 PDT
User-Agent:       Mozilla/4.75 [en] (X11; U; Linux 2.2.12 i386)
Build Identifier: Mozilla/5.0 (X11;U; FreeBSD i386;en-US;rv:1.0rc1) Gecko/200020905

If I make my own XML element in the HTML <HEAD> of a frameset it isn't
recognized/created and I can't do any XBL binding to it...

<HTML>
<HEAD>
<MyElement Help> </MyElement>
</HEAD>
<Frameset>  
   <frame>
</frameset>
</html>

Reproducible: Always

Steps to Reproduce:
1.Load a HTML frameset document with a user defined element in the<HEAD>
2.Try to access the element or do some XBL binding...element wasn't created.

Actual Results:  
Nothing

Expected Results:  
Created the user defined element in the DOM model and call the XBL constructor.
Comment 1 Randell Jesup [:jesup] 2002-09-09 07:25:29 PDT
Taking bug, will attach a patch

harishd: Could you rule on whether we're correct that userdefined should be
valid in <head>?
Comment 2 Randell Jesup [:jesup] 2002-09-09 07:56:47 PDT
Created attachment 98435 [details] [diff] [review]
Patch

Patch to add userdefined to the gHeadKids list
Comment 3 Randell Jesup [:jesup] 2002-09-09 07:57:32 PDT
Ready for review
Comment 4 harishd 2002-09-09 10:05:41 PDT
> 2.Try to access the element or do some XBL binding...element wasn't created.

That's not good. IMO, parser should not restrict any tag ( even userdefined ) from 
being a part of the content model. Also, userdefined tags have no specific rules
and hence MAY appear in the head context.
Comment 5 harishd 2002-09-09 10:07:01 PDT
Comment on attachment 98435 [details] [diff] [review]
Patch

Please add a one line comment with bug #. With that r=harishd
Comment 6 Randell Jesup [:jesup] 2002-09-11 16:16:56 PDT
I'll add a comment on the end with the bug #.  jst, sr?
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2002-09-11 16:44:39 PDT
Comment on attachment 98435 [details] [diff] [review]
Patch

sr=jst

But please, diff -u next time!
Comment 8 Randell Jesup [:jesup] 2002-09-12 13:14:05 PDT
Fix checked in
Comment 9 rbs 2002-09-20 18:38:26 PDT
It seems that this patch had some issues. I traced what goes on when a
user-defined tag is outside the <HEAD> and saw something strange happening.
Suppose <MyElement> is inside the <BODY> instead of the <HEAD>. The code will
roll into HandleStartToken() and do what I illustrate below (traced in the
debugger):

nsresult CNavDTD::HandleStartToken(CToken* aToken) {
[...]
//XXXrbs
const nsAString& tag = ((CStartToken*)aToken)->GetStringValue();
if (tag.Equals(NS_LITERAL_STRING("myElement")))
  NS_BREAK();

      PRBool theExclusive=PR_FALSE;
      theHeadIsParent=nsHTMLElement::IsChildOfHead(theChildTag,theExclusive);
>>>with the patch, this *always* returns |theHeadIsParent=true| for a
>>>user-defined tag
     
[...]

#if 0
        //we'll move to this approach after beta...
      if(!isTokenHandled) {

        if(theHeadIsParent && (theExclusive || mOpenHeadCount>0)) {
          result=AddHeadLeaf(theNode);
        }
        else result=HandleDefaultStartToken(aToken,theChildTag,theNode); 
      }
#else
        //the old way...
      if(!isTokenHandled) {
        if(theHeadIsParent || 
          (mOpenHeadCount>0  && (eHTMLTag_newline==theChildTag ||
eHTMLTag_whitespace==theChildTag))) {
            result=AddHeadLeaf(theNode);

>>>since |theHeadIsParent| is true, the smippet above
>>>will *always* add userdefined as a _leaf_ of the head

        }
        else result=HandleDefaultStartToken(aToken,theChildTag,theNode); 
      }
#endif
      //now do any post processing necessary on the tag...
      if(NS_OK==result)
        DidHandleStartTag(*theNode,theChildTag);
    }//if
  } //if

[...]
}
Comment 10 rbs 2002-09-23 16:41:29 PDT
Filed bug 170413 about the regression.
Comment 11 harishd 2002-09-24 09:56:51 PDT
May be all we need is 

...
if (!isTokenHandled) {
  if (theHeadIsParent || 
      (mOpenHeadCount > 0  && (eHTMLTag_newline == theChildTag    || 
                               eHTMLTag_whitespace == theChildTag ||
                               eHTMLTag_userdefined == theChildTag)) 
    result=AddHeadLeaf(theNode);
  }
  else {
    result = HandleDefaultStartToken(aToken, theChildTag, theNode); 
  }
}
...

Note: Patch ( id = 98435 ) should be reverted.
Comment 12 rbs 2002-09-26 03:00:27 PDT
Tried harishd's change but noted with the DOM inspector that it goes back to
square one, i.e, with the change, all userdefined tags get added as children of
the <body>.

Tracing in the debugger, I saw that something extra might be needed in
CNavDTD::OpenContainer(). Indeed, with the change, the code eventually rolls up
to CNavDTD::OpenContainer() and since userdefined isn't one of the special cases
that are there, it falls to the common codepath which pushes it as a child of
the <body>:

  if(!done) {
    [...]
    result=(mSink) ? mSink->OpenContainer(*aNode) : NS_OK; 
    [...]
    mBodyContext->Push(aNode,aStyleStack);
  }
Comment 13 Randell Jesup [:jesup] 2002-09-26 15:46:23 PDT
*** Bug 170413 has been marked as a duplicate of this bug. ***
Comment 14 Randell Jesup [:jesup] 2002-09-26 15:48:43 PDT
I'd like to keep the discussion here.  I'm going to back the patch out as
harishd suggested until we can find a solution.
Comment 15 Bob Clary [:bc:] 2002-11-13 05:29:47 PST
Randell, I am still seeing user defined tags inside body appearing as children
of the head.
http://lxr.mozilla.org/seamonkey/source/htmlparser/src/nsElementTable.cpp#102
shows this patch was not back out yet. 

This causes any XBL which uses user defined tags to fail. 
Comment 16 harishd 2002-11-13 16:47:51 PST
Have backed out the patch.
Comment 17 Bob Clary [:bc:] 2002-11-13 23:32:30 PST
Any chance of getting this backed out of 1.2 as well?
Comment 18 Randell Jesup [:jesup] 2002-11-14 21:39:39 PST
Sorry, I could have sworn I'd backed this out.  Thinking back, I think the tree
was closed at the time.  My apologies.  I'm asking for backout permission for 1.2
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-11-15 07:31:03 PST
a=dbaron for backing out of 1.2
Comment 20 Randell Jesup [:jesup] 2002-11-15 07:34:24 PST
Backed out of 1.2 branch
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-11-28 06:22:01 PST
It looks like the 1.2-branch backout was done incorrectly.  The 9 was not
changed to an 8.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-11-28 06:30:51 PST
I believe this incorrect backout caused bug 182441 and bug 182348.
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-11-28 07:09:03 PST
I've moved tracking of the regressions to bug 182500.
Comment 24 harishd 2003-05-01 16:10:48 PDT
Created attachment 122246 [details] [diff] [review]
Patch v1.1

Treat the userdefined tags similar to whitespace such that it becomes a part of
the head context if there is an open head and becomes a part of the body
otherwise.
Comment 25 harishd 2003-05-01 17:48:01 PDT
FYI: Parser regression test PASSED. Also, ran the patch against the urls listed
under Debug->Verifications menu. URLs loaded fine.
Comment 26 harishd 2003-05-02 11:22:17 PDT
Created attachment 122313 [details] [diff] [review]
Patch v1.2

Same as patch v1.1 but with a mild clean up.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-02 12:08:17 PDT
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

sr=jst
Comment 28 Nisheeth Ranjan 2003-05-02 13:34:02 PDT
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

looks good.
Comment 29 harishd 2003-05-02 13:37:37 PDT
This fix is essential to unblock 195260 ( which is a serious bug ) and therefore
nominating for 1.4b.

Taking the bug.
Comment 30 (not reading, please use seth@sspitzer.org instead) 2003-05-02 13:43:05 PDT
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

a=sspitzer
Comment 31 harishd 2003-05-02 13:58:13 PDT
Fix landed.

Note You need to log in before you can comment on or make changes to this bug.