1292441 - Write some reftest to check background-position animation's layerization

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example reftest

background-position-x/y animation creates an active layer, but even if the animation is overridden by an important style, an active layer is still created.

Attaching file is a reftest to confirm it, IIUC layerization, this test should pass, but actually it leads to a crash. 

The problem is that ActiveLayerTracker::IsBackgroundPositionAnimated() uses (indirectly) nsLayoutUtils::HasCurrentAnimationOfProperty()[1].   nsLayoutUtils::HasCurrentAnimationOfProperty() does check whether animated property is overridden by important styles only if the property is 'transform' or 'opacity'.

[1] http://hg.mozilla.org/mozilla-central/file/0ba72e8027cf/layout/base/ActiveLayerTracker.cpp#l449

In bug 1278136 nsLayoutUtils::HasCurrentAnimationOfProperty() might be changed, but the new one will not check the important style either.

If we really don't want to create an active layer in such cases, we can provide a utility function which checks whether the property is overridden or not for other properties.

Markus?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I could not find the needinfo field while filing this bug.

Comment 2

[Markus Stange [:mstange]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> background-position-x/y animation creates an active layer, but even if the
> animation is overridden by an important style, an active layer is still
> created.

Okay. That's not perfect, but does it hurt us on real pages?

> Attaching file is a reftest to confirm it, IIUC layerization, this test
> should pass, but actually it leads to a crash. 

The reftest-opaque-layer code checks whether the element's display items have been assigned to an opaque PaintedLayer. In this case, one of the element's display items (the only one actually) created its own layer and was not assigned to a PaintedLayer. It's possible that the code isn't handling that case properly and crashing. I suppose we could make it handle the case where there's really just one layer for the element.

> If we really don't want to create an active layer in such cases, we can
> provide a utility function which checks whether the property is overridden
> or not for other properties.
> 
> Markus?

Is it worth the effort? Why did you run into this issue?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I could not find the needinfo field while filing this bug.

It's under "Set bug flags". I can never find it either.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Thank you, Markus!  Then I can replace nsLayoutUtils::HasCurrentAnimationOfProperty() in ActiveLayerTracker::IsStyleAnimated().

Also, I am going to re-use this bug to write reftests which check current behavior of background-position-x/y animations.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1292441 - Part 1: Avoid crashes when 'reftest-opaque-layer' is specified against an element having an display item which is not assigned to a PaintedLayer.

Review commit: https://reviewboard.mozilla.org/r/69890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69890/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1292441 - Part 2: Reftest for backround-position-x animation.

Review commit: https://reviewboard.mozilla.org/r/69892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69892/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8778734 [details]
Bug 1292441 - Part 1: Avoid crashes when 'reftest-opaque-layer' is specified against an element having an display item which is not assigned to a PaintedLayer.

https://reviewboard.mozilla.org/r/69890/#review67010

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8778735 [details]
Bug 1292441 - Part 2: Reftest for backround-position-x animation.

https://reviewboard.mozilla.org/r/69892/#review67014

::: layout/reftests/css-animations/background-position-important.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<title>background-position-x animation overridden by important style</title>
> +<style>
> +@keyframes holdBackgroundPosition {
> +  from,to { background-position-x: 50%; }

So you're overriding the value 50% with the value 50%. Wouldn't it be better to choose a different value in the (overridden) animation, so that it's clear that we want the override to work?

::: layout/reftests/css-animations/background-position-important.html:13
(Diff revision 1)
> +#test {
> +  height: 100px;
> +  width: 100px;
> +  background-repeat: no-repeat;
> +  background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAAZCAMAAADzN3VRAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAAZQTFRF/wAAAAAAQaMSAwAAABJJREFUeNpiYBgFo2AwAIAAAwACigABtnCV2AAAAABJRU5ErkJggg==); /* a 25x25 px red box */
> +  background-position-x: 50% ! important;

I see "!important" a lot more than I see "! important".

::: layout/reftests/css-animations/reftest.list:32
(Diff revision 1)
> +fails == background-position-running.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position should create an active layer.
> +fails == background-position-important.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position overridden by imporatant style also creates an active layer.

I think the "should" here is a little misleading - yes, it fails at the moment, but that's not what we would want it to do in a perfect world. How about this:
\# This test fails the reftest-opaque-layer check since animating background-position currently creates an active layer, and reftest-opaque-layer only handles items assigned to PaintedLayers.

I've added the part about PaintedLayers because the active layer is in fact opaque, but reftest-opaque-layer does not check that.

::: layout/reftests/css-animations/reftest.list:33
(Diff revision 1)
>  fails == stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html # bug 1278136 and bug 1279403
>  fails == stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html # bug 1278136 and bug 1279403
> +== background-position-in-delay.html background-position-ref.html
> +== background-position-after-finish.html background-position-ref.html
> +fails == background-position-running.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position should create an active layer.
> +fails == background-position-important.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position overridden by imporatant style also creates an active layer.

How about:
# This test fails the reftest-opaque-layer check since animating background-position overridden by a non-animated !important style also creates an active layer, and reftest-opaque-layer only handles items that are assigned to PaintedLayers.

(also fixing the "imporatant" typo)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thank you, Markus! I am really really appreciated for you review.

(In reply to Markus Stange [:mstange] from comment #7)
> ::: layout/reftests/css-animations/background-position-important.html:6
> (Diff revision 1)
> > +<!DOCTYPE html>
> > +<html>
> > +<title>background-position-x animation overridden by important style</title>
> > +<style>
> > +@keyframes holdBackgroundPosition {
> > +  from,to { background-position-x: 50%; }
> 
> So you're overriding the value 50% with the value 50%. Wouldn't it be better
> to choose a different value in the (overridden) animation, so that it's
> clear that we want the override to work?

This is intentional because I want to know, I think we should know if some day we don't create an active layer for such animations which are overridden by important style.   If we use the different value there, this test will fail as before once we don't create an active layer for the animations any more.

> ::: layout/reftests/css-animations/reftest.list:32
> (Diff revision 1)
> > +fails == background-position-running.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position should create an active layer.
> > +fails == background-position-important.html background-position-ref.html # This test should fail on reftest-opaque-layer check since animating background-position overridden by imporatant style also creates an active layer.
> 
> I think the "should" here is a little misleading - yes, it fails at the
> moment, but that's not what we would want it to do in a perfect world. How
> about this:
> \# This test fails the reftest-opaque-layer check since animating
> background-position currently creates an active layer, and
> reftest-opaque-layer only handles items assigned to PaintedLayers.
> 
> I've added the part about PaintedLayers because the active layer is in fact
> opaque, but reftest-opaque-layer does not check that.

Thank you for telling me about it.  I am really unfamiliar with our layers.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8778735 [details]
Bug 1292441 - Part 2: Reftest for backround-position-x animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69892/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00ae10ae77aa
Part 1: Avoid crashes when 'reftest-opaque-layer' is specified against an element having an display item which is not assigned to a PaintedLayer. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8670b9482db9
Part 2: Reftest for backround-position-x animation. r=mstange

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/00ae10ae77aa
https://hg.mozilla.org/mozilla-central/rev/8670b9482db9