Cannot stop animation with stop button or escape key

VERIFIED FIXED in mozilla1.5alpha

Status

defect
P3
major
VERIFIED FIXED
19 years ago
2 years ago

People

(Reporter: moz_user, Assigned: rbs)

Tracking

(Blocks 3 bugs, {access, helpwanted, regression})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

19 years ago
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 2

19 years ago
Do you get any feedback on the console?
Reporter

Comment 3

19 years ago
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

18 years ago
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

18 years ago
updating component
Assignee: asa → pavlov
Component: Browser-General → ImageLib
QA Contact: doronr → tpreston

Updated

18 years ago
Whiteboard: [imagelib]

Comment 6

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

Comment 7

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

Comment 8

18 years ago
All operating systems, major (many news sites are unreadable without this 
feature), regression.
Severity: normal → major
Keywords: regression
OS: Linux → All
Hardware: PC → All

Comment 9

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

Comment 10

18 years ago
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.
Keywords: 4xp, nsCatFood

Comment 11

18 years ago
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.

Updated

18 years ago
Target Milestone: --- → mozilla0.9.2

Comment 12

18 years ago
I agree. This should be moved to 0.9.1 specially cuz akkana is willing to help! :)
Target Milestone: mozilla0.9.2 → mozilla0.9.1

Comment 13

18 years ago
The right way to do this isn't trivial.

Comment 14

18 years ago
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...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Blocks: 40388
Blocks: 21623

Updated

18 years ago
Priority: -- → P4

Comment 15

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

Updated

18 years ago
Keywords: rtm

Comment 16

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

Updated

18 years ago
Blocks: 11875

Updated

18 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 17

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

Updated

18 years ago
Keywords: nsenterprise

Updated

18 years ago
Keywords: access
*** Bug 89507 has been marked as a duplicate of this bug. ***
*** Bug 90491 has been marked as a duplicate of this bug. ***

Comment 20

18 years ago
-> 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 21

18 years ago
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

18 years ago
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.
Keywords: mozilla0.9.3
Removing nsenterprise nomination.
Keywords: nsenterprise

Updated

18 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 24

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

Marcos.

Updated

18 years ago
Blocks: 96873

Updated

18 years ago
Blocks: 98995

Comment 25

18 years ago
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Updated

18 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.8

Updated

18 years ago
Summary: Cannot stop animation with webNavigation.stop → Cannot stop animation with webNavigation.stop (escape key)

Comment 26

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

Updated

18 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

18 years ago
Blocks: 119597

Comment 27

18 years ago
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.
Keywords: mozilla1.0

Comment 28

18 years ago
Also it's a regression, 4.x parity, and affects accessibility.  Nominate nsbeta1
as well.
Keywords: nsbeta1

Updated

18 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
*** Bug 128180 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Blocks: advocacybugs

Comment 30

18 years ago
per adt, critical for nsbeta1. hence plus.
Keywords: nsbeta1nsbeta1+
Priority: P4 → P3
Whiteboard: [imagelib] → [adt3]
*** Bug 129901 has been marked as a duplicate of this bug. ***

Comment 32

17 years ago
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.
Assignee: pavlov → trudelle
Component: ImageLib → XP Apps
QA Contact: tpreston → paw

Comment 33

17 years ago
->morse
Assignee: trudelle → morse

Comment 34

17 years ago
Posted patch nsDocumentViewer.cpp.diff (obsolete) — Splinter Review
Please Review and Comment on this. ;)

Comment 35

17 years ago
--> over to tpreston@netscape.com
QA Contact: paw → tpreston

Comment 36

17 years ago
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.

Updated

17 years ago
Keywords: patch, review
Whiteboard: [adt3] → [adt3][fix in hand]

Comment 37

17 years ago
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!
Attachment #75533 - Flags: review+

Comment 38

17 years ago
>(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.

Comment 40

17 years ago
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

17 years ago
:)
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

17 years ago
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

17 years ago
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 ..
Assignee: morse → akkana
Comment on attachment 75533 [details] [diff] [review]
nsDocumentViewer.cpp.diff

sr=jst
Attachment #75533 - Flags: superreview+
Comment on attachment 75533 [details] [diff] [review]
nsDocumentViewer.cpp.diff

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #75533 - Flags: approval+

Comment 46

17 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 47

17 years ago
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

17 years ago
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. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 49

17 years ago
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

17 years ago
> 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

17 years ago
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.
Attachment #75533 - Flags: approval+

Comment 53

17 years ago
Bumping off to 1.0.1 -- probably too late to get a patch through for 1.0 even if
we had a new patch.
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 54

