1385013 - CSS animate background color not working correctly

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Joshua Nixon]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Using CSS animation on body color is not working as expected. See examples:
Test 1 - animation works as expected - body element contains only a single character (nonbreaking space)
Test 2 - animation does NOT work - body element is empty

Note that inclusion of elements such as images, divs, etc. in test file SOMETIMES make the page work - empty div does not seem to work, empty img element or linked image can work, but img element with base64 encoded image does NOT work.

All examples tested work as expected in Safari, but those with empty body elements do not work in Firefox [Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0 ID:20170628075643 CSet: 90f18f9c15f7c71c755e387cfc193974fcf8b29c]


Actual results:

Without any text in the document, animation does not occur unless mouse is moving - if mouse stops, animation stops. Additionally, during mouse movement, animation is not smooth.

When there is text in the document, the animation works as expected - but ONLY if there is text present (even a nonbreaking space will allow animation, as in example HTML page)


Expected results:

Animation on body background should not depend on page content - if body element exists, animation should occur.

Comment 1

[Joshua Nixon]

Attachment: Test 1 - nbsp in body - works

Comment 2

[Joshua Nixon]

Attachment: Test 2 - empty body - does NOT work unless mouse is moving

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5613020111ba45dda63615dfa5a791f7ad1a8e4a&tochange=fe7766352e3eb84c9da204d866e8a25d814a09d8

Regressed by:  Bug 1166500

@Hiroyuki Ikezoe (:hiro),
Your patch seems to cause the problems.
Could you look this?

Comment 4

[Alice0775 White]

Indeed, setting dom.animations.offscreen-throttling = false fixes the problem.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you Alice for narrow it down.  My wild guess is that IsScrolledOutOfView() seems to fail.  I will check it later.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

The height of the empty body in question is zero, so transformedRect.Intersects() check in IsFrameScrolledOutOfView() fails. We have to figure out how to avoid the situation somehow.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I am going to fix this soon.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3324c413d55b40fee3b37771353d7c72f0ced3ab

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

nsRect.Intersecs returns true if the given rect is empty, so it misleads
us that the target frame is out of the parent frame in such cases.  We
should check all vertexes respectively for the empty rect instead.  The
conditions which is used for the check is a part of the nagation of
BaseRect::Intersects().

The reftest causes timeout without this fix since the animation never updates
after the initial paint.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

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

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195650

::: commit-message-c6a26:1
(Diff revision 1)
> +Bug 1385013 - Check all vertexes for the taget frame are outside of the parent frame if the target frame is empty. r?birtles

s/taget/target/

::: commit-message-c6a26:3
(Diff revision 1)
> +Bug 1385013 - Check all vertexes for the taget frame are outside of the parent frame if the target frame is empty. r?birtles
> +
> +nsRect.Intersecs returns true if the given rect is empty, so it misleads

s/Intersecs/Intersects/

::: commit-message-c6a26:6
(Diff revision 1)
> +Bug 1385013 - Check all vertexes for the taget frame are outside of the parent frame if the target frame is empty. r?birtles
> +
> +nsRect.Intersecs returns true if the given rect is empty, so it misleads
> +us that the target frame is out of the parent frame in such cases.  We
> +should check all vertexes respectively for the empty rect instead.  The
> +conditions which is used for the check is a part of the nagation of

s/nagation/negation/

::: layout/generic/nsFrame.cpp:10526
(Diff revision 1)
> +    // in case of the transformed rect is empty, we need to check all vertexes
> +    // are out of the parent view.

I would probably say something like,

"If the transformed rect is empty it represents a single point that we should check is outside the scrollable rect."

Also, would 

  !scrollableRect.Contains(transformedRect.x, transformedRect.y)

work here?

