Closed
Bug 641198
Opened 14 years ago
Closed 14 years ago
Rollover animated gif show only one frame.
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mdirenzo, Assigned: azakai)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
701 bytes,
text/html
|
Details | |
4.64 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0 Starting from the first FF4b I installed this problem is present. This animation works fine on ALL other browser (including FF3) on all platform (WIN, MAC and LINUX). When you use 2 animated gif, with rollover, they animate the first time and then you only see ONE frame for all other over and out activity. Reproducible: Always Attached is a simple testcase (no javascript involved) that works in Fx3.6 and other but not in Fx4 Rollover gif should animate not only show first frame.
The first beta I installed was FF4b11. To try a rollback I downloaded some older versions. I installed 4b3 pre and all animations work fine except for the one on the right top, where there are 2 animations, one in the background and one in the foreground. Then I installed 4b6pre and animations work ONLY for the hover activity, when I move the mouse out I only see the first frame of the animated gif. Then I was not able to find other releases to install... I confirm again that "my site" works perfectly with all other browser I tested (FF3 up to the last 3.6.15 - Opera, IE 7 - IE 8 - Safari - Chrome) on different OS (WINXP - WIN 7 - VISTA - OSX - LINUX) where the browsers work.
Comment 3•14 years ago
|
||
Regression window: Works: http://hg.mozilla.org/mozilla-central/rev/75d2145d1bd3 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre ID:20100907194131 Fails: http://hg.mozilla.org/mozilla-central/rev/4bb022d84a31 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre ID:20100907182227 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75d2145d1bd3&tochange=4bb022d84a31 Suspected bug: Bug 359608 - Animated GIFs are animated even when user navigates to another page
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Comment 4•14 years ago
|
||
In local build(from m-c repository), build from fd33aa85de93 : fails build from d35c0d22fa71 : works build from 459a07b6bdee : works build from 75d2145d1bd3 : works Triggered by: fd33aa85de93 Alon Zakai — Bug 359608 - Animated GIFs are animated even when user navigates to another page.r=bholley,bz;sr=bz;a=blocker
Blocks: 359608
Comment 5•14 years ago
|
||
Alon, can you take a look at this please?
Component: General → ImageLib
QA Contact: general → imagelib
Comment 6•14 years ago
|
||
I'm guessing the issue is that the nsImageLoader::OnStartContainer call to StartAnimation would restart the animation from the beginning if it had already finished. The new setup doesn't seem to have a way of restarting finite animations: it assumes all animations are infinite....
Assignee | ||
Comment 8•14 years ago
|
||
On Chrome 10.0.648.114 beta, I don't see animations with the test page. Only a single frame changes when hovering. FF4 also shows a single frame change on hover, but the opposite of the frames Chrome shows. So I am confused. Are both browsers wrong here?
The site http://www.riflessivo.eu/intro/home_en.html (which involve preloading JS) works perfectly with chrome (last official release). The test file only show the last frame of animations that is, at least, a better behaviour compared to FF4. In this case we loose the animation but we can see the open and close actions correctly. Please see also the site that is more complete than the test html.
Component: ImageLib → General
Updated•14 years ago
|
Assignee: azakai → nobody
Component: General → ImageLib
Comment 11•14 years ago
|
||
Alon, Chrome shows the _last_ frame of the animation once the animation is one. It looks like Firefox 4 shows the _first_ frame of the animation. Arguably both browsers are "wrong" (in that Fx 3.6 and Opera show the whole animation), but Firefox 4's behavior seems _very_ wrong to me.
Assignee | ||
Comment 12•14 years ago
|
||
I believe it makes sense to reset mAnimationFinished in ResetAnimation(). After all, if we are resetting the animation, presumably we want it to start again even if it already finished. This fixes the no-JS testcase here and the website itself. I am pushing to try.
Assignee | ||
Comment 13•14 years ago
|
||
Comment on attachment 520359 [details] [diff] [review] patch Looks good on try, asking for review.
Attachment #520359 -
Flags: review?(joe)
Reporter | ||
Comment 14•14 years ago
|
||
Hi, I would like to test it but I do not know how.. I'm sorry... I'm not a developer. Anyway, let me thanks Alon for solving this issue... I hope I'll see it in next RC.
Comment 15•14 years ago
|
||
Mario, there is no next rc planned; final release of Firefox 4 will be this Tuesday pending critical stop-ship bugs (which this bug is not). Assuming the patch is correct it will show up in Firefox 5 in a few months.
Assignee | ||
Comment 16•14 years ago
|
||
Comment on attachment 520359 [details] [diff] [review] patch Found a bug while writing a test.
Attachment #520359 -
Flags: review?(joe)
Assignee | ||
Comment 17•14 years ago
|
||
Better patch, with test.
Attachment #520359 -
Attachment is obsolete: true
Attachment #521036 -
Flags: review?(joe)
Comment 18•14 years ago
|
||
Comment on attachment 521036 [details] [diff] [review] patch v2 >diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp >- if (ShouldAnimate()) >+ if (ShouldAnimate()) { > StartAnimation(); >+ // The animation may not have been running before, if mAnimationFinished >+ // was false (before we changed it to true in this function). So, mark the >+ // animation as running. >+ mAnimating = PR_TRUE; >+ } I almost feel like we should somehow use EvaluateAnimation or something like it here. Maybe we can unconditionally stop animation and then evaluate? It would help a lot if StopAnimation/StartAnimating set mAnimating.... >diff --git a/modules/libpr0n/test/reftest/gif/test_bug641198.html b/modules/libpr0n/test/reftest/gif/test_bug641198.html >+ document.getElementById("animated").setAttribute("class", "animated" + ((counter % 2)+1)); >+ setTimeout(doTimeout, counter == 3 ? 500 : 250); // Wait a bit more for the last one, to prevent oranges >+ } >+ counter++; This test is just *definitely* going to cause intermittent oranges, I know it :( Ehsan and I tried coming up with a MozAfterPaint method of testing animated images, but it can't be relied upon *either*. I think our only option for reliable animated image tests is to use imgILoader::LoadImage directly like the libpr0n unit tests do.
Attachment #521036 -
Flags: review?(joe) → review+
Assignee | ||
Comment 19•14 years ago
|
||
>
> >diff --git a/modules/libpr0n/test/reftest/gif/test_bug641198.html b/modules/libpr0n/test/reftest/gif/test_bug641198.html
>
> >+ document.getElementById("animated").setAttribute("class", "animated" + ((counter % 2)+1));
> >+ setTimeout(doTimeout, counter == 3 ? 500 : 250); // Wait a bit more for the last one, to prevent oranges
> >+ }
> >+ counter++;
>
> This test is just *definitely* going to cause intermittent oranges, I know it
> :(
>
> Ehsan and I tried coming up with a MozAfterPaint method of testing animated
> images, but it can't be relied upon *either*. I think our only option for
> reliable animated image tests is to use imgILoader::LoadImage directly like the
> libpr0n unit tests do.
I pushed this several times to try (I had other problems to work out), and I never saw an orange due to this. So while it is a risk for oranges, I at least think such oranges will be very rare if they do occur.
Keywords: checkin-needed
Comment 20•14 years ago
|
||
> This test is just *definitely* going to cause intermittent oranges
The test also only tests that the image ends up showing the last frame; not that it animates, right?
And it takes a full second to run?
Assignee | ||
Comment 21•14 years ago
|
||
Yes, it's a reftest, so only the last frame matters. It does test that it animated properly, though, at least in the sense that the last frame is shown and not the first (which is enough to catch the problem in this bug). It takes a second, yes. I can make it shorter, but am worried about higher risk of oranges. Do you think it is safe to do that?
Comment 22•14 years ago
|
||
I wonder whether it's better to just do a chrome test and directly listen for the notifications the animation sends....
Assignee | ||
Comment 23•14 years ago
|
||
We do have some browser chrome tests for animations, yeah, but they have limitations. I believe the safest thing regarding ensuring correct results is an actual reftest. But yes, this approach has definite downsides too...
Comment 24•14 years ago
|
||
What makes an actual reftest better? My suggestion was to use a chrome (not browser chrome) test which uses the privileges it has to install an animation listener and then uses those notifications to decide when to end the test. Then at test end you can screenshot and compare just like reftest does to make sure that no only did the animation happen but you ended up with the right bits.
Assignee | ||
Comment 25•14 years ago
|
||
Sounds interesting, is there an example of a similar test I can take a look at? 2nd question, should I not land this meanwhile? Or file a followup bug to replace the test with a better one?
Comment 26•14 years ago
|
||
Landing this in the meantime sounds fine. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug451286.xul might be ok to crib. Or the various things in docshell/test/chrome that include WindowSnapshot.js.
Assignee | ||
Comment 27•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/be4ecb32b875 I will file a followup bug for an improved test.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•