From Bugzilla Helper: User-Agent: Mozilla/4.72 [en] (X11; U; Linux 2.4.0-test10 i686) BuildID: Mozilla 0.7 thru build ID 2001022308 Trying to control the behavior of an animated GIF that's been loaded into a <browser> element. Reload appears to work, but stop doesn't. Reproducible: Always Steps to Reproduce: 1.Launch the attached XUL 2.Click on "Stop" 3.Click on "Reload" Actual Results: Stop has no effect. Reload can be seen to reload image. Expected Results: Stop should halt the animation. If you use Mozilla's browser and open the file (mozilla/chrome/modern/skin/modern/global/animthrob.gif), you can start and stop the animation using the reload and stop toolbar buttons just fine. Unsure why the attached xul doesn't work.

Do you get any feedback on the console?

Only when "Reload" is clicked. It says the following: Document chrome://global/skin/animthrob.gif loaded successfully There isn't any feedback when "Stop" is clicked.

When saving the attachment, renaming it to *.XUL and loading in on 2001031604 Win2k with Modern skin, I don't even see the animated GIF.

updating component

*** Bug 76766 has been marked as a duplicate of this bug. ***

*** Bug 78036 has been marked as a duplicate of this bug. ***

All operating systems, major (many news sites are unreadable without this feature), regression.

*** Bug 78120 has been marked as a duplicate of this bug. ***

Nominate for nsCatFood. A lot of people use stop for this purpose, and it has always worked in every past version of Netscape. Also adding Sairuh, who marked a similar bug (on the stop button not greying out) as nsCatFood and might want to know about this bug covering the regression.

Pavlov: I'd be happy to help with this if I can. I poked around a bit in the image loading code but didn't get very far, could use a pointer if you have an idea where to start.

I agree. This should be moved to 0.9.1 specially cuz akkana is willing to help! :)

The right way to do this isn't trivial.

Per PDT Triage effort: changing the targeted milestone to "mozilla0.9.2". Please feel free to renominate/address bug if time is available AFTER all currently targeted 0.9.1 bugs are resolved. .Angela...

*** Bug 84740 has been marked as a duplicate of this bug. ***

*** Bug 85133 has been marked as a duplicate of this bug. ***

*** Bug 87401 has been marked as a duplicate of this bug. ***

*** Bug 89507 has been marked as a duplicate of this bug. ***

*** Bug 90491 has been marked as a duplicate of this bug. ***

-> 0.9.4

Um... just as a heads up: This bug causes Mozilla to hit 100% CPU usage on display of a single animated image. CPU usage stays at 100% until you quit Mozilla completely, even if that image is no longer being displayed. This CPU hogging severely affects performance under Linux, and as a result I have moved back to Netscape v4.7. This is a showstopper bug - it should be fixed now rather than later.

Now that the focus stealing has been fixed this is the single most annoying bug present in mozilla at the moment. It is a basic part of a browsers functionality to stop animations, loading etc with ESC. This is a blocker in my eyes. I am proposing this bug for 0.9.3. Please consider fixing it for that milestone.

Removing nsenterprise nomination.

This should be fixed ASAP, IMHO, because it's very annoying sometimes. Marcos.

.

Bug 2586 was just fixed, which stops animated images in print preview mode. Can that code be leveraged to fix this?

This will probably not make it into 0.9.9.. Proposing for 1.0, this is ancient, has a lot of votes and is an integral part of a sound browsing experience.

Also it's a regression, 4.x parity, and affects accessibility. Nominate nsbeta1 as well.

*** Bug 128180 has been marked as a duplicate of this bug. ***

per adt, critical for nsbeta1. hence plus.

*** Bug 129901 has been marked as a duplicate of this bug. ***

giving this to xpapps. see http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsPresContext.h#304 for the call that needs to be made to turn off animations on a page. This can be used when stop is pressed.

->morse

Please Review and Comment on this. ;)

--> over to tpreston@netscape.com

