390210 - <marquee> fails to close if it contains unclosed <center> tag, scrolls whole page

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Cédric "chewey" Menge]

Attachment: testcase for marquee gone mad.

In current trunk builds, </marquee> fails to close a scrolling section if it contains an unclosed <center>, see attached testcase: The whole page is "marqueed".

This does *not* happen in branch builds.

To see this in the wild, look at the URL if you feel strong enough.

Comment 1

[Martijn Wargers (dead)]

Regression from bug 369326, it regressed between 2007-02-15 and 2007-02-17.

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

Not sure whether I should use (kBlock|kSpecial) or kFlowEntity.
I'm not sure in what circumstances I would see the difference.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 274763 [details] [diff] [review]
patch

Martijn, can you add mochitests for <marquee>s as children of random elements? It's hard to know the full extent of one of these changes just via code inspection.

Sorry for the delay in review :-/

Comment 5

[j.j.]

No progress here?

Comment 6

[Martijn Wargers (dead)]

Sorry, I still need to make the mochitests that Blake asked in comment 4.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase with marquees as child of various elements

Ok, sorry for the delay. This is the test that Blake asked for in comment 4.
It can be relatively easily converted into a mochitest.
With the patch applied, nothing changes in the results. This is expected, because it tests something different as the first testcase. I think Blake wanted to be sure that it wouldn't regress some unrelated things.

There are some failure alerts in IE7, which indicates the difference in behavior with Mozilla. It might be interesting to take a look at, but those are only for very special cases, I think.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch with mochitest

Ok, now with mochitest.
Without the patch the test fails on one test, with patch it succeeds on all tests.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 358851 [details] [diff] [review]
patch with mochitest

Let's try this on the trunk. I had a momentary flash of panic when thinking about differences in stuff like <marquee><div>, but I doubt that any *real* content has those (famous last words).

Comment 10

[Martijn Wargers (dead)]

Attachment: patch with 2 mochitests

(In reply to comment #9)
> (From update of attachment 358851 [details] [diff] [review])
> Let's try this on the trunk. I had a momentary flash of panic when thinking
> about differences in stuff like <marquee><div>, but I doubt that any *real*
> content has those (famous last words).

Yes, well I was wondering about why you weren't asking about that before, but I have done that with the added mochitest in this patch.
I also switched some of the arguments in the first mochitest in the is() function. It doesn't matter for the test itself, but it was used wrongly.