REGRESSION: scrolldelay=0 causes marquee not to display

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anantha Keesara]

I .Steps:
-----------
1. Go to: http://o2jam.9you.com/

II. Issue:
-----------------
Notice that the marquee on left side of page is not displayed in Safari.
(also on http://o2jam.9you.com/index.html)
(also on http://mj.9you.com/)

III. Other browsers
--------------------------
FF,IE, Opera: ok
Safari:  not ok

[Anantha Keesara]

Created attachment 16358 [details]
scrolldelay=0 causes marquee not to display

[Gavin Sherlock]

It is displayed in release Safari on the Mac, but not in the nightly (r25699), so this is a regression.  Note, in release Safari, the marquee text does overlap the left border, but at least the marquee text is there.

[Alexander Luck]

also in Windows XP (r25699)

[Eric Seidel (no email)]

Regressions are all P1, according to the priority guidelines.

[Dave Hyatt]

This is not a regression.  It does not display in shipping Safari.  Unless you are saying it is a regression from Safari 2...

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> This is not a regression.  It does not display in shipping Safari.  Unless you
> are saying it is a regression from Safari 2...

Safari 2.0.4 (419.3) with original WebKit on Mac OS X 10.4.11 (8S165) does not scroll the marquee, either, but it DOES show the entire text of the marquee.

Safari 3.0.4 (523.12.2) with a local debug build of WebKit r29336 on 10.4.11 does not scroll the marquee, but shows the first half of the first character in the marquee on the right side of the element.

So it's a regression in the sense that the text of the marquee can't be read.

[Gavin Sherlock]

Maybe it should be split into two bugs then, one of which is a regression, and one of which isn't.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Maybe it should be split into two bugs then, one of which is a regression, and
> one of which isn't.

It would probably be best to simply fix the bug to make the text scroll.  :)

The Safari 2.0/3.0 behavior difference was just to determine if the bug was a regression or not.

[Eric Seidel (no email)]

My guess is:

    if (speed() != marqueeSpeed()) {
        m_speed = marqueeSpeed();
        if (m_timer.isActive())
            m_timer.startRepeating(speed() * 0.001);
    }

in Marquee::updateMarqueeStyle() is the buggy code.  m_speed is initialized to 0 to begin with, so I believe the marquee is never started due to that check.

Two things we'd need to check before making a fix are:  1.  what does a negative marqueSpeed() mean? is that even possible?  and 2.  we may need a m_haveStartedMarqueeFirstTime bool to prevent restarting the timer when marqueeSpeed() == whatever m_speed is initialized too, and m_timer being stopped (due to an explicit stop)

[Aaron Golden]

Created attachment 19812 [details]
Patch to make marquee elements handle empty strings in the truespeed attribute correctly

The patch appears to fix all the test cases currently attached to the bug---with the patch marquees scroll correctly on all of the test pages and without the patch none of them do.

[Darin Adler]

Comment on attachment 19812 [details]
Patch to make marquee elements handle empty strings in the truespeed attribute correctly

Thanks for contributing this fix. Looks great!

This patch has tab characters in it. We can't land it like that, so please make a patch that does not have it.

+        * ChangeLog:

The ChangeLog itself should not be listed in the change log.

+        * WebCore.xcodeproj/project.pbxproj:

The project file should not be listed in the change log.

The patch needs to include a test case in the LayoutTests directory. We require a test case for each bug we fix. It should be relatively straightforward to convert the tests attached to this bug into a layout test.

review- because of the lack of tests

[Aaron Golden]

Created attachment 19877 [details]
Patch to make marquee elements handle empty strings in the truespeed attribute correctly, includes a new layout test, no tabs in the patch this time

Posted new patch.  This one doesn't have any tab characters and includes a new layout test.

[David Kilzer (:ddkilzer)]

Comment on attachment 19877 [details]
Patch to make marquee elements handle empty strings in the truespeed attribute correctly, includes a new layout test, no tabs in the patch this time

Looks great!  r=me

Note that this patch will need pixel test results generated as well.

Thanks Aaron!

[David Kilzer (:ddkilzer)]

Committed revision 31178.