70030 - Cannot stop animation with stop button or escape key

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Becki Sanford]

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.

Comment 1

[Becki Sanford]

Attachment: Sample XUL file containing animated GIF and Reload/Stop controls

Comment 2

[Peter "jag" Annema]

Do you get any feedback on the console?

Comment 3

[Becki Sanford]

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.

Comment 4

[Oliver Klee]

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.

Comment 5

[Asa Dotzler [:asa]]

updating component

Comment 6

[timeless]

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

Comment 7

[Jesse Ruderman]

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

Comment 8

[Jesse Ruderman]

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

Comment 9

[Gilles Durys]

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

Comment 10

[Akkana Peck]

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.

Comment 11

[Akkana Peck]

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.

Comment 12

[Gagan]

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

Comment 13

[Stuart Parmenter]

The right way to do this isn't trivial.

Comment 14

[Angela Butler]

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...

Comment 15

[R.K.Aa.]

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

Comment 16

[Jesse Ruderman]

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

Comment 17

[Matthew Cline]

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

Comment 18

[Matthias Versen [:Matti]]

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

Comment 19

[Matthias Versen [:Matti]]

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

Comment 20

[Jason Kersey]

-> 0.9.4

Comment 21

[Graham Leggett]

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.

Comment 22

[Diego Biurrun]

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.

Comment 23

[Jussi-Pekka Mantere]

Removing nsenterprise nomination.

Comment 24

[mdimitrio]

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

Marcos.

Comment 25

[Stuart Parmenter]

.

Comment 26

[:deanis74]

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

Comment 27

[Diego Biurrun]

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.

Comment 28

[Akkana Peck]

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

Comment 29

[Matthias Versen [:Matti]]

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

Comment 30

[Gagan]

per adt, critical for nsbeta1. hence plus.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Stuart Parmenter]

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.

Comment 33

[Peter Trudelle]

->morse

Comment 34

[Jim Song]

Attachment: nsDocumentViewer.cpp.diff

Please Review and Comment on this. ;)

Comment 35

[Paul Wyskoczka]

--> over to [e-mail]

Comment 36

[Diego Biurrun]

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 37

[Akkana Peck]

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!

Comment 38

[mdimitrio]

>(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.

Comment 39

[Reinout van Schouwen]

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.

Comment 40

[Akkana Peck]

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?

Comment 41

[Jim Song]

:)
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.

Comment 42

[Diego Biurrun]

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.

Comment 43

[Akkana Peck]

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 44

[Johnny Stenback (:jst)]

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

sr=jst

Comment 45

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 46

[Akkana Peck]

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).

Comment 47

[amutch]

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

Comment 48

[Scott MacGregor]

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. 

Comment 49

[Akkana Peck]

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).

Comment 50

[John Morrison]

> 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.

Comment 51

[Jim Song]

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 52

[Asa Dotzler [:asa]]

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

removing approval since this was backed out.

Comment 53

[Akkana Peck]

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

Comment 54

[scottputterman]

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

Comment 55

[scottputterman]

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

Comment 56

[Matthias Versen [:Matti]]

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

Comment 57

[Diego Biurrun]

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

Marking patch that caused regressions as obsolete.

Comment 58

[Akkana Peck]

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 ...

Comment 59

[Samir Gehani]

And back to morse.

Comment 60

[rpotts (gone)]

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

Comment 61

[Stuart Parmenter]

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.

Comment 62

[rpotts (gone)]

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.

Comment 63

[Stuart Parmenter]

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...

Comment 64

[ArronM (:paper)]

Attachment: Adding StopAnimation() to DocumentViewer

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.

Comment 65

[Diego Biurrun]

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

Comment 66

[ArronM (:paper)]

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.

Comment 67

[rpotts (gone)]

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

Comment 68

[rpotts (gone)]

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

Comment 69

[ArronM (:paper)]

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.

Comment 70

[Diego Biurrun]

Freshly pulled and cleanly built CVS works on Linux with your patch.  Great work
Arron!

Morse, rpotts, pavlov, can you r= and sr= this?

Comment 71

[Stephen P. Morse]

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 72

[Diego Biurrun]

Comment on attachment 82570 [details] [diff] [review]
Adding StopAnimation() to DocumentViewer

Adding r= to the attachment as per morse's comment.

Comment 73

[rpotts (gone)]

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

Comment 74

[R.K.Aa.]

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

Comment 75

[ArronM (:paper)]

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.

Comment 76

[R.K.Aa.]

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

Comment 77

[rpotts (gone)]

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

Comment 78

[John Morrison]

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.

Comment 79

[Poligraf Sharikov]

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?

Comment 80

[John Morrison]

It's not related.

Comment 81

[ArronM (:paper)]

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

Comment 82

[Matthias Versen [:Matti]]

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

Comment 83

[Stephen P. Morse]

reassigned to component owner

Comment 84

[Diego Biurrun]

Comment on attachment 82570 [details] [diff] [review]
Adding StopAnimation() to DocumentViewer

Unfortunately, this patch has bitrotted and no longer applies to current CVS...

Comment 85

[ArronM (:paper)]

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)

Comment 86

[Aaron Leventhal]

Accessibilty team needs an ETA for this as well.

Comment 87

[Alfonso Martinez]

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

Comment 88

[Jason Price]

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

Comment 89

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 90

[Simon Fraser [no longer active]]

hoping for some action on this bug.

Comment 91

[Diego Biurrun]

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...

Comment 92

[Jon Brams]

This is a real serious usability thing.

Comment 93

[Aaron Leventhal]

Renominating - this is necessary for accessibility.

Comment 94

[ArronM (:paper)]

Attachment: Adding StopAnimation() to DocumentViewer

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.

Comment 95

[Aaron Leventhal]

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

Comment 96

[Samir Gehani]

Nav triage team: nsbeta1+/adt3

Comment 97

[Aaron Leventhal]

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?

Comment 98

[rpotts (gone)]

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

-- rick

Comment 99

[Aaron Leventhal]

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

Comment 100

[rpotts (gone)]

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

Comment 101

[Michael Gabriel]

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

Comment 102

[Samir Gehani]

Over to jag.

Comment 103

[Daniel B.]

Attachment: another case: animated GIF file that can't be stopped

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).

Comment 104

[Daniel B.]

still problem in 1.4a (2003040105)

Comment 105

[Daniel B.]

Still broken in 1.4rc1 (2003052908)

Comment 106

[Diego Biurrun]

Comment on attachment 101229 [details] [diff] [review]
Adding StopAnimation() to DocumentViewer

Patch no longer applies.

Comment 107

[rbs]

Attachment: fast patch

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.

Comment 108

[rbs]

-> taking to drive the patch in

Comment 109

[Akkana Peck]

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?)

Comment 110

[Johnny Stenback (:jst)]

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 111

[Johnny Stenback (:jst)]

Comment on attachment 127058 [details] [diff] [review]
fast patch

sr=jst

Comment 112

[Akkana Peck]

Comment on attachment 127058 [details] [diff] [review]
fast patch

If jst is happy with it, then I am.

Comment 113

[rbs]

Checked in.

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

Comment 114

[Matthias Versen [:Matti]]

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

Comment 115

[Jeremy M. Dolan]

Filed bug 213377 for Firebird.

Comment 116

[Daniel Wang]

v

Comment 117

[gk_2000]

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!

Comment 118

[Tony Mechelynck [:tonymec]]

(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").

Comment 119

[gk_2000]

(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

Comment 120

[Dan Harkless]

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.