272293 - Phrase-level tags (such as <em>) do not close when there is an open "Inline" tag (such as <nobr>)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[rmsimons50]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

<h3><font face="arial" color="#009966"><em>E-mail</em><nbr><img src="eemail.gif"
hspace="20" width="45" height="52"></font></h3>

THE ABOVE HTML WORKS CORRECTLY, BUT THE FOLLOWING HTML IGNORES THE </EM>

<h3><font face="arial" color="#009966"><em>E-mail<nbr><img src="eemail.gif"
hspace="20" width="45" height="52"></em></font></h3>

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Actual Results:  
Following text is italicized.

Expected Results:  
Following text should not be italicized.

It works in IE.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

I can see the bug with this testcase in Mozilla1.7, but I can't see the bug
anymore using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20041128
Firefox/0.9.1+
So I think this has already been fixed somehow.

Comment 2

[rmsimons50]

Attachment: Execute in Firefox Version 1.0 to see the errors.

This attachment illustrates the problem more simply than the original example
involving <img...> which should be ignored.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: minimized testcase

A really minimized testcase (the other testcases were good, but this is much
easier to debug).

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

The problem here is that we don't let phrasals close kInlineEntity's. I let them
do so in nsElementTable.cpp and that change passes the parser regression tests
with flying colors. The only actual kInlineEntity's are: <dd>, <dt>, <nobr>,
<xmp>, which are all tested in the tests so I'm pretty confident with this
change. Are there any other problems I should look out for?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.. how does this interact with residual style handling on <dd>/<dt> ?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Neither <dd> or <dt> are residual style tags, so we should just close them and
be done with them... this is as it should be, I think (reopening <dd> or <dt>
doesn't make sense to me).

Comment 7

[Boris Zbarsky [:bzbarsky]]

No, I mean the following case:

<em>
<dl>
  <dt>Text </em> More text </dt>
</dl>

Would we end up closing out the <dt> when we hit </em>?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Good point. How about changing <nobr> back to kExtensions and letting phrasals
(and specials) close kExtensions. To avoid "breaking" <multicol> support (note:
<multicol> is currently the only kExtension that is a container, <wbr> and
<spacer> are both kNonContainers), I've made <multicol> a kBlock (which is what
it really is anyway) to avoid <em><multicol></em>foo</multicol> so we'll do RS
handling. Note that we treat kExtension as an inline entity.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That sounds reasonable at first blush, but I really don't remember this code
well... test carefully!  ;)

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This is basically what I described above. The only real change is that I had to
make <multicol> contain kFlowEntity (== kInlineEntity | kBlockEntity) since RS
handling didn't quite work correctly without that. This patch also cleans up
some whitespace and comments.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

See comment 8 for an explanation of what's going on. This doesn't affect <dt>
or <dd>. I've tested both <multicol> and <nobr> with this patch and they work
as expected.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Just as a note, my patch actually *improves* our <multicol> support. The old
code didn't allow even #text as a child of <multicol>.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

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

sr=dmose

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.