Closed Bug 167493 Opened 22 years ago Closed 21 years ago

eHTMLTag_userdefined should be included as a valid head kid

Categories

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

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.4beta

People

(Reporter: tvluu, Assigned: harishd)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch v1.2
2.29 KB, patch
nisheeth_mozilla
: review+
jst
: superreview+
sspitzer
: approval1.4b+
Details | Diff | Splinter Review 
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.
Taking bug, will attach a patch

harishd: Could you rule on whether we're correct that userdefined should be
valid in <head>?
Assignee: harishd → rjesup
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: mozilla1.0.2, mozilla1.2
OS: FreeBSD → All
Hardware: PC → All
Summary: eHTMLTag_userdefined should be included in the as a valid head kid → eHTMLTag_userdefined should be included as a valid head kid
Attached patch Patch (obsolete) — — Splinter Review 
Patch to add userdefined to the gHeadKids list
Ready for review
Status: NEW → ASSIGNED
Keywords: patch, review
> 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 on attachment 98435 [details] [diff] [review]
Patch

Please add a one line comment with bug #. With that r=harishd
Attachment #98435 - Flags: review+
I'll add a comment on the end with the bug #.  jst, sr?
Comment on attachment 98435 [details] [diff] [review]
Patch

sr=jst

But please, diff -u next time!
Attachment #98435 - Flags: superreview+
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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

[...]
}
Blocks: 170413
Filed bug 170413 about the regression.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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);
  }
*** Bug 170413 has been marked as a duplicate of this bug. ***
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.
Status: REOPENED → ASSIGNED
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. 
Have backed out the patch.
Any chance of getting this backed out of 1.2 as well?
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

a=dbaron for backing out of 1.2
Backed out of 1.2 branch
It looks like the 1.2-branch backout was done incorrectly.  The 9 was not
changed to an 8.
I believe this incorrect backout caused bug 182441 and bug 182348.
I've moved tracking of the regressions to bug 182500.
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
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.
Attachment #98435 - Attachment is obsolete: true
Attachment #98435 - Flags: superreview+
Attachment #98435 - Flags: review+
Blocks: 195260
FYI: Parser regression test PASSED. Also, ran the patch against the urls listed
under Debug->Verifications menu. URLs loaded fine.
Attachment #122246 - Flags: superreview?(jst)
Attachment #122246 - Flags: review?(nisheeth)
Attached patch Patch v1.2 — — Splinter Review 
Same as patch v1.1 but with a mild clean up.
Attachment #122246 - Attachment is obsolete: true
Attachment #122246 - Flags: superreview?(jst)
Attachment #122246 - Flags: review?(nisheeth)
Attachment #122313 - Flags: superreview?(jst)
Attachment #122313 - Flags: review?(nisheeth)
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

sr=jst
Attachment #122313 - Flags: superreview?(jst) → superreview+
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

looks good.
Attachment #122313 - Flags: review?(nisheeth) → review+
This fix is essential to unblock 195260 ( which is a serious bug ) and therefore
nominating for 1.4b.

Taking the bug.
Assignee: rjesup → harishd
Status: ASSIGNED → NEW
Flags: blocking1.4b?
Priority: -- → P1
Target Milestone: --- → mozilla1.4beta
Attachment #122313 - Flags: approval1.4b?
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

a=sspitzer
Attachment #122313 - Flags: approval1.4b? → approval1.4b+
Fix landed.
Status: NEW → RESOLVED
Closed: 22 years ago → 21 years ago
Resolution: --- → FIXED
Flags: blocking1.4b?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: