Closed Bug 1451724 Opened 6 years ago Closed 6 years ago

[stylo] CSS transforms margin-bottom and translate incorrect behavior

Categories

(Core :: CSS Parsing and Computation, defect)

58 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: ggriffiths, Assigned: manishearth)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image firefox_issue.gif
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

See the attached GIF and Code below.
- Applying a transform: translate on a child element, with a transition on the transform and applying a margin to the parent element, that also has transition on "margin-bottom", causes strange animation behavior in 60.0b9. (See in animated gif).

Important things to note:
prior to translate the child transform is: transform: translate(450px);
after translate the child transform is: transform: translate(0px,0px);

Note the omitting of the "Y-axis" transform in the first, and its presence in the second.
If either:
you add ",0px" to the first translate: issue is gone.
you remove ",0px" (y-axis) from the second translate Y issue is gone.
 
According to the spec: "specifies a 2D translation by the vector [tx, ty], where tx is the first translation-value parameter and ty is the optional second translation-value parameter. If <ty> is not provided, ty has zero as a value."

if i set:
transform: translate(0,0);
Without the "px", issue is there still.


Code to reproduce in the following HTML file:
--- code start
<!DOCTYPE html>
<html>
<style>
     .margin-anim {
        transition: margin-bottom 0.8s ease-in-out;
    }

    .rectangle {
        background:red;
        width: 356px;
        height: 48px;
        /* translate only specifying x */
        transform: translate(450px);
        transition: transform 300ms;
    }

    .move-left {
        transform: translate(0px,0px);
    }

    .animate-margin-bottom {
        margin-bottom: 8px;
    }

</style>

<body>
    <div class="margin-anim">
        <div class="rectangle"></div>
    </div>  
    <script>
        function transform() {
            var rectangle = document.querySelector('.rectangle');

            setTimeout(()=>{
                // add class that defines margin on parent which is animated
                rectangle.parentElement.classList.add('animate-margin-bottom');
                // transform child
                rectangle.classList.add('move-left'); 
            },300);
        }

        function reset() {
            var rectangle = document.querySelector('.rectangle');
            rectangle.parentElement.classList.remove('animate-margin-bottom');
            rectangle.classList.remove('move-left'); 
        }
    </script>
    <button id="transform" onclick="transform()">Transform</button>
    <button id="transform" onclick="reset()">Reset</button>
</body>
</html>
--- code end





Actual results:

The animation goes in a "V" shape instead of side to side.

I've tested in FF 52 ESR - issue is not present
tested in Latest Chrome - issue is not present
test in Edge - issue not present


Expected results:

Animation should function from right to left.
#1 regression,
The rectangle is moved from right to left. But no transition animation:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6bc39f03ea571fb109ae603b0c8f69778ec14384&tochange=92ff0c88e94d176ff9615abe61322413b7f38e55

#1 Regressed by: 92ff0c88e94d	Manish Goregaokar — servo: Merge #18750 - Make transforms generic (from Manishearth:transform-generic); r=emilio,xidorn


#2 regression,
The animation goes in a "V" shape instead of side to side:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=251fb7b394132abe8947b526def1318a59fd65e3&tochange=ba242fe9760e1744c2bdb463bbc679a741254cd5

#2 Regressed by: ba242fe9760e	Manish Goregaokar — servo: Merge #19604 - stylo: Correctly handle interpolation where optional second argument for translate(), skew(), scale() exists in one but not the other (from Manishearth:animate-second); r=emilio
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Keywords: regression
Summary: CSS transforms margin-bottom and translate incorrect behavior → [stylo] CSS transforms margin-bottom and translate incorrect behavior
Version: 60 Branch → 58 Branch
btw, no bugzilla #, It cannot block.... :(
@Manish Goregaokar,
Your bunch of patch seems to cause the problem. Can you look into this?
Flags: needinfo?(manishearth)
I have to head out, but this is likely fixed by

diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs
index bb6931cdde..342f0cb839 100644
--- a/components/style/properties/helpers/animated_properties.mako.rs
+++ b/components/style/properties/helpers/animated_properties.mako.rs
@@ -1204,7 +1204,8 @@ impl Animate for ComputedTransformOperation {
             ) => {
                 Ok(TransformOperation::Translate(
                     fx.animate(tx, procedure)?,
-                    Some(fy.unwrap_or(*fx).animate(&ty.unwrap_or(*tx), procedure)?)
+                    Some(fy.unwrap_or(LengthOrPercentage::zero())
+                           .animate(&ty.unwrap_or(LengthOrPercentage::zero()), procedure)?)
                 ))
             },
             (
I don't quite see how the margin-bottom is relevant here, but I'll have a closer look
Flags: needinfo?(manishearth)
(In reply to Manish Goregaokar [:manishearth] from comment #5)
> I don't quite see how the margin-bottom is relevant here, but I'll have a
> closer look

Hiro may know, but I suspect the parent margin animation prevents the child animation from running on the compositor, and we use different code paths there... Or something like that.
Flags: needinfo?(hikezoe)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> (In reply to Manish Goregaokar [:manishearth] from comment #5)
> > I don't quite see how the margin-bottom is relevant here, but I'll have a
> > closer look
> 
> Hiro may know, but I suspect the parent margin animation prevents the child
> animation from running on the compositor, and we use different code paths
> there... Or something like that.

That's quite right.  We don't run transform animations on the compositor if there is other animation that affect geometric change (e.g. width, height, margin of course) and the animation started at the same time when the transform animations started.
Flags: needinfo?(hikezoe)
Assignee: nobody → manishearth
Comment on attachment 8965710 [details]
Bug 1451724: Add reftest ;

https://reviewboard.mozilla.org/r/234238/#review240190

Looks good, thanks :)
Attachment #8965710 - Flags: review?(emilio) → review+
Comment on attachment 8965711 [details]
Bug 1451724: Add reftest ;

https://reviewboard.mozilla.org/r/234240/#review240196

There's no reason not to make this a WPT test, right? If so, let's do that instead.

::: layout/reftests/bugs/1451724-1-ref.html:15
(Diff revision 1)
> +}
> +</style>
> +</head>
> +<body>
> +<div class=box id=box></div>
> +<script type="text/javascript">

Same remark as in the other file. Also, please make this a WPT test instead.

::: layout/reftests/bugs/1451724-1.html:13
(Diff revision 1)
> +    height: 200px;
> +}
> +</style>
> +</head>
> +<body>
> +<div class=box id=box></div>