::: layout/reftests/css-animations/animation-on-empty-height-frame.html:16
(Diff revision 1)
> +<script>
> +window.addEventListener('load', () => {
> +  const body = document.querySelector('body');
> +  body.style.animation = 'anim 100s step-end reverse';
> +  body.addEventListener('animationstart', () => {
> +    // This MozAfterPaint event corresponds to the white background paint.

I found this test a little hard to follow so perhaps we could add an extra parenthetical as a second/third line to the comment:

  // (The animation will initially paint the background red since it is playing
  // a step-end animation in reverse.)

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195654

::: commit-message-c6a26:3
(Diff revision 2)
> +nsRect.Intersects returns false if the given rect is empty, so checking
> +!nsRect.Intersects() misleads that the target frame is out of the parent frame
> +in such cases.  We should check all vertexes respectively for the empty rect
> +instead.  The conditions which is used for the check is a part of the negation
> +of BaseRect::Intersects().

How about:

  nsRect.Intersects returns false if either of the rectangles are empty,
  so if we check !transformedRect.Intersects(scrollableRect) and
  transformedRect is empty, we will end up returning true from
  IsFrameScrolledOutOfView even though the point represented by the empty
  transformedRect might be inside the scrollableRect.

Some explanation of why transformedRect is empty would probably also help. What does it mean for it to be empty?

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195656

Clearing review request for now since I think we should add a code comment saying why transformedRect can be empty and what that means.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190132/diff/3-4/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195668

::: commit-message-c6a26:3
(Diff revision 4)
> +We create empty rectangle (zero-height or zero-width) frame for element which
> +has no content inside it, e.g.  '<p></p>'.  And nsRect.Intersects returns false

Though I haven't check that, but I guess if writing-mode is vertical, then '<p></p>' will be zero-width.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195650

> I would probably say something like,
> 
> "If the transformed rect is empty it represents a single point that we should check is outside the scrollable rect."
> 
> Also, would 
> 
>   !scrollableRect.Contains(transformedRect.x, transformedRect.y)
> 
> work here?

The 'empty' for rect means zero-width or zero-height, so we can't use Contains(x, y) or Contains(Point) handy there.

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

https://reviewboard.mozilla.org/r/190132/#review195672

::: layout/generic/nsFrame.cpp:10526
(Diff revision 4)
> +    // If the transformed rect is empty it represents a line that we should
> +    // check is outside the the scrollable rect.

s/represents a line or a point/

::: layout/reftests/css-animations/animation-on-empty-height-frame.html:17
(Diff revision 4)
> +window.addEventListener('load', () => {
> +  const body = document.querySelector('body');
> +  body.style.animation = 'anim 100s step-end reverse';
> +  body.addEventListener('animationstart', () => {
> +    // This MozAfterPaint event corresponds to the white background paint.
> +    // (The animation will initiall paint the background red since it is playing

s/initiall/initially/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190132/diff/4-5/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69e2ab081d3d
Check all vertexes for the target frame are outside of the parent frame if the target frame is empty. r=birtles

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/69e2ab081d3d

Comment 22

[Boris Chiou [:boris]]

Hi hiro,

If we want to uplift bug 1303235, should we uplift this bug first because we might have some test failures if we land bug 1303235 without the patch in this bug?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Yes, right. This bug is definitely needed for that bug.  Apart from bug 1303235, we can/should uplift the patch here in this bug since this bug affects 57 for android.  Anyway it's not clear to me why Ryan marked this bug wontfix.  Ryan?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Didn't look like it was a high-priority issue for 57 at this point. If I'm wrong, feel free to nominate it for approval :)

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Good to hear. Thanks!

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1166500
[User impact if declined]: User won't see some animations
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]:  It does an additional simple check for the case that we'd missed in the original bug.
[String changes made/needed]: None

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

This is not a new regression, I don't believe this is a must fix, let this fix ride the 58 train. If any top sites are impacted, please let me know and we can reconsider.

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

RyanVM pinged me and told me that the fix from this one will help Bug 1405744. Jet, can you please confirm? If yes, then I'd be happy to uplift to 57. Please let me know.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Right, this bug is prerequisite for bug 1303235, and now the reporter of bug 1405744 commented that bug 1303235 fixed bug 1405744.

Comment 30

[Jet Villegas (inactive)]

(In reply to Ritu Kothari (:ritu) from comment #28)
> RyanVM pinged me and told me that the fix from this one will help Bug
> 1405744. Jet, can you please confirm? If yes, then I'd be happy to uplift to
> 57. Please let me know.

Yes, please expedite this uplift. Thx!

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Bugzilla does not allow to re-request for uplift against the patch which were declined once,  so ni? to Ritu.

Thank you!

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919211 [details]
Bug 1385013 - Check all vertexes for the target frame are outside of the parent frame if the target frame is empty.

Let's take this one.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd097e21e8d6

Comment 34

[Andrei Vaida [:avaida]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Hiroyuki Ikezoe's assessment on manual testing needs and the fact that this fix has automated coverage.