1281428 - SVG image with animated opacity and a fill filter is blank until hovered

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Florian Quèze [:florian]]

The animation I'm adding in bug 1275262 stopped working recently. In my fx-team build from 2016-06-10 it worked fine; in Gijs' fx-team build from 2016-06-13 he observed that the camera icon was just blank (I now reproduced the same issue on a current fx-team build), and reappeared after hovering it.

Comment 1

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: recent regression on Nightly.

mozregression points to https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=658c25a8405745e52f72f1437bd14a5339e4ca9d&tochange=dc033604b24d7f8e87af05265038cc9e9f301438
 -> bug 1263829

Comment 2

[Florian Quèze [:florian]]

Attachment: test.svg

Comment 3

[Florian Quèze [:florian]]

Attachment: test.xul

Steps to reproduce:
1. Download the 2 test files (test.svg and test.xul) into the same folder.
2. In about:config, set dom.allow_XUL_XBL_for_file to true.
3. Load file://.../test.xul

Expected result:
Animated (blinking/pulsing) camera icon.

Actual result:
Blank page until the mouse is moved above it. Once the mouse is moved, the animation starts working. Reloading the page (Cmd+R) makes the page look blank again.


Note: replacing the fill filter with fill="rgb(...)" directly on the SVG path element hides the bug.

Comment 4

[Jonathan Watt [:jwatt]]

It seems like dom.allow_XUL_XBL_for_file doesn't work any more. I get the same error message after loading the testcase locally as I do loading it from bmo:

  This page uses an unsupported technology that is no longer
  available by default in Firefox.

  Please contact the website owners to inform them of this problem

Can you create an HTML testcase, Florian?

Comment 5

[Florian Quèze [:florian]]

(In reply to Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) from comment #4)
> It seems like dom.allow_XUL_XBL_for_file doesn't work any more.

I used it to get the regression range using mozregression, so I can guarantee you that it works.

Comment 6

[Jonathan Watt [:jwatt]]

That doesn't help me at all...

Comment 7

[Florian Quèze [:florian]]

You may want to try with e10s disabled, as I'm not sure how about:config propagates preferences changes to the content process.

Comment 8

[Florian Quèze [:florian]]

Attachment: test.html

I managed to reproduce with an HTML testcase, but it isn't as reliable as the XUL one. To reproduce I click randomly once or twice on the blinking camera icon or outside of it (alternatively), and sometimes the animation stops (the icon disappears) after the click. Moving the mouse makes the animation restart.

Comment 9

[Florian Quèze [:florian]]

The behavior has changed again recently. Now the animation doesn't stop, but the opacity is either 0% or 100%, we don't get anything in between.

mozregression shows https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=067521ccd17907843e1db644ca0f02a1f5a6520a&tochange=eac0c056235ee1174fc092fe159d2d5710331fbd
-> bug 1283827

Comment 10

[Matt Woodrow (:mattwoodrow)]

I managed to reproduce this a few times in my Nightly, but can't do so in a local build.

Does this fix the problem?: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58a5372e62d

Comment 11

[Florian Quèze [:florian]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I managed to reproduce this a few times in my Nightly, but can't do so in a
> local build.
> 
> Does this fix the problem?:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58a5372e62d

No, it doesn't (I applied the patch locally to have results quickly without waiting for try).

Comment 12

[Marcia Knous [:marcia]]

Tracking 50+ for this SVG animation regression.

Comment 13

[Florian Quèze [:florian]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I managed to reproduce this a few times in my Nightly, but can't do so in a
> local build.

Are you still unable to reproduce? Bug 1206233 will make this even more visible.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Not yet, but I just added a new patch in bug 1287122 that is quite likely related. Does that patch (or more likely, both of them) fix this issue?

Comment 15

[Florian Quèze [:florian]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Not yet, but I just added a new patch in bug 1287122 that is quite likely
> related. Does that patch (or more likely, both of them) fix this issue?

attachment 8774197 [details] [diff] [review] from bug 1287122 fixes the regression from bug 1283827 that I observed in comment 9 here, and reverts the behavior to what was described in comment 1 to 8, ie. the regression from bug 1263829 remains.

When you said "both of them", I assume you meant attachment 8774197 [details] [diff] [review] + the patch in the try build at comment 10. Adding this second patch doesn't (visibly) change the behavior.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't skip building layers if we're not actually supposed to consider the opacity

I managed to reproduce it with the XUL testcase, needed a 3rd fix :)

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8775384 [details] [diff] [review]
Don't skip building layers if we're not actually supposed to consider the opacity

Review of attachment 8775384 [details] [diff] [review]:
-----------------------------------------------------------------

> I managed to reproduce it with the XUL testcase, needed a 3rd fix :)

\o/

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8775384 [details] [diff] [review]
Don't skip building layers if we're not actually supposed to consider the opacity

Review of attachment 8775384 [details] [diff] [review]:
-----------------------------------------------------------------

I can confirm that applying attachment 8774197 [details] [diff] [review] from bug 1287122 + this patch fixes the animation issue :-). Thanks!

Now it would be great if all these fixes could land before 50 merges to aurora.

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +632,5 @@
>         aParams.callerPaintsOpacity)) {
>      opacity = 1.0f;
>    }
> +  if (opacity == 0.0f) {
> +    return;

This is |return DrawResult::SUCCESS;| since bug 1258510.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd88c9c8068
Don't skip paint painting SVG effects due to 0 opacity if the opacity is being handled by a separate item. r=jwatt

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bd88c9c8068

Comment 21

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on Nightly 52.0a1 -20160920030429, Aurora 51.0a2 - 20160920004004 and Beta 50.0b1-20160920155715.