I can't believe what my sores eyes have just seen... I just fetched, patched and rebuilt. After a year of cursing I can finally stop animations again! The patch is working fine over here. Jim, you have just earned my eternal gratitude :-) Where can I send you some beer and pizza? Everybody ending with @netscape.com please have a look and review, we _need_ to have this in 1.0, this has been an embarassment for far too long.

Comment on attachment 75533 [details] [diff] [review] nsDocumentViewer.cpp.diff Wow, that's all it takes? Looks safe, works fine. (Clicking on stop unfortunately doesn't do it, you have to use ESC. That's probably covered under another bug somewhere.) r=akkana. Let's get this puppy in for 1.0!

>(Clicking on stop unfortunately doesn't do it, you have to use ESC. That's probably covered under another bug somewhere.) Humm, this bug is "Cannot stop animation with webNavigation.stop", so I suppose the stop button should do it too, am I wrong? The STOP button perhaps should check to see if there's any animated GIFs in the page. If there was, it would remaing active, giving the user a chance to press it again to disable the animations. Anyway, having ESC to stop it is better then not being able to stop it at all. > Let's get this puppy in for 1.0! Agreed. Marcos.

Marcos: In NS4, the stop button goes disabled when the page is finished loading. However- if you click on it, even in disabled state, animated gifs will be stopped nevertheless! Perhaps not the most intuitive behavior, but it's convenient in everyday use.

Clicking on the greyed-out stop button worked in earlier (pre-libpr0n) versions of mozilla. But I guess that "bug" got "fixed". The NS4 I used (on Unix) didn't actually grey out the stop button if there were animations going. Not greying out is probably a better model, since users won't discover that they can click on stop to stop animations if the button looks disabled. Meanwhile, is any sr'ed cc'ed here? Jag, can you have a look at the patch, or recommend someone else?

:) Thanks for all your guys' praising! Diego, I would like your pissa very much. But I'm afraid that would already be eaten up by Lufthansa Air guys on the way to Beijing. Anyway, thanks! And pls do me a favor to check it in, I am still on my way applying the rights.

Jim, if you provide me with the contact information of a pizza joint (or whatever food you happen to like best) that accepts international credit cards, I'll do my best to keep my promise and get that bounty to you. IIRC this is quite common in the samba project.

I'll take this bug since it looks like it needs a champion to get the patch checked in. Still trying to find a super-reviewer ..

Comment on attachment 75533 [details] [diff] [review] nsDocumentViewer.cpp.diff sr=jst

Comment on attachment 75533 [details] [diff] [review] nsDocumentViewer.cpp.diff a=asa (on behalf of drivers) for checkin to the 1.0 trunk

The fix is in. Thanks, Jim! Does anyone know the bug number for making the stop button not grey out? I know there was one at one point, but I don't know what happened to it or whether it's still open (we might need a new bug for that).

Would this bug be what you are looking for? UI doesn't update when animations stopped with stop button http://bugzilla.mozilla.org/show_bug.cgi?id=21623 You might want to check to see if this fixes these bugs: The stop button means nothing http://bugzilla.mozilla.org/show_bug.cgi?id=115963 Stop button doesn't stop via esc http://bugzilla.mozilla.org/show_bug.cgi?id=91822 Pressing disabled stop button kills picture loading http://bugzilla.mozilla.org/show_bug.cgi?id=56618

we had to back this out because it caused some performance regressions with page layout. See Bug #133428. I think akkana is going to talk to the contributor about a new patch. re-opening.