17 years ago
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.
Keywords: nsbeta1-

Comment 55

17 years ago
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.
Keywords: nsbeta1+
*** Bug 141591 has been marked as a duplicate of this bug. ***

Comment 57

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

Marking patch that caused regressions as obsolete.
Attachment #75533 - Attachment is obsolete: true

Comment 58

17 years ago
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 ...
Assignee: akkana → sgehani
Status: REOPENED → NEW
QA Contact: tpreston → paw

Comment 59

17 years ago
And back to morse.
Assignee: sgehani → morse

Updated

17 years ago
Keywords: patch, reviewhelpwanted

Comment 60

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

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

Comment 66

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

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

Morse, rpotts, pavlov, can you r= and sr= this?
Keywords: patch, review

Comment 71

17 years ago
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.

Updated

17 years ago
Summary: Cannot stop animation with webNavigation.stop (escape key) → Cannot stop animation with stop button or escape key

Comment 72

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

Adding r= to the attachment as per morse's comment.
Attachment #82570 - Attachment description: Adding StopAnimation() to DocumentViewier → Adding StopAnimation() to DocumentViewer
Attachment #82570 - Flags: review+

Comment 73

17 years ago
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

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

Comment 75

17 years ago
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

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

Comment 77

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
It's not related.

Comment 81

17 years ago
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. ***

Updated

17 years ago
Target Milestone: mozilla1.0.1 → mozilla1.1alpha

Comment 83

17 years ago
reassigned to component owner
Assignee: morse → sgehani

Comment 84

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

Unfortunately, this patch has bitrotted and no longer applies to current CVS...
Attachment #82570 - Attachment is obsolete: true

Comment 85

17 years ago
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

17 years ago
Accessibilty team needs an ETA for this as well.

Comment 87

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

Comment 88

17 years ago
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?

Updated

17 years ago
Blocks: 160678

Updated

17 years ago
Blocks: 163708
hoping for some action on this bug.

Comment 91

17 years ago
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...
Keywords: mozilla1.0mozilla1.2

Updated

17 years ago
Blocks: majorbugs

Comment 92

17 years ago
This is a real serious usability thing.

Comment 93

17 years ago
Renominating - this is necessary for accessibility.
Keywords: nsbeta1-nsbeta1

Comment 94

17 years ago
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

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

Comment 96

17 years ago
Nav triage team: nsbeta1+/adt3
Keywords: nsbeta1nsbeta1+

Comment 97

17 years ago
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

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

-- rick

Comment 99

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

Comment 100

17 years ago
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

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

Comment 102

17 years ago
Over to jag.
Assignee: sgehani → jaggernaut

Updated

17 years ago
Whiteboard: [adt3][fix in hand] → [adt3]

Updated

17 years ago
Keywords: mozilla1.2

Updated

17 years ago
Target Milestone: mozilla1.1alpha → mozilla1.4alpha

Comment 103

16 years ago
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).

Updated

16 years ago
Target Milestone: mozilla1.4alpha → mozilla1.4beta

Comment 104

16 years ago
still problem in 1.4a (2003040105)

Comment 105

16 years ago
Still broken in 1.4rc1 (2003052908)

Comment 106

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

Patch no longer applies.
Attachment #101229 - Attachment is obsolete: true
Assignee

Comment 107

16 years ago
Posted patch fast patchSplinter Review
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.
Assignee

Comment 108

16 years ago
-> taking to drive the patch in
Assignee: jaggernaut → rbs
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Assignee

Updated

16 years ago
Attachment #127058 - Flags: superreview?(jst)
Attachment #127058 - Flags: review?(akkana)

Comment 109

16 years ago
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
Attachment #127058 - Flags: superreview?(jst) → superreview+

Comment 112

16 years ago
Comment on attachment 127058 [details] [diff] [review]
fast patch

If jst is happy with it, then I am.
Attachment #127058 - Flags: review?(akkana) → review+
Assignee

Comment 113

16 years ago
Checked in.

I have filed bug 212455 about comment 81 that Stop() can be called a few times.
Status: NEW → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED
*** Bug 213165 has been marked as a duplicate of this bug. ***

Comment 115

16 years ago
Filed bug 213377 for Firebird.

Comment 116

16 years ago
v
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.5alpha → M1

Updated

16 years ago
Target Milestone: M1 → mozilla1.5alpha
Product: Core → Mozilla Application Suite

Updated

14 years ago
No longer blocks: majorbugs

Comment 117

6 years ago
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").

Comment 119

6 years ago
(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

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.