eHTMLTag_userdefined should be included as a valid head kid

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
HTML: Parser
P1
normal
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Thu-Van Luu, Assigned: harishd)

Tracking

Trunk
mozilla1.4beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.29 KB, patch
Nisheeth Ranjan
: review+
(not reading, please use seth@sspitzer.org instead)
: approval1.4b+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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

Updated

15 years ago
Summary: eHTMLTag_userdefined should be included in the as a valid head kid → eHTMLTag_userdefined should be included as a valid head kid
Created attachment 98435 [details] [diff] [review]
Patch

Patch to add userdefined to the gHeadKids list
Ready for review
Status: NEW → ASSIGNED
Keywords: patch, review
(Assignee)

Comment 4

15 years ago
> 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.
(Assignee)

Comment 5

15 years ago
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
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 9

15 years ago
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

[...]
}

Updated

15 years ago
Blocks: 170413

Comment 10

15 years ago
Filed bug 170413 about the regression.
(Assignee)

Comment 11

15 years ago
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 → ---

Comment 12

15 years ago
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

Comment 15

15 years ago
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. 
(Assignee)

Comment 16

15 years ago
Have backed out the patch.

Comment 17

15 years ago
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.
(Assignee)

Comment 24

14 years ago
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.
Attachment #98435 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #98435 - Flags: superreview+
Attachment #98435 - Flags: review+

Updated

14 years ago
Blocks: 195260
(Assignee)

Comment 25

14 years ago
FYI: Parser regression test PASSED. Also, ran the patch against the urls listed
under Debug->Verifications menu. URLs loaded fine.
(Assignee)

Updated

14 years ago
Attachment #122246 - Flags: superreview?(jst)
Attachment #122246 - Flags: review?(nisheeth)
(Assignee)

Comment 26

14 years ago
Created attachment 122313 [details] [diff] [review]
Patch v1.2

Same as patch v1.1 but with a mild clean up.
Attachment #122246 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #122246 - Flags: superreview?(jst)
Attachment #122246 - Flags: review?(nisheeth)
(Assignee)

Updated

14 years ago
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 28

14 years ago
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

looks good.
Attachment #122313 - Flags: review?(nisheeth) → review+
(Assignee)

Comment 29

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #122313 - Flags: approval1.4b?
Comment on attachment 122313 [details] [diff] [review]
Patch v1.2

a=sspitzer
Attachment #122313 - Flags: approval1.4b? → approval1.4b+
(Assignee)

Comment 31

14 years ago
Fix landed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Flags: blocking1.4b?
You need to log in before you can comment on or make changes to this bug.