Peter Simonyi Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180621125625

Steps to reproduce:

Load attached testcase.

The key is that the 'content' for a ::before pseudo-element is set to an animated image.  In this case I used an APNG because it was convenient, but I tried a GIF too and it works the same.

This testcase also has a checkbox that, when checked, sets the checkbox's background-image to the same image as above.  (It's a replaced element, so it doesn't show, but it still seems to make a difference.)


Actual results:

The image doesn't appear animated — unless you check the checkbox!  Adding the same animation anywhere on the page seems to make the generated-content animation work again.


Expected results:

The image should always be animated.

	Peter Simonyi Reporter
	Comment 1 • 6 years ago

Mozregression:
No more inbound revisions, bisection finished.
Last good revision: 8d1f4fe78843a2948e244414ee8bdf46660bea39
First bad revision: cd4d140d2826391e40660efce32438d01b181a1e
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d1f4fe78843a2948e244414ee8bdf46660bea39&tochange=cd4d140d2826391e40660efce32438d01b181a1e

That range is just bug 215083.

	Daniel Holbert [:dholbert]
	Comment 2 • 6 years ago

(mozregression range sounds like 63 is the first affected release, then --> setting flags accordingly.)

	Daniel Holbert [:dholbert]
	Comment 3 • 6 years ago

More specific regression range: I can reproduce the bug with this cset (the first from bug 215083):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca43c308ce1
...but not with this cset (its parent):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1f4fe78843

So this is specifically a regression from 2ca43c308ce1.

Based on the way this is manifesting, I think it's a scenario where we're failing to trigger "nsLayoutUtils::RegisterImageRequestIfAnimated()"

	Daniel Holbert [:dholbert]
	Comment 4 • 6 years ago

I poked at this in rr a bit (breaking in nsImageFrame::Init, which is where we should call imageLoader->FrameCreated, which is what normally previously got us registered for animation repaints via nsLayoutUtils::RegisterImageRequestIfAnimated)

So far I found 2 distinct issues, in a built targeted at https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca43c308ce1 (which I assume persist in mozilla-central):

 (1) We are creating an image with Kind = NonGeneratedContentProperty , which is *the wrong type* in this case, since this testcase uses ::before which is generated content.  In particular, I can see that we're hitting the ShouldCreateImageFrameForContent() check in nsCSSFrameConstructor and returning "true", which makes us use NS_NewImageFrameForContentProperty.  We presumably are expecting to intercept generated content somewhere before we hit that, not sure though...


 (2) Animation is apparently broken for images with Kind = NonGeneratedContentProperty.  I'll attach a testcase that demonstrates this more directly (removing the ::before selector from the testcase we've got now).


I assume (1) will become a non-issue as of bug 1472403 so perhaps it's not worth focusing on; but (2) might remain an issue.

	Daniel Holbert [:dholbert]
	Comment 5 • 6 years ago

(In reply to Daniel Holbert [:dholbert] from comment #4)
>  (2) Animation is apparently broken for images with Kind =
> NonGeneratedContentProperty.  I'll attach a testcase that demonstrates this
> more directly (removing the ::before selector from the testcase we've got
> now).

Here's a testcase to demonstrate this. (This particular testcase isn't a regression, since it didn't work at all before bug 215083 landed, but it demonstrates a defect in our implementation from that bug.)

	Daniel Holbert [:dholbert]
	Comment 6 • 6 years ago

I meant to say in comment 4 but didn't quite lay out the connection:

Since we're getting an image with Kind == NonGeneratedContentProperty (which, as noted, is the wrong state to be in for the original testcase here at least), we end up skipping the Kind-guarded FrameCreated() call here:
https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/layout/generic/nsImageFrame.cpp#306-312

...and that's where we would've made our RegisterImageRequestIfAnimated() call to start receiving repaint callbacks from the refresh driver.  So that's why we're not animating here.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 7 • 6 years ago

Yeah, this makes sense. Note that shortly after landing the patches that regressed this I filed bug 1472403, which notices that we are indeed using the non-generated path. I'm going to write the fix on top of that I think, since it makes it easier to reason about, given we no longer have three different code paths.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 8 • 6 years ago

I thought of just moving the tracking to nsImageFrame instead of
nsImageLoadingContent entirely, though that would mean I need to handle it also
in nsImageBoxFrame and nsSVGImageFrame, which was even more duplicated code.

Ideas for testing this welcome, though all our animated image test-cases are
borked (all reftests in image/test/reftest/apng are disabled, and all the ones
for gifs that have animations as well).

I'll cross-ref this bug and bug 1473651 so that we can write a test for this
once that's fixed.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 6 years ago

Comment on attachment 8990174 [details]
Bug 1473651: Track animated image status for generated content and content: url().

Timothy, you don't have a Phabricator thingie, but either your or dholbert's review works for me :)

	Phabricator Automation
	Comment 10 • 6 years ago