Unfortunately, this checkin caused regression bug 133428, so the checkin was backed out. It turns out that Stop() is called at least once, and sometimes twice, at the beginning of every page load (to stop anything that might be happening on the current page), so for every page load, we were adding at least, one, and sometimes two, walks of the entire content tree looking for images to stop. Bummer! jrgm had a good suggestion in that bug: > Actually, independent of whether this is part of the slowdown in Ts, perhaps > DocumentViewerImpl::Stop() should take an argument, so that when called by > nsDocShell::Stop (i.e., user action stop), it will propagate the 'stop' to the > images, but when called from nsDocShell::SetupNewViewer (to shut down current > activity), it avoids calling SetImageAnimationMode(). Word on #mozilla suggests that Rods may be the owner of nsDocumentViewer and the person to consult about whether it's allowable to add a new argument. I don't see any other obvious way to tell when we're being called for a user-initiated Stop() as opposed to a "call Stop reflexively just in case, before loading something else". Jim, if you want to make a patch that does something like this, I'll review it and try to get it reviewed/approved in time ... It might also be really useful to know why Stop() is sometimes being called twice (that doesn't seem like a good idea).

> It might also be really useful to know why Stop() is sometimes being called > twice I think (dangerous, but occasionally I do it) that it's not being called twice, but it is being called once for each document in the page (e.g., stop is called for the parent document, and then for any child <frame>s and <iframe>s). But I don't know that for a fact, just an ad hoc observation.

Sorry to see another regression, even sorry for I am bustling to finish something else. But I still have a suggestion(not sure). When destroy the current page, don't call DocumentViewer::Stop, but just call mDocument->StopDocumentLoad(if it is not loaded). We may even be faster, by removing mPresShell->UnsuppressPainting and removing StopDocumentLoad sometimes.

Comment on attachment 75533 [details] [diff] [review] nsDocumentViewer.cpp.diff removing approval since this was backed out.

Bumping off to 1.0.1 -- probably too late to get a patch through for 1.0 even if we had a new patch.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

*** Bug 141591 has been marked as a duplicate of this bug. ***

Comment on attachment 75533 [details] [diff] [review] nsDocumentViewer.cpp.diff Marking patch that caused regressions as obsolete.

