1473651 - Animated images don't animate when used in CSS generated content unless the animation is already running elsewhere

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Peter Simonyi]

Attachment: Testcase

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.

Comment 1

[Peter Simonyi]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (using 'content' on a div rather than on generated content - i.e. directly using the new feature from bug 215083 )

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

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1473651: Track animated image status for generated content and content: url().

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Comment 10

[Phabricator Automation]

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

Comment 11

[Timothy Nikkel (:tnikkel)]

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33e19a7ad664
Track animated image status for generated content and content: url(). r=tnikkel

Comment 15

[Timothy Nikkel (:tnikkel)]

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

Comment 16

[Cristian Brindusan [:cbrindusan]]

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

Comment 17

[Peter Simonyi]

Both of the attached testcases are animated now.  Thanks!

Comment 18

[Emil Ghitta, QA [:emilghitta]]

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.