Closed Bug 1526847 Opened 10 months ago Closed 8 months ago

ComputeSuitableScaleForAnimation should check other transform-like properties

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

In order to support compositor animations for individual transforms, I'd like to replace the hardcode of eCSSProperty_transform with an array or property set. Most of the replacements are simple, but the scale calculation is a little bit tricky [1], because it seems we don't really do interpolation in this function, instead, we roughly calculation the scale factor. It's ok if we only have transform property. However, we want to support rotate and scale properties for layer animations, so we need a more sophisticate way to calculate it.

[1] https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/layout/base/nsLayoutUtils.cpp#396-413

Assignee: nobody → boris.chiou

For the other replacements, I will file other bugs.

Priority: -- → P3

So we can let KeyframeEffect::ContainsAnimatedScale check individual
transforms in the future.

We update ComputeSuitableScaleForAnimation in the next patch.

For now, we only have eCSSProperty_transform. However, after we support
compositor animations on individual transform and motion-path, we will
extend this list.

Note: For individual transform, the scale factors are affected by rotate and
scale properties. However, we still can use this list because the scale
factors of translate should always be (1.0, 1.0). If there is any
performance impact, we can add a new list without translate property.

Depends on D19525

Summary: ComputeSuitableScaleForAnimation should check individual transforms → AnimationValue::GetScaleValue should take individual transforms into account

hiro, I sent you review requests of this bug because I think it should be ok to land this separately from Bug 1505225. The current DisplayItemType::TYPE_TRANSFORM is still mapping {eCSSProperty_transform}, so the result of these patches should be the same as that before, until we extend the list for individual transforms in Bug 1425837.

I think we can land this, but it's not testable, right? The only way I can think of to check the result of ComputeSuitableScaleForAnimation is to run individual transform properties on the compositor and see the results of its rendering.

Yes. For now, all we can do is something like that these patches don't break the current tree, and check if the minor refactoring looks reasonable.

So for me, this can be landed lastly with some valid test cases (and I think we should).

I see. I will hold this for now. :)

Comment on attachment 9043321 [details]
Bug 1526847 - Part 1: Let GetScaleValue support individual transforms.

Revision D19525 was moved to bug 1529422. Setting attachment 9043321 [details] to obsolete.

Attachment #9043321 - Attachment is obsolete: true
Summary: AnimationValue::GetScaleValue should take individual transforms into account → ComputeSuitableScaleForAnimation should check other transform-like properties
No longer blocks: 1425837
Depends on: 1425837
Component: DOM: Animation → Layout
Status: NEW → ASSIGNED

Still trying to find a suitable test case for this. It seems we add this function for B2G in the beginning, and fix it due to some other bugs.

I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.

Just tried a scaling animation box containing some text, but unfortunately, this may not reflect the change I did because the reftest was not failed without the fix. Maybe need a trickier way. ;(

(In reply to Boris Chiou [:boris] from comment #12)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.

Just tried a scaling animation box containing some text, but unfortunately, this may not reflect the change I did because the reftest was not failed without the fix. Maybe need a trickier way. ;(

And I found a gfx issue. If we use reftest-no-flush, the edges of the animation box look blurred (AA issue, I guess) when taking the screenshot. This happens on both transform: scale(..) and scale: ... And this blur makes the result of this test intermittent.

Attachment #9043322 - Attachment description: Bug 1526847 - Part 2: Make ComputeSuitableScaleForAnimation check other transform-like properties. → Bug 1526847 - Let ComputeSuitableScaleForAnimation check other transform-like properties.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79686ffd6bac
Let ComputeSuitableScaleForAnimation check other transform-like properties. r=hiro
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.