Darnit, no new patch forthcoming. Assigning back to default component owner (I took this bug because I was willing to check in the patch, which unfortunately didn't work out). I'd be happy to take it back and shepherd another patch through, though ...

And back to morse.

In case anyone is interested, this problem arose because libpr0n treats 'cached images' in a way that bypasses the general 'stop' mechanism... The original idea behind 'stopping activity' was to leverage the load group. The purpose of a Load Group was to maintain a list of all 'activity' (as a collection of nsIRequests)... So, to 'stop' a page all that was required was to call Stop() on the load group... The original imglib represented *all* image loads as nsIRequests that were added to the document's load group... Unfortunately, the 'new imagelib' bypasses the entire loadgroup mechanism for cached images (including animated GIFs). This means that a secondary 'out-of-band' mechanism is now required to 'stop' images (ie. nsIPresContext::SetAnimationMode()). This is unfortunate because it precludes cached images from conforming to *any* of the expectations held by code at 'higher levels'... (ie. being able to stop, determine whether there is activity, or even being notified when an image is being loaded...) Personally, i think that the 'correct' solution is to add cached images and animated images back into the document's load group just like any other request... -- rick

rick? I don't believe you understand how imagelib works. Imagelib puts *all* image loads in the document's loadgroup, as nsIRequests. What it does *not* put in the loadgroup is the HTTP request. If you press stop while the document is loading, images stop because of the fact they *are* in the loadgroup. Since things get removed from the loadgroup after they are done loading, at the time when people press stop to "stop animations," the document's loadgroup is empty. What this bug is about, is that people want to also be able to stop animations after the page is done loading. This has nothing at all to do with loadgroups. Because they want to be able to stop animations after the document is finished, you need to look at the current animations on a page and stop them.

pav, i wish what you say is true... but:http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgRequestProxy. cpp#103 we do *not* put requests that are currently in the cache in load groups. in the past we *did* do this. that's what the LOAD_BACKGROUND flag used to be used for.. placing animating images within the loadgroup allowed them to automatically stopped when nsIRequest::Stop() was called on the loadgroup. of course, to some extent, this was easier because we used to *have* to decode each image frame... now, i believe that we hold onto every decoded frame of an animated GIF ? so, it is easier to blt it to the screen... but we lose the ability to track the activity on the page because this does not involve an nsIRequest.

erm, you are saying that we should put an image request in the loadgroup and immediatly remove it? that doesn't make much sense to me...

This probably isn't the perfect way of doing it. In fact, the suggestion of changing stop to pass in a parameter is probably better, but looks like it requires changing stop in nsIContentViewer, which would mean all derived classes would need to be updated. My knowledge of this whole area is very limited, so I may be wrong on that point. :-) Anyway, this patch adds StopAnimation to DocumentViewer, and adds a nsIWebNavigation::STOP_ANIMATION which calls it. STOP_ALL has been updated to include STOP_ANIMATION. Those 1 or two calls to stop aren't originatine from a STOP_ALL call (one of them originates from a STOP_CONTENT), so they skip stopping animations. Esc key uses STOP_ALL, which now stops the animation. If we go with this patch, I suggest another flag: const unsigned long STOP_CRITICAL = 0x03; which would stop content and network but not animation. This would become the perfered flag to stop a something that's going to close anyway.

Arron, does the patch work for you? Animations continue here with your patch + latest CVS under Linux.

Yes, but I only have Win2k. I did a full CVS pull from the early hours of May 10th. I applied the patch (had to do make in content/ folder, and then in the docshell/) Afterwards, pressing Esc on a page with animated gifs stopped them.

no pav, i am not suggesting that you add a request and then immediatly remove it. i was imaging a more creative solution... such as creating a request that actually represented the animation... (or a meta request to represent all of the animations). my point was that there is an existing mechanism to stop activity on a page. and it would be cleaner to tie into that existing mechanism rather than force caller to call multiple APIs in order to ensure that activity has ceased. presumably, you track animated GIFs somehow... and know when once is active... so, if you reflect this knowledge in the load group all you need to do is stop the animation when nsIRequest::Cancel(...) is called. this means that consumers do NOT have to know/care about animations... -- rick

What is the semantic difference between nsIWebNavigation::STOP_CONTENT and STOP_ANIMATIONS? Currently, STOP_CONTENT is meant to stop 'activity' on a page AFTER it has been loaded from the network. This 'activity' includes: 1. Animated GIFs 2. Javascript (timeouts) 3. Plugins The motivation behind STOP_CONTENT was to tie it up to the ESC key (among other things) to allow users to stop page activity... Why would someome want to 'stop' animations independently of other content activity (via the nsIWebNavigation interface) ? -- rick

Here's the logic I took when coming up with this solution. Hopefully it'll help explain why I did what I did: 1) DocumentViewerImpl::Stop(void) was being called many times on page closing/switches. Putting stop animation code there would slow it down too much (see Comment #49) and would be redundant. 2) Adding a parameter to stop() was beyond my current knowledge of the system. 3) I decided to add a DocumentViewerImpl::StopAnimation() which solely stops animated images (although I suppose it could be expanded to stop flash and others) 4) I had a choice between calling StopAnimation within nsDocShell::Stop (STOP_CONTENT), or creating a new definition to soley stop animations. STOP_CONTENT is called when clicking a link, which means we'd be adding overhead to every link click if we stopped animations on STOP_CONTENT. So I created STOP_ANIMATION. 5) I tested STOP_ANIMATION. It was only called when the Stop or Esc buttons were pressed, or when a window or tab was closed (all via STOP_ALL). Obviously, stopping animations is not important when closing windows, so I suggested a STOP_CRITICAL which would stop content and network. STOP_CRITICAL could be used on window closings and other appropriate places. As Rick pointed out, an added advantage to this solution would be the ability to stop content (javascript) independently of animations. Personally, I don't see much usefullness in that aspect. The only point of splitting it out was to kill the overhead time of stopping animations when they don't need to be.

Freshly pulled and cleanly built CVS works on Linux with your patch. Great work Arron! Morse, rpotts, pavlov, can you r= and sr= this?

Just a nit, but wouldn't it be cleaner to define STOP_ALL as follows: const unsigned long STOP_ALL = STOP_NETWORK + STOP_CONTENT + STOP_ANIMATION; With or without that change, r=morse.

Comment on attachment 82570 [details] [diff] [review] Adding StopAnimation() to DocumentViewer Adding r= to the attachment as per morse's comment.

i guess my biggest concern with this patch is that it breaks the assumption that animations are simply content... At the very least, i think that the call to nsIDocumentViewer::StopAnimations(...) should be folded into the STOP_CONTENT code in nsDocShell::Stop(...) As near as i can tell, the *real* problem is that stopping animations is currently, an expensive process. However, rather than fixing this root problem, this patch tries to avoid the issue, by hoisting the call up into the docshell... Calling a special method from the docshell solves this particular problem, but breaks the abstraction between the docshell and content viewer (again). I know this is not the only place where we manipulate the document viewer directly in the docshell... But, it seems like stopping animations hardly warrents creating a custiom API :-) Can't we just make nsDocShellImpl::Stop() stop animations in an effecient manner? -- rick

