Rotated element cropped when animated

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: eric.jacobsson, Assigned: hiro)

Tracking

({regression})

49 Branch
mozilla53
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50- wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

Hi,

I have issues with firefox cropping my element when they are keyframe-animated and rotated the same time. Here's a link to a jsfiddle where you can see my issue. Sorry for bloated style... 

https://jsfiddle.net/eric_jacobsson/av1sh7s8/5/


Actual results:

Image gets cropped until I change the transform-style (doesn't matter to what)


Expected results:

It should not be cropped at any time
(Reporter)

Updated

11 months ago

Comment 1

11 months ago
[Tracking Requested - why for this release] : css animation is broken


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=58b8b93105204b9e3c23f75e5031804356f4e468&tochange=02145adb5fc079b6b1363df6a7a8cc4e1c70ba55

Regressed by:02145adb5fc0	Hiroyuki Ikezoe — Bug 1277991 - We don't need to check ShouldBlockAsyncTransformAnimations() when we just want to know whether the frame has transform animations or not. r=birtles
Blocks: 1277991
Status: UNCONFIRMED → NEW
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Component: Activity Streams: General → Layout
Ever confirmed: true
Flags: needinfo?(hiikezoe)
Keywords: regression
Product: Firefox → Core
Tracking 53+ due to css animation being broken.
tracking-firefox53: ? → +
(Assignee)

Comment 3

11 months ago
Interesting.  Before bug 1277991, we didn't set NS_FRAME_MAY_BE_TRANSFORMED for the rotate element because there are animations preventing the transform running on the compositor.  After bug 1277991, we set NS_FRAME_MAY_BE_TRANSFORMED correctly.  There seems to be a problem of crop region.
(Assignee)

Comment 4

11 months ago
Created attachment 8813881 [details]
A reduced test case

This bug seems to happen only on display:table.

I did not know that transform animations on table element does not run on the compositor.  That's the reason why this bug does not appear before landing bug 1277991.

Mats, do you know why cropping region is different from other element only if the element is display:table?
Flags: needinfo?(hiikezoe) → needinfo?(mats)

Comment 5

11 months ago
I know very little about transform animations so I don't know the answer
to your question.  Maybe you intended to ask Matt Woodrow though?
Flags: needinfo?(mats) → needinfo?(matt.woodrow)
Created attachment 8813887 [details]
reporter's original testcase
One thing that's interesting about tables is that they have two frames -- the table frame, which is the primary frame, and the table wrapper frame, which inherits certain properties from the table frame, and is the table frame's *parent*.
Track 51+/52+ as css animation is broken.
tracking-firefox51: ? → +
tracking-firefox52: ? → +
(Assignee)

Comment 9

11 months ago
dbaron, thank you for the hint.  Your comment was really helpful (as usual) to track this down. 
In the culprit change <https://hg.mozilla.org/mozilla-central/rev/02145adb5fc0>, I did set NS_FRAME_MAY_BE_TRANSFORMED flag to the primary frame of the table which is a frame that is not support transform[1].  We should check IsFrameOfType(eSupportsCSSTransforms) there.

[1] https://hg.mozilla.org/mozilla-central/file/05328d3102ef/layout/tables/nsTableFrame.h#l387

So, the real culprit is this change <https://hg.mozilla.org/mozilla-central/rev/df16848a29e356152b3374344a4e279a0dc05235>

Note that I will handle the problem that transform animations on table element don't run on the compositor in a separate bug (bug 1320608).
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

11 months ago
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 10

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b911d94897d0d3c7d831bda84dc77d0f079ed4
Comment hidden (mozreview-request)

Comment 12

11 months ago
mozreview-review
Comment on attachment 8814796 [details]
Bug 1319378 - Don't set NS_FRAME_MAY_BE_TRANSFORMED against nsIFrame that does not support transforms.

https://reviewboard.mozilla.org/r/95890/#review96218

r=dbaron if you've tested that the reftest fails without the patch
Attachment #8814796 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

11 months ago
Thanks!

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #12)
> Comment on attachment 8814796 [details]
> Bug 1319378 - Don't set NS_FRAME_MAY_BE_TRANSFORMED against nsIFrame that
> does not support transforms.
> 
> https://reviewboard.mozilla.org/r/95890/#review96218
> 
> r=dbaron if you've tested that the reftest fails without the patch

Sure, I did. (but forgot to mention..)

Comment 14

11 months ago
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/8611e9d0bdaa
Don't set NS_FRAME_MAY_BE_TRANSFORMED against nsIFrame that does not support transforms. r=dbaron
Backed out in https://hg.mozilla.org/integration/autoland/rev/9d051cf1d568 for Win8 failures, mostly but not quite entirely e10s, in table-overflowed-by-animation.html like https://treeherder.mozilla.org/logviewer.html#?job_id=7353040&repo=autoland (or https://treeherder.mozilla.org/logviewer.html#?job_id=7338653&repo=autoland for the more rare non-e10s).
(Assignee)

Comment 16

11 months ago
Gosh!  The reftest needs fuzzy-if.  I will collect how many difference and pixels there are and re-land with the numbers.
(Assignee)

Comment 17

11 months ago
The failure seems to happen only on Win8 x64.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2192ac0ef62b247d2d07731afd5e2aff75288b3a

And the difference of all of failure cases are the same, "max difference: 1, number of differing pixels: 5".
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d378fe43e8271ab8af01bad146c0525817d024b2
Comment hidden (mozreview-request)

Comment 19

11 months ago
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/c91e2710718e
Don't set NS_FRAME_MAY_BE_TRANSFORMED against nsIFrame that does not support transforms. r=dbaron

Comment 20

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c91e2710718e
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 21

11 months ago
Since this is not a new regression in 50, I'd rather this ride the 51 train to release.
status-firefox50: affected → wontfix
tracking-firefox50: ? → -

Comment 22

11 months ago
Hi Hiro, could you please nominate the fix for uplift to Beta51 and Aurora52? Thanks!
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 23

11 months ago
Comment on attachment 8814796 [details]
Bug 1319378 - Don't set NS_FRAME_MAY_BE_TRANSFORMED against nsIFrame that does not support transforms.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1277991
[User impact if declined]: Seeing cut-out transform animation on table element.
[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]:  
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]:  No.
[Why is the change risky/not risky?]: This patch just stop setting an flag which represents 'transform' against an nsIFrame that does not allowed 'transform'. 
[String changes made/needed]: None.
Attachment #8814796 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

11 months ago
The test case in the patch needs fuzzy-if on Linux in beta branch, since, I guess, it's caused by the difference that Linux still uses cairo backend in the beta branch.  I just push a try with an additional fuzzy-if to see if it works.

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

I am going to request to uplift to beta after the try.
Thanks!
(Assignee)

Comment 25

11 months ago
The try looks good.
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 26

11 months ago
Created attachment 8815983 [details]
Patch for beta branch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1277991
[User impact if declined]: Seeing cut-out transform animation on table element.
[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]:  
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]:  No.
[Why is the change risky/not risky?]: This patch just stop setting an flag which represents 'transform' against an nsIFrame that does not allowed 'transform'. 
[String changes made/needed]: None.
Attachment #8815983 - Flags: review+
Attachment #8815983 - Flags: approval-mozilla-beta?
Comment on attachment 8815983 [details]
Patch for beta branch

Fix a regression related to cut-out transform animation on table element. Beta51+ an Aurora52+. Should be in 51 beta 6.
Attachment #8815983 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8814796 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/c6a5f546cd7105ee509405f663cb61c5fd9e8ef1
status-firefox51: affected → fixed

Comment 29

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/379652ddad1f
status-firefox52: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.