Closed
Bug 167493
Opened 22 years ago
Closed 22 years ago
eHTMLTag_userdefined should be included as a valid head kid
Categories
(Core :: DOM: HTML Parser, defect, P1)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla1.4beta
People
(Reporter: tvluu, Assigned: harishd)
References
Details
Attachments
(1 file, 2 obsolete files)
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.
Comment 1•22 years ago
|
||
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•22 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
Comment 2•22 years ago
|
||
Patch to add userdefined to the gHeadKids list
Comment 3•22 years ago
|
||
Ready for 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+
Comment 6•22 years ago
|
||
I'll add a comment on the end with the bug #. jst, sr?
Comment 7•22 years ago
|
||
Comment on attachment 98435 [details] [diff] [review] Patch sr=jst But please, diff -u next time!
Attachment #98435 -
Flags: superreview+
Comment 8•22 years ago
|
||
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 [...] }
Comment 10•22 years ago
|
||
Filed bug 170413 about the regression.
Assignee | ||
Comment 11•22 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•22 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); }
Comment 13•22 years ago
|
||
*** Bug 170413 has been marked as a duplicate of this bug. ***
Comment 14•22 years ago
|
||
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•22 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•22 years ago
|
||
Have backed out the patch.
Comment 17•22 years ago
|
||
Any chance of getting this backed out of 1.2 as well?
Comment 18•22 years ago
|
||
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
Comment 20•22 years ago
|
||
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•22 years ago
|
||
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+
Assignee | ||
Comment 25•22 years ago
|
||
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)
Assignee | ||
Comment 26•22 years ago
|
||
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 27•22 years ago
|
||
Comment on attachment 122313 [details] [diff] [review] Patch v1.2 sr=jst
Attachment #122313 -
Flags: superreview?(jst) → superreview+
Comment 28•22 years ago
|
||
Comment on attachment 122313 [details] [diff] [review] Patch v1.2 looks good.
Attachment #122313 -
Flags: review?(nisheeth) → review+
Assignee | ||
Comment 29•22 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
Attachment #122313 -
Flags: approval1.4b?
Comment 30•22 years ago
|
||
Comment on attachment 122313 [details] [diff] [review] Patch v1.2 a=sspitzer
Attachment #122313 -
Flags: approval1.4b? → approval1.4b+
Assignee | ||
Comment 31•22 years ago
|
||
Fix landed.
Status: NEW → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
Flags: blocking1.4b?
You need to log in
before you can comment on or make changes to this bug.
Description
•