1391145 - stylo: translate function is shown as translate3d on animation inspector

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Manish left an important comment [1] in from_computed_value for Transform.

 // XXXManishearth we lose information here; perhaps we should try to                        
 // recover the original function? Not sure if this can be observed

Yeah, we can observe it on animation inspector.

https://hg.mozilla.org/mozilla-central/file/63ca686c3f1e/servo/components/style/properties/longhand/box.mako.rs#l1506

Comment 1

[Brian Birtles (:birtles)]

Yes, I noticed that this morning and couldn't remember if it was always like that. We should certainly try to fix that. (I'm surprised we didn't have any tests that cover that.)

Comment 2

[Brian Birtles (:birtles)]

This would be nice to fix in 57 if someone has a spare moment.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brian Birtles (:birtles) from comment #2)
> This would be nice to fix in 57 if someone has a spare moment.

This means P3 rather than P4, I guess.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1391145 - stylo: Preserve the variant of translate() values in computed ;

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

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1391145 - stylo: Preserve the variant of scale() values in computed transforms ;

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

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1391145 - stylo: Preserve the variant of rotate() values in computed transforms;

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

Comment 7

[Manish Goregaokar [:manishearth]]

I added code for handling translate/rotate/scale.

This does not handle matrix(), which is a tad more complicated. Do we care about that case?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Great! Thank you!

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> I added code for handling translate/rotate/scale.
> 
> This does not handle matrix(), which is a tad more complicated. Do we care
> about that case?

It would be nice to have the case, but it's more uncommon lower priority I think.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

All of patches look great! But each patch misses an important case, e.g. translate(100px, 100px). Gecko shows translate(100px, 100px) in keyframe as it is instead of 'translate3d(100px, 100px, 0px)' in devtools.

Comment 10

[Manish Goregaokar [:manishearth]]

How important is this?


We're coming close to the point where it's just better to have computed value as specified here, which is what gecko does anyway.

But I don't want to affect pure Servo here.

It occurs to me that if we're doing computed as specified; nox's generic tricks might help. But it's a nontrivial refactor; there's a lot of code around this that would have to be changed.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Yeah, fair enough.  I think 'translate' case is more important than others, but have no evidence.  Let's land these patches, and postpone remaining cases.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906858 [details]
Bug 1391145 - stylo: Preserve the variant of translate() values in computed ;

https://reviewboard.mozilla.org/r/178580/#review183662

::: servo/components/style/properties/longhand/box.mako.rs:1274
(Diff revision 1)
> -                            computed::length::LengthOrPercentage::zero(),
> -                            computed::length::Length::new(0)));
>                      }
>                      SpecifiedOperation::TranslateY(ref ty) => {
>                          let ty = ty.to_computed_value(context);
> -                        result.push(computed_value::ComputedOperation::Translate(
> +                        result.push(computed_value::ComputedOperation::TranslateX(ty));

Nit: TranslateY.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906859 [details]
Bug 1391145 - stylo: Preserve the variant of scale() values in computed transforms ;

https://reviewboard.mozilla.org/r/178582/#review183664

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906858 [details]
Bug 1391145 - stylo: Preserve the variant of translate() values in computed ;

https://reviewboard.mozilla.org/r/178580/#review183670

Oops, I think we need to modify Animate trait for TransformOperation too, no?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8906860 [details]
Bug 1391145 - stylo: Preserve the variant of rotate() values in computed transforms;

https://reviewboard.mozilla.org/r/178584/#review183672

r=me with modification for TransformOperation.animate().

Comment 16

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/18492

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed this out from autoland for frequently failing mochitests layout/style/test/test_animations.html and layout/style/test/test_animations_omta.html on Linux x64:

https://hg.mozilla.org/integration/autoland/rev/092944affb617ed74cd494a8feb7ac99fba2100a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/092944affb61

Comment 19

[Manish Goregaokar [:manishearth]]

There's probably a sign bug in the matrix stuff causing this. I'll look into this, but we can let this patch skip 57 I think. Let me know if there are objections.

If we're going to be aiming for 58 we can do the proper fix and use generics to make computed/specified transform use the same format. It's more work and a lot more churn, but the correct solution (and will lead to cleaner code).

Comment 20

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1391145 - stylo: Fix TransformOperation.animate() implementation ;

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

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8906858 [details]
Bug 1391145 - stylo: Preserve the variant of translate() values in computed ;

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

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8906859 [details]
Bug 1391145 - stylo: Preserve the variant of scale() values in computed transforms ;

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

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8906860 [details]
Bug 1391145 - stylo: Preserve the variant of rotate() values in computed transforms;

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

Comment 24

[Manish Goregaokar [:manishearth]]

No need for review, these are the same commits. Just pushing up rebased versions.

There's something fishy going on with that test failure.

When run with


@keyframes anim {
from {
    transform: rotate(0deg) scaleX(1);
}
to {
    transform: rotate(270deg) scale3d(1,1,1); 
}
}

and `animation: anim 2s linear forwards;`

the two animate identically in gecko/stylo and stylo-with-patches.


However, if I inspect it through the animation inspector and manually drag it around, the animation in stylo-with-patches is *backwards*. I'm not sure why. You can also see this by pressing play in the animation inspector and pausing halfway through.


hiro, any idea why this would happen? I can't see anything wrong in these patches, but it would be good to know where it can go wrong and how the animation inspector manipulates animations like that.

My full testcase (which makes this pretty evident) is :

<style type="text/css">

#foo #bar {
    width: 100px;
    height: 100px;
    background: red;
    animation: anim 2s linear forwards;
}

@keyframes anim {
from {
    transform: rotate(0deg) scaleX(1);
}
to {
    transform: rotate(270deg) scale3d(1,1,1); 
}
}
</style>

<div id=foo style="width:300px; height: 300px; background: yellow;">
    <div id=bar>abcd</div>

</div>

Comment 25

[Hiroyuki Ikezoe (:hiro)]

This is an interesting test case.  On Gecko we are checking each transform operations matches [1], e.g. scaleX() can be matched scale3d() so we don't fall back to decompose each matrices.  So we have to do the same thing in Animate trait. I am sorry that I missed these cases in the review.
 
[1] https://hg.mozilla.org/mozilla-central/file/a0eb21bf55e1/layout/style/StyleAnimationValue.cpp#l155

Comment 26

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 27

[Manish Goregaokar [:manishearth]]

This should be fixed by https://github.com/servo/servo/pull/18750 (not in m-c yet)

Comment 28

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/92ff0c88e94d