256731 - Bad tag inside phrase-element tags (EM, STRONG, VAR etc) fail them to close

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[chAlx]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040806
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040806

Bad tag: some unspecified opening-tag-like string: <tag>
Phrase elements: EM, STRONG, DFN, CODE, SAMP, KBD, VAR, CITE, ABBR, and ACRONYM
Issued code (fragment, but works standalone too):
  Normal text ... <em>Phrase element <bad_tag> </em> ... Normal text again
Problem: closing EM doesn't work while preferred with bad (unsupported) tag: all text to end is EMed.

Reproducible: Always
Steps to Reproduce:
Open any page with detailed bad content

Actual Results:  
Plain text ... Italic text ... Italic text

Expected Results:  
Plain text ... Italic text ... Plain text

Not issued code:
 - whith normal (specified) tag instead of bad one;
 - with any (same or not) closing bad tag after or instead of bad one;
 - with normal (not phrase-element) markup tags like I or B.

Comment 1

[chAlx]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm... Shouldn't the residual style code handle this?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

It probably should. Confirming (I see this on a week-old cvs build).

Comment 4

[Boris Zbarsky [:bzbarsky]]

So why does <span> work while <bad_tag> does not?  For that matter, are there
valid HTML tags that also don't work?

Chances are this is an nsElementTable tweak if we can figure out what to tweak.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Not understanding most of the code here makes this a bit difficult to debug :-).
Sorry if the following is basic knowledge and just repetition. Skip the next
paragraph for my conclusion.

I traced what's happening and I found that the root of the problem is that EM
tags cannot close kFlowEntity tags. Thus nsHTMLTags::GetCloseTargetForEndTag
stops when it sees an eHTMLTag_userdefined (which currently is a kFlowEntity --
going by parents, which seems to be how this system classifies tags). Then it
returns eHTMLTag_unknown, and CNavDTD::HandleEndToken peters out. I checked this
by making eHTMLTag_userdefined a 'kSpecial' tag, and the page then was parsed
properly (this probably is *not* the fix).

Anything that has a 'parent' of kFlowEntity should trigger this. See:
http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsElementTable.cpp#2090
and note <em> is phrase-level.

Perhaps we need to special-case the userdefined tags or put them in another group?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 265137 has been marked as a duplicate of this bug. ***

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the problem is that opening the userdefined tag doesn't close <em>, but
closing <em> doesn't close the userdefined tag either, huh?

I think having </phrase> close out userdefined tags should be ok, but it's a
scary change... jst?  peterv?  rbs?  heikki?  Do you know anything about this stuff?

Comment 8

[rbs]

> I think having </phrase> close out userdefined tags should be ok

me too.

The only couple of bugs that I have seen with userdefined tags were bug 167493
(and related). In real life (TM), such tags happen in copy-paste (from M$ cruft
bug 195260) or XBL -- all of which are well-formed. So the fix will be fine.
There aren't significant pages that have come to depend on the existing buggy
behavior. 

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This allows phrasals to close userdefined tags (special casing them). I'm not
entirely sure about style issues in this file (or where to put the test in the
loop) but this made the most sense to me.

This patch passes the parser regression tests, for what they're worth.

Comment 10

[rbs]

Comment on attachment 163261 [details] [diff] [review]
patch v1

r=rbs

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163261 [details] [diff] [review]
patch v1

sr=bzbarsky.  This is probably the safe way to do this, yes.

Could you add this testcase to the parser regression tests, please?  And for
that matter, just generally add all the testcases you can find to them?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: regression testcase

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 163262 [details]
regression testcase

rbs, could you give me r+sr to check this in to the parser regression tests?

Comment 14

[rbs]

Comment on attachment 163262 [details]
regression testcase

You don't need r/sr for regression tests. Just check them in, in good faith.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Both the regression test and the patch have been checked in.