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>
Confirming that such marquee renders as static text now. I didn't verify that this revision caused the problem.
<rdar://problem/9756806>
It seems caused by changing RenderBox's API from rightmostPosition() to maxXLayoutOverflow() in RenderMarquee::computePosition(). We can't get correct contentWidth by maxXLayoutOverflow().
It works fine, if use maxPreferredLogicalWidth() instead of maxXLayoutOverflow().
<http://trac.webkit.org/changeset/73385>
Created attachment 108550 [details] Test case
Created attachment 115125 [details] Patch for Marquee-Behavior="alternate"
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?
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.
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.
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
Does reading the scroll offset reveal the marquee position?
Created attachment 118783 [details] updated patch Replacing manual test case with a layout test.
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
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.
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
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
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
Created attachment 122757 [details] Updated Patch Updated testcase to reduce test case execution time.
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?
Created attachment 123543 [details] Updated Patch Updated patch with review comments implemented.
> 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
Comment on attachment 123543 [details] Updated Patch Clearing flags on attachment: 123543 Committed r105772: <http://trac.webkit.org/changeset/105772>
All reviewed patches have been landed. Closing bug.
This patch added marquee-alternate.html which appears to be flaky.