*** Bug 144592 has been marked as a duplicate of this bug. ***

I understand where Rick is coming from. What really should be done is determine why DocumentViewerImpl::Stop(void) is being called so many times when a page is being closed/switched with another. Only one Stop() should be needed, perhaps even none. One Stop() would mean two less calls to stop animations. As well, we need to determine how to speed up stopping animations. On my machine (PIII 933) and a heavy website (CNN, slashdot, etc), mPresContext::SetImageAnimationMode() takes a second or more to run. However, this bug is over a year old, and I imagine getting someone to do it properly will take another year. In the mean time, I suggest using this patch, and filing 3 new bug reports, one named "Stop Animations when nsDocShell::Stop(STOP_CONTENT) is used", one for "reduce the # of calls to DocumentViewerImpl::Stop(void) on page switches" and one for "Speed up stopping animations on a page". The first one would be dependent upon the last two of course.

*** Bug 145540 has been marked as a duplicate of this bug. ***

After talking with jst about this problem... He suggested that we add a VoidArray to the PresContext to hold any frames that are 'stoppable' -- currently this list would hold image and object frames (so that plugins would be correctly stopped too). So, each time an image frame was created with an animated GIF (or an object frame is created) it adds itself to the VoidArray in the corresponding PresContext... Then, if we need to stop content , we simply pull each frame out of the void array and stop it... as each frame is stopped it is removed from the array. This way, if stop is called more than once, there is not alot of overhead because the array is empty... How does this approach sound? -- rick

I just want to note that I don't believe Stop is being called more than once per document, unless someone has evidence to the contrary. See comment #50. It's just that SetImageAnimationMode, which walks the content tree, was too expensive to pay on every page load. Maintaining a list for "stoppable" items seems appropriate, hopefully cheap enough.

Mozilla 1.0 RC2/Win2K hanged on me two times when using STOP button to stop loading a page. Is it related to this bug, or is it something else?

It's not related.

