WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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.
Top of Page
Format For Printing
XML
Clone This Bug