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.

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?

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

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

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.

I am going to fix this soon.

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 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 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 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 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 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/

Pushed by hikezoe@mozilla.com: 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

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?

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?

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

Good to hear. Thanks!

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

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.

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

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

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

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.

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