Sounds like a good solution to me. :) If I read the code correctly, the code in nsHTMLDocument:GetImages could be replaced by this new array, and that would save many CPU cycles when someone calls GetImages. In light of that, perhaps the image list should not be an array, but a nsIDOMHTMLCollection? As for DocumentViewerImpl::Stop, it is being called a few times. Just put a printf into it and watch. Below are two examples. It really doesn't matter (in respects to this bug) if it's being called many times, just as long as there's some logic in your new SetImageAnimationMode to check to see if the passed in parameter is indeed different than mImageAnimationMode. If it's the same, no need to walk the array again. -- Steps: Go to www.mozilla.org. Switched to www.google.com (same window, new URL) 1) DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x035e9238) line 1720 nsDocShell::Stop(nsDocShell * const 0x0316a8c0, unsigned int 2) line 2466 LocationImpl::SetHrefWithBase(const nsAString & {...}, nsIURI * 0x034de998, int 0) line 554 2) DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x035e9238) line 1720 nsDocShell::SetupNewViewer(nsDocShell * const 0x0316a8b0, nsIContentViewer * 0x036bde18) line 4386 ---- Close google.com window 1) DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x0179a640) line 1720 nsDocShell::Stop(nsDocShell * const 0x0183b928, unsigned int 7) line 2466 nsDocShell::Destroy(nsDocShell * const 0x0183b92c) line 2707 2) For Child 0 DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x036bde18) line 1720 nsDocShell::Stop(nsDocShell * const 0x0316a8c0, unsigned int 7) line 2466 nsDocShell::Stop(nsDocShell * const 0x0183b928, unsigned int 7) line 2495 nsDocShell::Destroy(nsDocShell * const 0x0183b92c) line 2707 3) For Child 1 DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x033c79d0) line 1720 nsDocShell::Stop(nsDocShell * const 0x034d7210, unsigned int 7) line 2466 nsDocShell::Stop(nsDocShell * const 0x0183b928, unsigned int 7) line 2495 4) DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x033c79d0) line 1720 nsDocShell::Stop(nsDocShell * const 0x034d7210, unsigned int 7) line 2466 nsDocShell::Destroy(nsDocShell * const 0x034d7214) line 2707 nsWebShell::Destroy(nsWebShell * const 0x034d7214) line 1298 nsFrameLoader::Destroy(nsFrameLoader * const 0x034c3a30) line 270 nsHTMLFrameInnerFrame::Destroy(nsHTMLFrameInnerFrame * const 0x034cc6e8, nsIPresContext * 0x029066b8) line 728 nsFrameList::DestroyFrames(nsIPresContext * 0x029066b8) line 131 nsContainerFrame::Destroy(nsContainerFrame * const 0x034d8018, nsIPresContext * 0x029066b8) line 141 5) DocumentViewerImpl::Stop(DocumentViewerImpl * const 0x036bde18) line 1720 nsDocShell::Stop(nsDocShell * const 0x0316a8c0, unsigned int 7) line 2466 nsDocShell::Destroy(nsDocShell * const 0x0316a8c4) line 2707 nsWebShell::Destroy(nsWebShell * const 0x0316a8c4) line 1298 nsFrameLoader::Destroy(nsFrameLoader * const 0x0316a858) line 270 nsHTMLFrameInnerFrame::Destroy(nsHTMLFrameInnerFrame * const 0x030f8708, nsIPresContext * 0x029066b8) line 728 nsFrameList::DestroyFrames(nsIPresContext * 0x029066b8) line 131 nsContainerFrame::Destroy(nsContainerFrame * const 0x0316069c, nsIPresContext * 0x029066b8) line 141 nsFrameList::DestroyFrames(nsIPresContext * 0x029066b8) line 131

*** Bug 145726 has been marked as a duplicate of this bug. ***

reassigned to component owner

Comment on attachment 82570 [details] [diff] [review] Adding StopAnimation() to DocumentViewer Unfortunately, this patch has bitrotted and no longer applies to current CVS...

What's the status on this bug? 1.1a is long gone. I can certainly unbitrot the patch I created, but only if there's a reason to. As I said in Comment #75 (second paragraph), the patch was meant to be an "in between" fix, so that we'd have animation stopping ablilities while the real fix was done (Comment #77). For the last 2 months, we could have had at least a temporary way to stop animation (assuming the patch didn't cause pageload regression)

Accessibilty team needs an ETA for this as well.

*** Bug 160678 has been marked as a duplicate of this bug. ***

What is the current status of this (stop animated gifs) bug/feature request?

what would you guess what the current status is, looking at the # of recent comments?

hoping for some action on this bug.

Somebody please look at the patch and get this in in the near future. This is a _basic_ feature and should not be missing from a decent browser...

This is a real serious usability thing.

Renominating - this is necessary for accessibility.

