Closed Bug 1391145 Opened 7 years ago Closed 7 years ago

stylo: translate function is shown as translate3d on animation inspector

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: hiro, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1391145 - stylo: Preserve the variant of translate() values in computed ;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1391145 - stylo: Preserve the variant of scale() values in computed transforms ;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1391145 - stylo: Preserve the variant of rotate() values in computed transforms;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1391145 - stylo: Fix TransformOperation.animate() implementation ;
59 bytes, text/x-review-board-request
Details 
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
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.)
This would be nice to fix in 57 if someone has a spare moment.
Priority: P4 → P3
(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.
Assignee: nobody → 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?
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.
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.
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.
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 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.
Attachment #8906858 - Flags: review?(hikezoe) → review+
Comment on attachment 8906859 [details]
Bug 1391145 - stylo: Preserve the variant of scale() values in computed transforms ;

https://reviewboard.mozilla.org/r/178582/#review183664
Attachment #8906859 - Flags: review?(hikezoe) → review+
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 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().
Attachment #8906860 - Flags: review?(hikezoe) → review+
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
Flags: needinfo?(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).
Flags: needinfo?(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>
Flags: needinfo?(hikezoe)
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
Flags: needinfo?(hikezoe)
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: affected → wontfix
This should be fixed by https://github.com/servo/servo/pull/18750 (not in m-c yet)
https://hg.mozilla.org/mozilla-central/rev/92ff0c88e94d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Attachment #8909978 - Flags: review?(hikezoe)
Depends on: 1424798
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: