Closed Bug 1355344 Opened 8 years ago Closed 8 years ago

stylo: Make perspective-origin animatable

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1355344 part 2 - Enable w-p-t of perspective-origin animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Following bug 1354053, I try to implement this property. It has two fields, 'vertical' and 'horizontal'. This fields's type is LengthOrPercentage.
Assignee: nobody → mantaroh
Comment on attachment 8856867 [details] Bug 1355344 part 1 - Make perspective-origin animatable on stylo. https://reviewboard.mozilla.org/r/128786/#review131300 We have already Interpolate trait for horizontal and vertical pair. Please see impl Interpolate for Position.
Comment on attachment 8856867 [details] Bug 1355344 part 1 - Make perspective-origin animatable on stylo. https://reviewboard.mozilla.org/r/128786/#review131308 ::: servo/components/style/properties/gecko.mako.rs:2008 (Diff revision 1) > + pub fn clone_perspective_origin(&self) -> longhands::perspective_origin::computed_value::T { > + use properties::longhands::perspective_origin::computed_value::T; > + use values::computed::LengthOrPercentage; > + T { > + horizontal: LengthOrPercentage::from_gecko_style_coord(&self.gecko.mPerspectiveOrigin[0]) > + .expect("clone for LengthOrPercentage failed"), The message does not have information which property failed. Should be something like this: Expected length or percentage for horizontal value of perspective-origin ::: servo/components/style/properties/longhand/box.mako.rs:1861 (Diff revision 1) > boxed=True, > creates_stacking_context=True, > fixpos_cb=True, > animation_type="normal")} > > // FIXME: This prop should be animatable Drop this comment.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > Comment on attachment 8856867 [details] > Bug 1355344 - Make perspective-origin animatable on stylo. > > https://reviewboard.mozilla.org/r/128786/#review131300 > > We have already Interpolate trait for horizontal and vertical pair. Please > see impl Interpolate for Position. You can re-use it by adding 'pub type T = Position;' in mod computed_values for perspective-origin.
Comment on attachment 8856867 [details] Bug 1355344 part 1 - Make perspective-origin animatable on stylo. https://reviewboard.mozilla.org/r/128786/#review131300 Thank you for review. I changed to using Position structure. If we use Position, we can drop ToCSS function also.
Comment on attachment 8856867 [details] Bug 1355344 part 1 - Make perspective-origin animatable on stylo. https://reviewboard.mozilla.org/r/128786/#review131808 This looks good to me. Emilio?
Attachment #8856867 - Flags: review?(hikezoe) → review+
Attachment #8856867 - Flags: review?(emilio+bugs)
Comment on attachment 8856867 [details] Bug 1355344 part 1 - Make perspective-origin animatable on stylo. https://reviewboard.mozilla.org/r/128786/#review131852 Looks great modulo that, thanks for fixing this! :) ::: servo/components/style/properties/longhand/box.mako.rs:1873 (Diff revision 3) > + use properties::animated_properties::Interpolate; > use values::computed::LengthOrPercentage; > + use values::computed::Position; > > - #[derive(Clone, Copy, Debug, PartialEq)] > #[cfg_attr(feature = "servo", derive(HeapSizeOf))] I believe the `derive(HeapSizeOf)` is unnecessary and won't build in Servo, make sure to remove this before landing it.
Attachment #8856867 - Flags: review?(emilio+bugs) → review+
Priority: -- → P1
Thanks, hiro and emilio, Before land it, I added the w-p-t of this property. hiro, I have a question about animation of 'composite=add'. In my environment(stylo), this animation doesn't work, however gecko work well. Example page is as follow: http://output.jsbin.com/nevedoh Is stylo work KeyframeEffectOptions::compsite?
Not yet (bug 1329878), but yes, it's worth adding the tests. Thanks!
Comment on attachment 8857731 [details] Bug 1355344 part 2 - Enable w-p-t of perspective-origin animation. https://reviewboard.mozilla.org/r/129672/#review132420 I think it would be nice to have test cases of percentage. ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:958 (Diff revision 1) > 'rgb(120, 120, 120) 10px 10px 10px 0px' }]); > }, property + ': shadow'); > }, > }; > > +const position = { positionType insteaf of position for consistency.
Attachment #8857731 - Flags: review?(hikezoe) → review+
Comment on attachment 8857731 [details] Bug 1355344 part 2 - Enable w-p-t of perspective-origin animation. https://reviewboard.mozilla.org/r/129672/#review132420 I added the percentage tests, however, getComopoutedStyle return calculated value(i.e. return px value when specifying percentage). So I added utility of converting calculated value from percentage.
I did miss that part 2 does not change MANIFEST.json at all. Please include the change in the commit. Thanks!
Attachment #8856867 - Attachment is obsolete: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20) > I did miss that part 2 does not change MANIFEST.json at all. Please include > the change in the commit. Thanks! Thanks, I updated manifest file. This test has passed :https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b81cc91d4cf06d2742f6ebfe7518d07d41725c&group_state=expanded
Status: NEW → ASSIGNED
Pushed by mantaroh@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5ef0eb3d0b26 part 2 - Enable w-p-t of perspective-origin animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/5ef0eb3d0b26
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: