64230 – REGRESSION (r73385): Marquee with behavior="alternate" is not working

RESOLVED FIXED 64230

REGRESSION (r73385): Marquee with behavior="alternate" is not working

https://bugs.webkit.org/show_bug.cgi?id=64230

Summary REGRESSION (r73385): Marquee with behavior="alternate" is not working

Divakar

Reported 2011-07-09 05:32:40 PDT

Marquee with behavior="alternate" is not working after changeset - 73385. This bug is introduced after layout overflow re-architecture. It is observed in live site: http://zlastrona.org/ testcase: <html> <body> <marquee behavior="alternate">This is a simple marquee with behavior as alternate.</marquee> </body> </html>

Attachments
Test case (125 bytes, text/html) 2011-09-23 15:49 PDT, Andy Estes	no flags	Details
Patch for Marquee-Behavior="alternate" (4.72 KB, patch) 2011-11-15 01:51 PST, Parag Radke	no flags	Details Formatted Diff Diff
Updated patch (2.71 KB, patch) 2011-11-24 05:45 PST, Parag Radke	no flags	Details Formatted Diff Diff
updated patch (4.71 KB, patch) 2011-12-12 07:17 PST, Parag Radke	no flags	Details Formatted Diff Diff
Updated patch (5.96 KB, patch) 2011-12-22 08:26 PST, Parag Radke	no flags	Details Formatted Diff Diff
Updated Patch (13.02 KB, patch) 2011-12-28 03:39 PST, Parag Radke	no flags	Details Formatted Diff Diff
Updated Patch (13.01 KB, patch) 2012-01-17 06:29 PST, Parag Radke	no flags	Details Formatted Diff Diff
Updated Patch (6.54 KB, patch) 2012-01-23 05:10 PST, Parag Radke	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2011-07-11 15:59:31 PDT

Confirming that such marquee renders as static text now. I didn't verify that this revision caused the problem.

Alexey Proskuryakov

Comment 2 2011-07-11 15:59:54 PDT

<rdar://problem/9756806>

Hyungwook Lee

Comment 3 2011-09-01 03:01:03 PDT

It seems caused by changing RenderBox's API from rightmostPosition() to maxXLayoutOverflow() in RenderMarquee::computePosition(). We can't get correct contentWidth by maxXLayoutOverflow().

Divakar

Comment 4 2011-09-13 03:54:31 PDT

It works fine, if use maxPreferredLogicalWidth() instead of maxXLayoutOverflow().

Andy Estes

Comment 5 2011-09-23 15:48:12 PDT

<http://trac.webkit.org/changeset/73385>

Andy Estes

Comment 6 2011-09-23 15:49:04 PDT

Created attachment 108550 [details] Test case

Parag Radke

Comment 7 2011-11-15 01:51:36 PST

Created attachment 115125 [details] Patch for Marquee-Behavior="alternate"

Simon Fraser (smfr)

Comment 8 2011-11-15 11:12:03 PST

Comment on attachment 115125 [details] Patch for Marquee-Behavior="alternate" View in context: https://bugs.webkit.org/attachment.cgi?id=115125&action=review Patch looks OK, but I'm not convinced that the test is good. > LayoutTests/fast/html/marquee-alternate.html:5 > + It verifies marquee alternate behavior.For this test to pass, you should see a the text should scroll to the left side extreem and than again scroll back to the right. and it should keep on doing it.</p> "extreem". Is this test actually testing the bug fix? Wouldn't you have to wait for a cycle of animation to really test things?

Parag Radke

Comment 9 2011-11-21 04:35:37 PST

1."extreem". >Change is done. 2.Is this test actually testing the bug fix? Wouldn't you have to wait for a cycle of animation to really test things? >I could not find a way to know when the animation cycle ended for marquee, and how to get marquee text positions to compare with the expected ideal positions according to alternate behavior.Also I tried testing using the location of marquee but I always get the location of the RenderBlock which contains the Marquee than the actual animating text/image. Please suggest a way to test this.

Parag Radke

Comment 10 2011-11-21 05:12:04 PST

The Actual comment is. >"extreem". Change is done. >Is this test actually testing the bug fix? Wouldn't you have to wait for a cycle of animation to really test things? I could not find a way to know when the animation cycle ended for marquee, and how to get marquee text positions to compare with the expected ideal positions according to alternate behavior.Also I tried testing using the location of marquee but I always get the location of the RenderBlock which contains the Marquee than the actual animating text/image. Please suggest a way to test this.

Parag Radke

Comment 11 2011-11-24 05:45:48 PST

Created attachment 116500 [details] Updated patch I added a manual testcase to check the marquee alternate behavior, as i couldn't find any way to test it programmatically

Simon Fraser (smfr)

Comment 12 2011-11-28 11:25:09 PST

Does reading the scroll offset reveal the marquee position?

Parag Radke

Comment 13 2011-12-12 07:17:24 PST

Created attachment 118783 [details] updated patch Replacing manual test case with a layout test.

WebKit Review Bot

Comment 14 2011-12-12 08:05:59 PST

Comment on attachment 118783 [details] updated patch Attachment 118783 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10842128 New failing tests: fast/html/marquee-alternate.html

Andy Estes

Comment 15 2011-12-15 21:37:26 PST

Comment on attachment 118783 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118783&action=review I applied your patch on my machine and marquee-alternate.html fails when I run it. What guarantees that a cycle will complete in 4.6 seconds? Are you assuming the marquee is a certain width based on the text within it? That isn't a safe assumption. > LayoutTests/fast/html/marquee-alternate.html:10 > +var fullCycle = 4600; It's unfortunate that this test requires 4.6 seconds to run. You can shorten this time by using marquee's scrollAmount and scrollDelay properties.

Parag Radke

