Bug 64230

Summary: REGRESSION (r73385): Marquee with behavior="alternate" is not working
Product: WebKit Reporter: Divakar <divakar.webkit>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aestes, ap, dglazkov, hyatt, parag, simon.fraser, webkit.review.bot, withlhw
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://zlastrona.org/
Attachments:
Description Flags
Test case
none
Patch for Marquee-Behavior="alternate"
none
Updated patch
none
updated patch
none
Updated patch
none
Updated Patch
none
Updated Patch
none
Updated Patch none

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
DetailsFormatted DiffDiff
Updated patch (2.71 KB, patch)
2011-11-24 05:45 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
updated patch (4.71 KB, patch)
2011-12-12 07:17 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Updated patch (5.96 KB, patch)
2011-12-22 08:26 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Updated Patch (13.02 KB, patch)
2011-12-28 03:39 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Updated Patch (13.01 KB, patch)
2012-01-17 06:29 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
Updated Patch (6.54 KB, patch)
2012-01-23 05:10 PST, Parag Radke 		no flags
DetailsFormatted DiffDiff
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.