no need for both id and class, just use one.

::: layout/reftests/bugs/1451724-1.html:14
(Diff revision 1)
> +}
> +</style>
> +</head>
> +<body>
> +<div class=box id=box></div>
> +<script type="text/javascript">

no need for `type=`.

::: layout/reftests/bugs/1451724-1.html:15
(Diff revision 1)
> +</style>
> +</head>
> +<body>
> +<div class=box id=box></div>
> +<script type="text/javascript">
> +    let anim = document.getElementById('box').animate([

If you use id, you can just do `box.animate(..)`.
Attachment #8965711 - Flags: review?(emilio)
fix landed as https://github.com/servo/servo/pull/20575, will push test soon
Attachment #8965711 - Attachment is obsolete: true
This merged.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Cameron McCormack (:heycam) from comment #14)
> This merged.

For posterity:
https://hg.mozilla.org/mozilla-central/rev/03d22e687292

Does this need the test to be pushed still? Also, does this need a Beta approval request?
Flags: needinfo?(manishearth)
Flags: in-testsuite?
Target Milestone: --- → mozilla61
Oh, I didn't realize emilio had already r+d this and was waiting for review. Landing
Flags: needinfo?(manishearth)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 06862c7331a4cd3a304a796196f11ac28683033a -d 079e73b36f8b: rebasing 459003:06862c7331a4 "Bug 1451724: Add reftest ; r=emilio" (tip)
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8965710 [details]
Bug 1451724: Add reftest ;

https://reviewboard.mozilla.org/r/234238/#review243234

I had more certainly not reviewed the test, fwiw. But also not received a review request either, looks like mozreview got confused because it used two be two different patches.

r=me for the test with the nits, and ensuring the WPT lint passes (should run on try).

::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y-ref.html:2
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>

Needs a title as well.

::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:3
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title></title>

Needs a title, author, and spec link.

::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:12
(Diff revision 2)
> +    background:red;
> +    width: 200px;
> +    height: 200px;
> +}
> +</style>
> +<body>

no need for body tags.

::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:14
(Diff revision 2)
> +    height: 200px;
> +}
> +</style>
> +<body>
> +<div id=box></div>
> +<script type="text/javascript">

No need for type=
So i just tested in 60.0b13 (beta channel) and the issue appears to have been fixed. :)

I'm a little confused by:
Regression bug RESOLVED as FIXED for Firefox 61 found in Firefox 60

Could someone clarify whether or not this fix will land in the Firefox 60 ESR/ Firefox 60.1 ESR?

First time filing as bug for FF and struggling to understand how the ESR fits into the release cycle, thanks
RESOLVED FIXED refers to when a patch lands on our development branch (mozilla-central), which is currently tracking Firefox 61. I can confirm that the testcase from the initial report indeed works fine on a current Nightly build.

It's a bit of a surprise if 60.0b13 is working correctly now for you since the patch from this bug hasn't been backported to Fx60 yet. FWIW, I can still reproduce the bug when testing locally with 60.0b13 (and as I mentioned before, I was able to verify that Nightly builds are fixed).

To answer your question RE: ESR60, anything that's fixed in Fx60 will be fixed in ESR60 also. They won't diverge until very close to release. So if this patch gets backported to 60, it'll be in the ESR as well.
My Bad, bug is still there, i forgot that i put a work around in our code. D'oh
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10530 for changes under testing/web-platform/tests
Probably too late to backport to Fx60 at this point given that the first RC build was just created today...
Flags: in-testsuite? → in-testsuite+
Do we want this on esr60 or should we wontfix for that branch?
Flags: needinfo?(manishearth)
I think we should backport but not sure
Flags: needinfo?(manishearth) → needinfo?(emilio)
Comment on attachment 8965710 [details]
Bug 1451724: Add reftest ;

I think if it can be in 60 it'd be nice.

Note that the approval request is for https://hg.mozilla.org/mozilla-central/rev/03d22e687292, but mozreview got confused and forgot about that revision afterwards. :(

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: trivial transform animation fix.
User impact if declined: Potential glitches.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): not really risky.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(emilio)
Comment on attachment 8965710 [details]
Bug 1451724: Add reftest ;

stylo fix, approved for 60.1esr
Attachment #8965710 - Flags: approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.