Alice0775 White Reporter
	Description • 7 years ago

[Tracking Requested - why for this release]:

Reproducible: always

Steps To reproduce:
1. Open attached

	Alice0775 White Reporter
	Comment 2 • 7 years ago

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c4ca88d519f1f4a1ea4938cbba047e89d3ff3ec&tochange=45fde181a497a187d01d5412f5b72897c7520517
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=958d2a5d10091401fd5e900e8e063d21940c137e&tochange=7f894f791cdf170d788507d0eff30024ce699523

Regressed by: Bug 1359709 , Bug 1361970

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 7 years ago

This page also shows a demo that prior to this bug correctly transformed the SVG, http://msuja.ws/svg.html

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 7 years ago

Hey Jet, can you help us get this prioritized? It's holding up some photon animation work right now (see dep list).

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 8 • 7 years ago

I can't reproduce this on OSX (I suspect it's due to partial presents, which we only use on Windows), but I think this will be the same as bug 1373335, which I will have a fix up for today.

	Alice0775 White Reporter
	Comment 10 • 7 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> I can't reproduce this on OSX (I suspect it's due to partial presents, which
> we only use on Windows), but I think this will be the same as bug 1373335,
> which I will have a fix up for today.

If not reproduce, try mouse hover over image and wait for a minutes.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 12 • 7 years ago

This code was restricting the visible region of the async-animation ContainerLayer to what was currently visible, not the prerendered area of the children that we want.

Previously, PostProcessLayers recomputed visible regions for ContainerLayers entirely, so this incorrect value got overwritten.

Now we don't bother updating ContainerLayers without an intermediate surface, so the broken visible region remained.

We don't actually use this region to render, but LayerTreeInvalidation was using it to compute the partial present region, which is why this only failed on Windows.

Apologies, couldn't get mozreview to work for this one.

	Marcia Knous [:marcia]
	Comment 13 • 7 years ago

Tracking 56+ for this UI regression.

	Chris Peterson [:cpeterson]
	Comment 14 • 7 years ago

I think I see this same bug as (Windows-only?) visual glitches in Treeherder's job panel opening animation.

	Markus Stange [:mstange]
	Comment 15 • 7 years ago

Comment on attachment 8879791 [details] [diff] [review]
Use the visible rect for children when restricting the items draw rect, since that includes the prerendering region if applicable.

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

Would be really nice to have a test here.

::: layout/painting/FrameLayerBuilder.cpp
@@ +4123,5 @@
>      ((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds, bounds);
>  #endif
>  
>      nsIntRect itemVisibleRect = itemDrawRect;
>      // We haven't computed visibility at this point, so item->GetVisibleRect()

I think this comment needs to be adjust in some way, because the item->GetVisibleRect() call it refers to now no longer exists.

	Daniel Holbert [:dholbert]
	Comment 16 • 7 years ago

I tested this patch out to see if it helps with a similar bug, but in doing so, I'm noticing that it doesn't actually help (on my system) with the testcases on this bug here.

("testcase 2" in particular -- I see serious tearing/artifacts during both phases of the button-triggered animation there.)

I'm using a mozilla-central build from today, with this patch layered on top.

	Daniel Holbert [:dholbert]
	Comment 17 • 7 years ago

(this is a local opt build (ac_add_options --enable-debug-symbols --enable-optimize), on 64-bit Ubuntu Linux 17.04)

	Daniel Holbert [:dholbert]
	Comment 18 • 7 years ago

Here's a screencast of what I see with the latest patch applied to today's mozilla-central, in an opt build.  Unlike with stock Nightly, I don't see much tearing/smearing (other than at the very bottom) during the first half of the animation, but I do in the second half -- around 0:09 seconds in the screencast.

(At mstange's suggestion, I've also applied bug 1373335's patch for this screencast, too -- that patch doesn't seem to impact behavior one way or the other here.)

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 19 • 7 years ago

Using the visible rect for children didn't make sense, we really do want something in the coordinate space *outside* the transform and using the inner visible rect doesn't give the right results.

We don't have anything that tells us what might be visible asynchronously in post-transform coords, so lets just skip this optimization (like we do a few lines earlier).

	Markus Stange [:mstange]
	Comment 20 • 7 years ago

Comment on attachment 8880176 [details] [diff] [review]
Don't intersect with the visible region for async animations

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

I hope it's not going to trigger the mAccumulatedChildBounds assertion.

	Daniel Holbert [:dholbert]
	Comment 21 • 7 years ago

Good news! I've confirmed locally that this just-posted patch fixes both of this bug's testcases for me, as well as my WhatsApp web use-case from bug 1375269.

(I'm using a build that *also* has bug 1373335's patch applied, if it matters. I know that other patch, on its own, wasn't sufficient to fix the testcases here [and it probably has no effect on them anyway].)

RE an automated test -- the "testcase2" here seems like it'd be fairly easy to convert into a MozReftestInvalidate reftest, I'd think!

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 23 • 7 years ago

Unfortunately the reftest harness doesn't have visibility into async animations, or the invalid rect generated for async animation paints (which was what was wrong here).

We'd need a way to override the animation timeline from a reftest, and also have MozAfterPaint events be delivered for async paints (possibly only under certain conditions?).

	Pulsebot
	Comment 24 • 7 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9618a52d6d17
Don't intersect with the visible rect for async animations, since we want to keep the prerendered area as visible too. r=mstange

	Carsten Book [:Tomcat]
	Comment 26 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9618a52d6d17