Working version of patch attachment 82570 [details] [diff] [review] To summarize what I remember about this bug: - First patch caused performance regressions because stop was being called many times (even on page load) - 2nd patch was meant as a "in between" patch until someone properly makes a patch that creates a stored list of animated images, which then can be quickly run through and stopped when ESC or stop is pressed. - 3rd patch is a freshened 2nd patch - It's unknown if the 2nd/3rd patch causes performance regressions. If it does, it should be less than the 1st patches perf regression.

Paper, Kerz mentioned that he should be able to help you test this for perf regressions.

Nav triage team: nsbeta1+/adt3

We have a patch for this -- why can't it move forward? Kerz, do you need anything else from us to test this for perf regressions?

I believe that the issue is that the patch we have is not the 'right' patch. -- rick

Rick, what's wrong with it? Paper submitted a new patch, and there are no comments on that one.

As near as i can tell, this new patch basically the same as the old one -- just works on the tip... the issues are still the same -- see comment #73. basically, i think we should be addressing the underlying problem -- that stopping animations is expensive.. rather than creating new public APIs to hack around the problem. -- rick

*** Bug 163708 has been marked as a duplicate of this bug. ***

Over to jag.

Here is an animated GIF whose movement can't be stopped. Pressing ESC has no visible effect. The "Stop" icon is dimmed from the start. The "Stop" menu item is disabled from the start. This is with build 2003021008 (1.3b).

still problem in 1.4a (2003040105)

Still broken in 1.4rc1 (2003052908)

Comment on attachment 101229 [details] [diff] [review] Adding StopAnimation() to DocumentViewer Patch no longer applies.

The patch avoids paying a hefty price when unneeded. That is, when the docViewer is notified that it will be going away [in DocumentViewerImpl::Unload()], flag that no further rendering will arise. And check that flag later in Stop(). If the flag hasn't been set, it means that the document is being rendered, and hitting Esc (or the stop button when bug 21623 is fixed) therefore activates the stop animation code path. So the patch achieves the same effect as passing a param to stop, but without tree-wide ramifications... and with the added robustness that JavaScript'ers expect from the back-end of the Unload() notification.

-> taking to drive the patch in

Are you sure that mEnableRendering is initialized to PR_FALSE, and that it's PR_FALSE at startup time? I don't see it being initialized at all, so its initial value at startup time looks like it will be random; then it's set to true as soon as PrepareToStartLoad is called -- does that happen before or after the various times Stop() gets called during the initial page load process? Have you run the page load performance test? If the test says load time doesn't go up, that would make me feel better, though I'd be happier if the value was initialized if we're going to rely on its initial value. (Or am I missing an initialization somewhere?)

Akkana, DocumentViewerImpl has a new operator that zeroes out all its members (NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW), so mEnableRendering is set to false on creation of DocumentViewerImpl objects.

Comment on attachment 127058 [details] [diff] [review] fast patch sr=jst

Comment on attachment 127058 [details] [diff] [review] fast patch If jst is happy with it, then I am.

Checked in. I have filed bug 212455 about comment 81 that Stop() can be called a few times.

*** Bug 213165 has been marked as a duplicate of this bug. ***

Filed bug 213377 for Firebird.

v

Can anyone from this thread help? "They" have betrayed us and have hacked this feature to death! Where are you? Please return, we need you!