Comment 16 2011-12-22 08:26:09 PST

Created attachment 120324 [details] Updated patch 1. Added a check in computePreferredLogicalWidths() for marquee behavior as this is a exception when we should actually calculate min/max preferred logical width even when style logical width is fixed (for the marquee content lenght). 2. I fixed the marquee width in the test case so that it would be platform independent and also reduced the test execution time to 500 ms using scrollAmount and scrollDelay

WebKit Review Bot

Comment 17 2011-12-22 10:57:39 PST

Comment on attachment 120324 [details] Updated patch Attachment 120324 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11000645 New failing tests: fast/workers/storage/use-same-database-in-page-and-workers.html fast/html/marquee-alternate.html

Parag Radke

Comment 18 2011-12-28 03:39:46 PST

Created attachment 120649 [details] Updated Patch Updated testcase now using fixed size image in place of text as different ports may use different fonts/font-size as default

Parag Radke

Comment 19 2012-01-17 06:29:24 PST

Created attachment 122757 [details] Updated Patch Updated testcase to reduce test case execution time.

Andy Estes

Comment 20 2012-01-17 16:04:15 PST

Comment on attachment 122757 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122757&action=review I don't think you need to add a new image for your test. There are numerous images already in LayoutTests that could be reused for this purpose. I also don't think it's okay to add an image of Apple's trademarked logo without permission from the copyright holder. r- because of this. I have some additional comments and questions below. > Source/WebCore/rendering/RenderBlock.cpp:4936 > + if (!isTableCell() && style()->logicalWidth().isFixed() && style()->logicalWidth().value() > 0 && style()->marqueeBehavior() != MALTERNATE) Your ChangeLog says that alternating marquees are a special case when calculating logical width, but you don't explain why, and I don't understand myself why this is the case. A previous version of your patch didn't have this check, so I'd like to see more of a "why" explanation. > LayoutTests/ChangeLog:9 > + and smaller testcase execution time. You don't need to say "...and smaller testcase execution time." The slower test was never checked in, so nobody will know that some previous iteration of this patch included a slower test case. > LayoutTests/fast/html/marquee-alternate-expected.txt:4 > +PASS on Initial Position > +PASS on after half Cycle Completion > +PASS on after full Cycle Completion The phrases "Initial Position" and "Cycle Completion" do not need to be capitalized. > LayoutTests/fast/html/marquee-alternate.html:10 > +var halfCycle = 75; > +var fullCycle = 150; The terms "halfCycle" and "fullCycle" are inaccurate. A full cycle of the alternating marquee takes 2ms, not 150ms. 150ms would be 75 cycles. Is it possible for this entire test to run in 2ms instead?

Parag Radke

Comment 21 2012-01-23 05:10:15 PST

Created attachment 123543 [details] Updated Patch Updated patch with review comments implemented.

Parag Radke

Comment 22 2012-01-23 05:29:20 PST

> I don't think you need to add a new image for your test. There are numerous images already in LayoutTests that could be reused for this purpose. I also don't think it's okay to add an image of Apple's trademarked logo without permission from the copyright holder. using a local image for the test case. > > Source/WebCore/rendering/RenderBlock.cpp:4936 > > + if (!isTableCell() && style()->logicalWidth().isFixed() && style()->logicalWidth().value() > 0 && style()->marqueeBehavior() != MALTERNATE) > > Your ChangeLog says that alternating marquees are a special case when calculating logical width, but you don't explain why, and I don't understand myself why this is the case. A previous version of your patch didn't have this check, so I'd like to see more of a "why" explanation. We need(style()->marqueeBehavior() != MALTERNATE) check as we always need the marquee's actual content width to compute the initial/end position in case of 'ALTERNATE'. So we need to calculate the logical width in Alternate case even if fixed width is specified as content has to animate between renderBox().right().x() - contentWidth() and renderBox().left().x() + contentWidth(). left right !<-----------[Render box width(may be fixed)]--------------->! ! <-content width->! | =================|at t=0 |================= |at t=half period | =================|at t=full period > > LayoutTests/ChangeLog:9 > > + and smaller testcase execution time. > You don't need to say "...and smaller testcase execution time." The slower test was never checked in, so nobody will know that some previous iteration of this patch included a slower test case. OK, done. > > LayoutTests/fast/html/marquee-alternate-expected.txt:4 > > +PASS on Initial Position > > +PASS on after half Cycle Completion > > +PASS on after full Cycle Completion > > The phrases "Initial Position" and "Cycle Completion" do not need to be capitalized. OK, done. > > LayoutTests/fast/html/marquee-alternate.html:10 > > +var halfCycle = 75; > > +var fullCycle = 150; > > The terms "halfCycle" and "fullCycle" are inaccurate. A full cycle of the alternating marquee takes 2ms, not 150ms. 150ms would be 75 cycles. Is it possible for this entire test to run in 2ms instead? No, it's not possible to complete this entire test in 2ms, i experimented with various combinations but for this latest test case the first move in position of marquee is observed only at 63ms and 2nd at 123ms

WebKit Review Bot

Comment 23 2012-01-24 12:24:27 PST

Comment on attachment 123543 [details] Updated Patch Clearing flags on attachment: 123543 Committed r105772: <http://trac.webkit.org/changeset/105772>

WebKit Review Bot

Comment 24 2012-01-24 12:24:32 PST

All reviewed patches have been landed. Closing bug.

Adam Barth

Comment 25 2012-01-24 15:38:07 PST

This patch added marquee-alternate.html which appears to be flaky.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component WebCore Misc.

Assignee

Nobody

Reported

2011-07-09 05:32 PDT

Modified

2012-01-24 15:38 PST History

CC List

9 users Show

URL

http://zlastrona.org/

Keywords InRadar, Regression

Depends on

Blocks