Comment on attachment 8990174 [details]
Bug 1473651: Track animated image status for generated content and content: url().

Timothy Nikkel (:tnikkel) has approved the revision.

https://phabricator.services.mozilla.com/D1994

	Timothy Nikkel (:tnikkel)
	Comment 11 • 6 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> Ideas for testing this welcome, though all our animated image test-cases are
> borked (all reftests in image/test/reftest/apng are disabled, and all the
> ones
> for gifs that have animations as well).

We definitely have tests that test that animated images animate. The first one that comes to mind is image/test/mochitest/test_discardAnimatedImage.html

In bug 215083, comment 68 and bug 215083, comment 69 we figured that the register calls were being made, so what did we miss there?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 12 • 6 years ago

So the register call that we thought should happen doesn't happen because we don't associate the request to the frame, and thus we bail out in:

  https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/layout/style/ImageLoader.cpp#627

I think we can't associate it from DidSetStyleContext as we do for backgrounds, borders, etc, since we have a clone of the request, so we need to register it manually when appropriate.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 13 • 6 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> > Ideas for testing this welcome, though all our animated image test-cases are
> > borked (all reftests in image/test/reftest/apng are disabled, and all the
> > ones
> > for gifs that have animations as well).
> 
> We definitely have tests that test that animated images animate. The first
> one that comes to mind is image/test/mochitest/test_discardAnimatedImage.html

I tried to extend this test but it didn't quite work out because they use the scripted observers of nsImageLoadingContent, which we don't use for this... Any other idea?

	Pulsebot
	Comment 14 • 6 years ago

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/33e19a7ad664
Track animated image status for generated content and content: url(). r=tnikkel

	Timothy Nikkel (:tnikkel)
	Comment 15 • 6 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
> (In reply to Timothy Nikkel (:tnikkel) from comment #11)
> > (In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> > > Ideas for testing this welcome, though all our animated image test-cases are
> > > borked (all reftests in image/test/reftest/apng are disabled, and all the
> > > ones
> > > for gifs that have animations as well).
> > 
> > We definitely have tests that test that animated images animate. The first
> > one that comes to mind is image/test/mochitest/test_discardAnimatedImage.html
> 
> I tried to extend this test but it didn't quite work out because they use
> the scripted observers of nsImageLoadingContent, which we don't use for
> this... Any other idea?

Ah right, sorry about that. The approach that that test used (taking snapspots with drawWindow) should be suitable though, we just don't get frame update notifications for when to take a snapshot. We have image/test/mochitest/animationPolling.js which does that in general but it seems to be only suitable for use on animated images that have a single (or finite) loop count. This case is similar, we just want to make sure that the image animates (it produces at least two different renderings that are also different from
not showing the image as well), but we don't care what the "final" image looks like. Perhaps you could build off of that?

	Cristian Brindusan [:cbrindusan]
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/33e19a7ad664

	Peter Simonyi Reporter
	Comment 17 • 6 years ago

Both of the attached testcases are animated now.  Thanks!

	Emil Ghitta, QA [:emilghitta]
	Comment 18 • 6 years ago

I have managed to reproduce this issue using Firefox 63.0a1 (BuildId:20180705220111) on Windows 10 64bit.

This issue is verified fixed using Firefox 63.0b7 (BuildId:20180917143811) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.