Closed
Bug 1332657
Opened 7 years ago
Closed 7 years ago
stylo: Make transform animatable
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: boris, Assigned: hiro)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
heycam
:
review+
manishearth
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
manishearth
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
manishearth
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
We make transform non-animatable [1] in stylo. Though we still have "lengths and percentages" problem for translate [2] in servo, we can still try to enable it, and let it work on some simple animation tests. [1] https://dxr.mozilla.org/servo/rev/1f76aa6ef7ec7db0a0c40223874a72e8f4059deb/components/style/properties/longhand/box.mako.rs#1053 [2] https://github.com/servo/servo/issues/13267
Reporter | ||
Comment 1•7 years ago
|
||
After applying patches of Bug 1317209, we start to get many assertions: REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-1.html | assertion count 18 is more than expected 3 to 10 assertions REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1272475-2.html | assertion count 22 is more than expected 4 to 10 assertions Both 1272475-1.html and 1272475-2.html use *Transform* to test, so we got assertions because we don't support transform now. I think this bug can fix them.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•7 years ago
|
||
I agree with Boris. Let's see if what happens: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685 Note that about 1272475-1.html and 1272475-2.html I did confirm that they crash because of servo's interpolation results NaN value there.
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > I agree with Boris. > Let's see if what happens: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685 > > Note that about 1272475-1.html and 1272475-2.html I did confirm that they > crash because of servo's interpolation results NaN value there. Ha, looks like you are working on this bug. Cool!
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > I agree with Boris. > Let's see if what happens: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9e36395ffa404c6f07e557d1d83e4ff75cb1b685 The try result looks reasonable to me. I found some issues in conversions nsCSSValue in the patch. I will revise it soon.
Assignee | ||
Comment 5•7 years ago
|
||
Here is another try with the revised patch.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > Here is another try with the revised patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f662e2a82046edacad0f10a0f0864a9734f5cdfe
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bbfc526c10b3d31206ba3ebdaf1152b45f4e96e With some modifications against reftest-stylo.lists and crashtest.list, now the try results are green.
Assignee | ||
Comment 8•7 years ago
|
||
Filed bug 1335998 for the interpolation problem that needs reference box.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8832757 [details] Bug 1332657 - Part 1: Use more proper function to set nsCSSValue. https://reviewboard.mozilla.org/r/108982/#review110208 ::: layout/style/ServoBindings.cpp:1088 (Diff revision 1) > } > > void > Gecko_CSSValue_SetPercentage(nsCSSValueBorrowedMut aCSSValue, float aPercent) > { > - aCSSValue->SetFloatValue(aPercent, eCSSUnit_Number); > + aCSSValue->SetPercentValue(aPercent); This is eCSSUnit_Percent, not eCSSUnit_Number, fwiw. Though that is probably more correct, the previous code was probably wrong.
Attachment #8832757 -
Flags: review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8832757 [details] Bug 1332657 - Part 1: Use more proper function to set nsCSSValue. https://reviewboard.mozilla.org/r/108982/#review110220
Attachment #8832757 -
Flags: review?(cam) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8832758 [details] Bug 1332657 - Part 2: Implement clone_transform. https://reviewboard.mozilla.org/r/108984/#review110226 ::: layout/style/ServoBindings.h:302 (Diff revision 1) > nsStyleQuoteValues* Gecko_NewStyleQuoteValues(uint32_t len); > NS_DECL_THREADSAFE_FFI_REFCOUNTING(nsStyleQuoteValues, QuoteValues); > > nsCSSValueSharedList* Gecko_NewCSSValueSharedList(uint32_t len); > + > +// Gettter for nsCSSValue Getter You might want to move the GetArrayItem calls up to this block. ::: layout/style/ServoBindings.cpp:1076 (Diff revision 1) > } > > +nscoord > +Gecko_CSSValue_GetAbsoluteLength(nsCSSValueBorrowed aCSSValue) > +{ > + // SetIntegerCoordValue() which is used in Gecko_CSSValue_GetAbsoluteLength() in Gecko_CSSValue_SetAbsoluteLength() ::: layout/style/ServoBindings.cpp:1144 (Diff revision 1) > { > aCSSValue->SetCalcValue(&aCalc); > } > > +nsStyleCoord::CalcValue > +Gecko_CSSValue_GetCalc(nsCSSValueBorrowed aCSSValue) Since this function relies on the structure of the nsCSSValue that is set in nsCSSValue::SetCalcValue, it would be nice to have the logic near there. Can you add a static function to nsCSSValue to do this work, right below nsCSSValue::SetCalcValue, and call it from here? ::: layout/style/ServoBindings.cpp:1153 (Diff revision 1) > + > + const nsCSSValue::Array* array = aCSSValue->GetArrayValue(); > + MOZ_ASSERT(array->Count() == 1, > + "There should be a 1-length array"); > + > + const nsCSSValue &rootValue = array->Item(0); Nit: "&" next to type. (And in the rest of this function too.) ::: servo/components/style/properties/gecko.mako.rs:1454 (Diff revision 1) > + x => panic!("The unit should not be {:?}", x), > + } > + } > + > + let mut result = vec![]; > + if !self.gecko.mSpecifiedTransform.mRawPtr.is_null() { If it is null, don't we want to return computed_value::T(None)?
Attachment #8832758 -
Flags: review?(cam) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8832760 [details] Bug 1332657 - Part 4: Adjust reftest expectations for transform animations. https://reviewboard.mozilla.org/r/108988/#review110244
Attachment #8832760 -
Flags: review?(cam) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8832761 [details] Bug 1332657 - Part 5: Adjust crashtest expectations for transform animations. https://reviewboard.mozilla.org/r/108990/#review110246
Attachment #8832761 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
Thank you for the review! (In reply to Cameron McCormack (:heycam) from comment #16) > ::: layout/style/ServoBindings.cpp:1144 > (Diff revision 1) > > { > > aCSSValue->SetCalcValue(&aCalc); > > } > > > > +nsStyleCoord::CalcValue > > +Gecko_CSSValue_GetCalc(nsCSSValueBorrowed aCSSValue) > > Since this function relies on the structure of the nsCSSValue that is set in > nsCSSValue::SetCalcValue, it would be nice to have the logic near there. > Can you add a static function to nsCSSValue to do this work, right below > nsCSSValue::SetCalcValue, and call it from here? Added nsCSSValue::GetCalcValue there. > ::: servo/components/style/properties/gecko.mako.rs:1454 > (Diff revision 1) > > + x => panic!("The unit should not be {:?}", x), > > + } > > + } > > + > > + let mut result = vec![]; > > + if !self.gecko.mSpecifiedTransform.mRawPtr.is_null() { > > If it is null, don't we want to return computed_value::T(None)? Good catch! Thanks!
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8832758 [details] Bug 1332657 - Part 2: Implement clone_transform. https://reviewboard.mozilla.org/r/108984/#review110474 ::: servo/components/style/properties/gecko.mako.rs:1441 (Diff revision 2) > + use values::computed::LengthOrPercentage; > + > + unsafe fn get_lop(value: &structs::nsCSSValue) -> LengthOrPercentage { > + match value.mUnit { > + eCSSUnit_Pixel => { > + LengthOrPercentage::Length(Au(bindings::Gecko_CSSValue_GetAbsoluteLength(value))) Can these be methods on nsCSSValue? see gecko_bindings/sugar/nscssvalue.rs for existing methods.
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8832758 [details] Bug 1332657 - Part 2: Implement clone_transform. https://reviewboard.mozilla.org/r/108984/#review110476
Attachment #8832758 -
Flags: review?(manishearth) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8832759 [details] Bug 1332657 - Part 3: Make transform property animatable. https://reviewboard.mozilla.org/r/108986/#review110478
Attachment #8832759 -
Flags: review?(manishearth) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
Final try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b99f6973eb16a626bed4e45fd75ebc47c19537f5
Assignee | ||
Comment 32•7 years ago
|
||
Servo PR: https://github.com/servo/servo/pull/15363
Comment 33•7 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a68e62ebe7 Part 1: Use more proper function to set nsCSSValue. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/def7c6ecd132 Part 2: Implement clone_transform. r=heycam,manishearth https://hg.mozilla.org/integration/mozilla-inbound/rev/76dab2f39230 Part 4: Adjust reftest expectations for transform animations. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2e3ba1f929 Part 5: Adjust crashtest expectations for transform animations. r=heycam
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1a68e62ebe7 https://hg.mozilla.org/mozilla-central/rev/def7c6ecd132 https://hg.mozilla.org/mozilla-central/rev/76dab2f39230 https://hg.mozilla.org/mozilla-central/rev/0f2e3ba1f929
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Did part 3 land? Did it need to?
Flags: needinfo?(hikezoe)
Comment 36•7 years ago
|
||
Part 3 is Servo code only, so it doesn't land in m-c yet, thanks for checking Wes!
Flags: needinfo?(hikezoe)
You need to log in
before you can comment on or make changes to this bug.
Description
•