(In reply to gk_2000 from comment #117) > Can anyone from this thread help? "They" have betrayed us and have hacked > this feature to death! Where are you? Please return, we need you! This bug was VERIFIED FIXED in 2003 and nothing significant has happened in this bug report since then. If it has suddenly stopped working, try to find a "last good" and a "first bad" versions, and open a new bug with the "regression" keyword (after checking, of course, that the same "new bug" hasn't been reported by someone else — if it has, just vote for that "new bug from someone else").

(In reply to Tony Mechelynck [:tonymec] from comment #118) > (In reply to gk_2000 from comment #117) > > Can anyone from this thread help? "They" have betrayed us and have hacked > > this feature to death! Where are you? Please return, we need you! > > This bug was VERIFIED FIXED in 2003 and nothing significant has happened in > this bug report since then. If it has suddenly stopped working, try to find > a "last good" and a "first bad" versions, and open a new bug with the > "regression" keyword (after checking, of course, that the same "new bug" > hasn't been reported by someone else — if it has, just vote for that "new > bug from someone else"). Sir, I request you to try this functionality in the newest versions (>20) of Mozilla. More about it in this closed thread: https://bugzilla.mozilla.org/show_bug.cgi?id=825486

This bug should be reopened, and tagged as an Accessibility issue. Many users have unusual brain wiring that results in them having hypervigilance (little to no ability to employ selective attention). Unlike most people, when it comes to distracting extraneous animation in the field of view, these folks can't "just ignore it" / "just tune it out", making this a serious problem. For comparison, note that most OSes have options to disable their use of unnecessary animations, and on Windows and iOS in particular, these are located under the Accessibility settings. Similarly, W3C and Android accessibility guidelines require giving users the ability to pause, stop, or hide looping animated elements. The proper bug to reopen would actually be bug 825486 from 5 years ago, where it was requested to fix the regression that removed the ability (reportedly present since Netscape version 1) to stop animated images. However, that bug has not only been closed as WONTFIX, but also been set so that normal Bugzilla users are prohibited from adding any comments, e.g. to provide reasoning why the developers should consider reopening it. That strikes me as incredibly user-hostile; I've never seen a bug-tracker issue set that way before. And presumably opening a new bug would just result in yet another RESOLVED DUPLICATE of the WONTFIX for the collection. Note that this might even be seen as a contravention of the Americans with Disabilities Act, since Mozilla is U.S.-based. The rationale for the WONTFIX was that web apps might want to use the Esc key; that's fine, but I think that's a far far more niche case than people who need to be able to stop animated images. And if you make the keystroke Shift + Esc, I think that tiny niche essentially disappears. If it's really a concern, an about:config setting could certainly be provided to allow changing and/or disabling the keystroke (and an addable toolbar button could be provided so animations could be stopped even with the keystroke disabled, like the old Stop button used to be able to do). The other justification for the WONTFIX was "Just use SuperStop or BetterStop; problem solved." That's great, and I have been using SuperStop for years, but it and BetterStop are XUL/XPCOM add-ons which will cease working next month with Firefox 57. And even for people moving to ESR or a Firefox fork to continue being able to use "legacy" extensions, these add-ons do not have the ability to stop animated favicons. My preferred solution is the traditional ability to have animated images play by default, and then be able to instantly stop all of them on the page/tab with a keystroke (or button-press), since animated GIFs and PNGs are sometimes informational (e.g. looping clips of video footage that constitute actual content on the page). However, lacking that ability, one could go to about:config and set image.animation_mode = once. This can still result in an intolerable amount of distracting animation, though (e.g. while scrolling through forum threads that use animated emoticons), since image animations will not stop until they get through all their frames while visible in the window (which sometimes takes quite a long time, if frame count and/or interframe delay are high). And even if one were able to tolerate that, setting image.animation_mode = once does not affect animated favicons. A third potential solution is the ability to quickly toggle image animation on or off globally, on demand. I use the QuickJava add-on to achieve this. Unfortunately it, too, is a XUL/XPCOM add-on with no announced plans to make a WebExtension version. Presumably much of its functionality (including animated image disabling) wouldn't be possible as a WebExtension anyway. And, once again, this is not a full solution even for people migrating to a browser version that continues to support traditional add-ons, since turning off QuickJava's "A" button does not stop animated favicons. If I'm not mistaken, even if the authors of these add-ons (which have not been updated in years) planned to rewrite them as WebExtensions, they would be unable to do so, without having a CSS property that allows disabling animations. A proposal has been made as https://github.com/w3c/csswg-drafts/issues/1615 to add such a property, but has received very little attention. I think until such time as that becomes a standard CSS property, a Firefox-specific one should be added, à la -moz-user-select etc., so that this important accessibility guideline can be met in Firefox 57 and beyond.